-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat(FileAction): add file action support #608
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
5a79f3c
to
10cc6af
Compare
10cc6af
to
36a5c10
Compare
The Same for the registering, the legacy way is registering per mimetype |
After a bit of thoughts, and seeing that there are very few number of actions I don't think we would really gain by splitting per mime. Instead of ['image/jpeg', 'image/png', ...].forEach(mime => registerAction(action, mime)) It's easier and cleaner to just do once per app registerAction(action) When opening the actions menu, then it would make it easy to just do something like this (which we would have to do anyway even if we group per mimes) <template>
<Actions>
<ActionButton v-for="action in enabled" ... />
</Actions>
</template>
<script>
const actions = getActions()
const enabled = actions.filter(action => action.enabled(this.file, this.currentView))
</script> I'm guessing most @juliushaertl any opinion? |
I think the enabled option makes sense. This way developers have one place for implementing their logic. I'd be a bit hesitant about the boolean value for default. From the API usage perspective the difference to the current way should not matter, but both the old and new approach lack way to properly handle multiple actions registering as a default. The current approach is that the last registered wins. I'm not sure about a better approach here tough. We could also allow passing a callback here so that apps can make this dependent on file attributes (like the disabled download), but this could also be skipped I guess if we consider viewer as the preferred default opener. One use case for this would be nextcloud/viewer#1440 where we would want to open pdf files with files_pdfviewer by default, except when secure view of collabora should be used. |
With this enabled boolean, this would be doable then. registerAction({
id: 'pdfviewer',
enabled(node) {
// node mime and secureview check
return boolean
},
default: true,
order: 10,
...
})
registerAction({
id: 'collabora',
enabled(node) {
// node mime check
return boolean
},
default: true,
order: 11,
...
}) That way we would return the first enabled default action ? |
The only downside this has is that app developers need to align how to handle those scenarios. But as long as we don’t decide to offer preferences for default apps or actions this seems like the most reasonable option. |
Could you elaborate? :) |
Mostly about nextcloud/viewer#2393 (and related feature requests we frequently get for Collabora) where there could be more than one app that provides a file handler and either the admin or users would prefer having the choice about the default. I don't think we need to have this as part of the initial API design, just something to keep in mind that this might be a topic in the future. Especially with a growing app ecosystem. |
Thinking about this again, I'd also say that the boolean value is good and the preference could still be handled with that. Maybe we just need to clarify that "default" only means that the registering app would like to be the default, but there are no guarantees about that due to the first-wins approach at the moment. |
Could be easily done afterwards if we have multiple defaults. |
Great! You should know this API is already being used on the trashbin branch and is working pretty great (with a few adjustments that i'll push later) 👍 🚀 |
ca1b66d
to
aeaec92
Compare
Done everyone! |
aeaec92
to
e48b188
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
Signed-off-by: John Molakvoæ <skjnldsv@protonmail.com>
e48b188
to
cb85015
Compare
RFC: nextcloud/server#21835