Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add openWith function to OCA.Viewer #1273

Merged
merged 3 commits into from
Jul 14, 2022
Merged

Add openWith function to OCA.Viewer #1273

merged 3 commits into from
Jul 14, 2022

Conversation

Raudius
Copy link
Contributor

@Raudius Raudius commented Jun 28, 2022

Adds the function OCA.Viewer.openWith(handler, filePath, options)

The function can be used for opening files with the specified handler. This functionality would be needed for cases where a handler may not be suitable as the default option for a mime-type (e.g. collabora and PDFs nextcloud/richdocuments#1894)

@Raudius Raudius requested review from juliusknorr, szaimen and skjnldsv and removed request for juliusknorr and szaimen June 28, 2022 08:24
@Raudius Raudius added enhancement New feature or request 3. to review Waiting for reviews labels Jun 28, 2022
@szaimen szaimen added this to the Nextcloud 25 milestone Jun 28, 2022
@skjnldsv
Copy link
Member

So, you would just register a new action called Open with Collabora for example?
Would you share some implementations or a bit more code?

I assume this is an issue about default registration priority?

@Raudius Raudius requested a review from julien-nc June 28, 2022 09:41
@Raudius
Copy link
Contributor Author

Raudius commented Jun 28, 2022

@skjnldsv

So, you would just register a new action called Open with Collabora for example?

Yes, there is a separate action in the Files app for PDF files that should open the file in the richdocuments viewer.
image

Would you share some implementations or a bit more code?

With the current PR implementation we can do something like this when clicking on the Open with Collabora button, and this will open the file with the Office component (assuming it has been registered):

OCA.Viewer.openWith(
  {
      id: 'richdocuments',
      component: Office,
      theme: 'light',
  }, 
  { path: context.fileInfoModel.getFullPath() }
)

I assume this is an issue about default registration priority?

Right now in Collabora we don't register the PDF mime with the viewer, but we want to open PDFs if explicitly chosen through the actions menu.

src/views/Viewer.vue Outdated Show resolved Hide resolved
src/services/Viewer.js Outdated Show resolved Hide resolved
@Raudius
Copy link
Contributor Author

Raudius commented Jun 29, 2022

@skjnldsv
Thanks for the review, after your feedback I changed some things around.

It didnt make much sense to pass the full handler to the openWith function so I changed it so we only need to pass the handler ID:

OCA.Viewer.openWith(
  'richdocuments', 
  { path: context.fileInfoModel.getFullPath() }
)

Also previously I immediately cleared the override handler after opening the file. But it makes more sense to clear it (as you suggested) when the viewer is closed, in the case that a file-list is passed to the viewer.

@Raudius Raudius requested a review from skjnldsv June 29, 2022 13:31
@szaimen
Copy link
Contributor

szaimen commented Jul 1, 2022

/compile amend /

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works and code looks sane 👍

@juliusknorr
Copy link
Member

@skjnldsv Mind to have another quick look as well?

@julien-nc
Copy link
Member

/compile amend /

Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the liberty to add 1ac581f
to avoid confusion between the selected handler and the one in the find lambda function.

Works fine with nextcloud/richdocuments#2298 👍
Code looks good.

Raudius and others added 3 commits July 14, 2022 15:24
Signed-off-by: Raul <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
Signed-off-by: Julien Veyssier <[email protected]>
Signed-off-by: nextcloud-command <[email protected]>
Signed-off-by: Julius Härtl <[email protected]>
@juliusknorr juliusknorr merged commit af58bed into master Jul 14, 2022
@juliusknorr juliusknorr deleted the enh/open_with branch July 14, 2022 20:13
@Raudius
Copy link
Contributor Author

Raudius commented Jul 20, 2022

/backport to 24

@backportbot-nextcloud backportbot-nextcloud bot added the backport-request Pending backport by the backport-bot label Jul 20, 2022
@backportbot-nextcloud
Copy link

The backport to 24 failed. Please do this backport manually.

Raudius pushed a commit that referenced this pull request Jul 20, 2022
Add `openWith` function to OCA.Viewer
Raudius pushed a commit that referenced this pull request Jul 20, 2022
Add `openWith` function to OCA.Viewer

Signed-off-by: Raul <[email protected]>
Raudius pushed a commit that referenced this pull request Jul 20, 2022
Add `openWith` function to OCA.Viewer

Signed-off-by: Raul <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews backport-request Pending backport by the backport-bot enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants