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

[RFC] File actions API #21835

Closed
1 of 4 tasks
skjnldsv opened this issue Jul 14, 2020 · 16 comments
Closed
1 of 4 tasks

[RFC] File actions API #21835

skjnldsv opened this issue Jul 14, 2020 · 16 comments

Comments

@skjnldsv
Copy link
Member

skjnldsv commented Jul 14, 2020

API

Issues

Currently, the file actions is super shady and not very standardised. You cannot request all actions for a file, nor dynamically load all scripts for a specific action.
This makes it hard to implement actions in other apps as the current implementation is deeply rooted into Files.

Goals

  1. Being able to register actions for a specific mime
  2. Being able to register actions for all mimes
  3. Being able to get actions & load scripts for a specific mime
  4. Being able to get actions & load scripts for all mimes

Proposal

js

/**
 * Register a file action
 * @param {FileAction} action the file action
 */
OCA.Files.Actions.register

/**
 * Get a file action for a specific mime
 * @param {string|array} [mime] optional
 */
OCA.Files.Actions.get
/**
 * Creates an instance of FileAction
 *
 * @param {object} action the action
 */
class FileAction {
    constructor({
        id: 'delete',
        name: (Fileinfo|Fileinfo[]) => 'Delete',
        icon: (Fileinfo|Fileinfo[]) => 'icon-delete',
        exec: (FileInfo) => {},
        execBatch: (FileInfo[]) => {},
        [enabled: (Fileinfo|Fileinfo[], view) => boolean]
        [order: 1],
        [default: false], // Is this the default action
        [inline: (FileInfo) => false],
    }) {}
}

php

With this, we would do the same as Sidebar or Viewer, we would create a dedicated LoadActions php event that you app can fire and every app that register actions will have to listen to this php event and addScript their own dedicated action script handler.

Questions

  • Not fond of the keys run & runMultiple, any other ideas?
  • Order should just be set automatically I guess? If not specifically specified of course?
  • How to manage priorities? Especially with default and order ?
  • Anything else?

Related issues

#9192
#5065
#12967

cc @ChristophWurst @jakobroehrl @juliushaertl @georgehrke @rullzer @MorrisJobke
cc from dedicated issues: @onny, @XA21X, @paspil, @ggeorgg, @dugite-code, @beanaroo, @janis91, @Thatoo, @szaimen, and @trendfischer @nickvergessen @jhass, @Thatoo, @mat-m, @juliushaertl, @kesselb, @sndrr, @ggeorgg

@skjnldsv skjnldsv self-assigned this Jul 14, 2020
@skjnldsv skjnldsv changed the title [rfc] FIle actions API [RFC] File actions API Jul 14, 2020
@juliusknorr
Copy link
Member

How would the proposal handle on which mimetype an action is returned and on which not? We also should have the possibility to make the action availabilty based on permissions, ideally just allow having a callback for the action itself to check if it should be presented.

@skjnldsv
Copy link
Member Author

skjnldsv commented Jul 14, 2020

How would the proposal handle on which mimetype an action is returned and on which not? We also should have the possibility to make the action availabilty based on permissions, ideally just allow having a callback for the action itself to check if it should be presented.

Right, so we should add a permission key. And then OCA.Files.Actions.get could instead take a FileInfo so you can get the actions on per-file basis? Would that be more efficient? It would be nicer to generate menus, as well as reducing the list when you pass an array of FileInfos? That way if on your three selected files you have one that is read only, we'd only return the actions matching the lowest permissions level?

@juliusknorr
Copy link
Member

I would actually prefer to have this in a more generic approach so that we are not limiting the properties on which the decision if an action is shown or not is made. Something like this:

class FileAction {
    constructor({
        id: 'delete',
        name: 'Delete',
        icon: 'icon-delete',
        run: (FileInfo) => {},
        runMultiple: (FileInfo[]) => {},
		shouldShow: (FileInfo) => {
			return FileInfo.mimetype === 'plain/text' && (FileInfo.permissions & PERMISSION_UPDATE) 
		},
		shouldShowMultiple: (FileInfo[]) => {
			return FileInfo.mimetype === 'plain/text' && (FileInfo.permissions & PERMISSION_UPDATE)
		},
    }) {}
}

Other than that, what would the inline option do?

Also there are some file actions that are rendering with more UI elements like the share entry or the files_lock app that might show an avatar and I think we should definitely still allow to provide a somehow richer interface than icon-class and text, but not sure about an ideal API there.

@skjnldsv
Copy link
Member Author

Other than that, what would the inline option do?

To be rendered inline, like in files. Comments, sharing for example :)
No idea how to proceed for this one, shall we standardise this too? have a function that returns a dom that we mount? 🤷

@juliusknorr
Copy link
Member

No idea how to proceed for this one, shall we standardise this too? have a function that returns a dom that we mount?

Yes, or provide the function with the element to render to as a reference, should work both ways.

@ChristophWurst
Copy link
Member

With this, we would do the same as Sidebar or Viewer, we would create a dedicated LoadActions php event that you app can fire and every app that register actions will have to listen to this php event and addScript their own dedicated action script handler.

This assumes that you know file actions will be required for the page that is getting rendered. If your app is active on another app's page, it might not be able to trigger this.

So in the end – to make the actions available everywhere – each app providing them would have to register at least a tiny portion of js on every page load, that can load the rest of the logic on demand.

Unless we are able to load some script on certain occasions, like even after the page load. I don't have any experience with that. It could work, but it might also be fragile. I don't know.

@skjnldsv
Copy link
Member Author

So in the end – to make the actions available everywhere – each app providing them would have to register at least a tiny portion of js on every page load, that can load the rest of the logic on demand.

Unless we are able to load some script on certain occasions, like even after the page load. I don't have any experience with that. It could work, but it might also be fragile. I don't know.

Php event to addScript with dynamic loading for components should be good I think.
Better than our LoadadditionalScript vent at least 🤷

@ChristophWurst
Copy link
Member

But addScript only adds a script to the page that is currently being rendered.

@skjnldsv
Copy link
Member Author

Isn't it the point? I missed what you wanted to say I think 🤔

@ChristophWurst
Copy link
Member

We can currently include arbitrary scripts when the page is loaded. So in order to decide if your script goes onto a a page you can either know that you need it (hard) or pessimistically load the script and leave it idle until something invokes it. This means lots of files are transferred even when the front-end does not use them.

In my opinion the better solution would be to load scripts on demand. Like the page loads, the user interacts with it for a few minutes and then BAM we need file actions. At page load we didn't know this was a requirement, so the scripts are not there. Now it would be nice if we could load them, have them register their things and then use them.

My point is that addScript can only be used for the first use case, not the latter.

@skjnldsv
Copy link
Member Author

My point is that addScript can only be used for the first use case, not the latter.

Sure, but the initial loaded script can also invoke an async bigger script, no?
It's fine to just load a tiny 1KB script that async load the full stuff when called?

In my opinion the better solution would be to load scripts on demand. Like the page loads, the user interacts with it for a few minutes and then BAM we need file actions

But how can we do that?
We'd have to call the server for a list of all the script to load for this, no?
It seems complicated?

@ChristophWurst
Copy link
Member

Sure, but the initial loaded script can also invoke an async bigger script, no?
It's fine to just load a tiny 1KB script that async load the full stuff when called?

Define "fine". It will be one script for each action. So that could be zero, one or 15 if many apps use this integration. We already have lots of files to fetch, so this just adds to the overall slowness of Nextcloud.

We'd have to call the server for a list of all the script to load for this, no?

Either we fetch a fresh list from the server or the list is sent with the initial page load.

It seems complicated?

Yep, that is the big downside 😟

@rullzer
Copy link
Member

rullzer commented Jul 17, 2020

So either lazy or via initial state I guess?

  • Apps register they provide file actions
  • Either initial state or via some endpoint the 'main' fileactions code can load this list (with the routes where to fetch the files)
  • On invokation, or when the page is fully loaded. This list is fetched/consumed
  • The required JS files are fetched on demand

I have no idea how to do it in js. But that should work right?

@skjnldsv
Copy link
Member Author

skjnldsv commented Jul 17, 2020

The only thing I can think of is webpack async loading.
But you'd still need a tiny script waiting for a call.

I'd honestly just go for a simple php Event to fire that call all the Actions scripts our devs registered?
It will still be much faster than the weird complete 1561TB of js we load on every files app that is rendered! 😭

@skjnldsv

This comment has been minimized.

@skjnldsv skjnldsv removed their assignment Jul 5, 2021
@skjnldsv skjnldsv mentioned this issue Jul 20, 2021
8 tasks
@skjnldsv
Copy link
Member Author

skjnldsv commented Dec 1, 2021

cc @nextcloud/server

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

4 participants