-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Modify do_action so default ctrl-click opens tab #20185
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
@azul I commented in the issue to clarify: Yes, ctrl-click should be the "Open in new tab" action. :)
c9dec70
to
0345ad0
Compare
var mime = this.fileActions.getCurrentMimeType(); | ||
var type = this.fileActions.getCurrentType(); | ||
var permissions = this.fileActions.getCurrentPermissions(); | ||
var action = this.fileActions.getDefault(mime, type, permissions); |
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.
none of these were used anywhere.
Somehow this seems to break the right click menu for me. |
Failing tests seem to be the same as in master. |
The right click menu is added by the files_rightclick app. If you want to test it on current master with right click, you will need to add the app and enable it. |
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.
The action that occurs from clicking on files to download seems to be broken now. Could you take a look at that?
Ahh.. got it. thanks! 😄 |
fileActions.getCurrentDefaultFileAction() returns the default file action for the currently selected file. There were a number of places querying for the mime, type and permissions of that file first to then query for the default action. Signed-off-by: Azul <azul@riseup.net>
Should be fixed. This is getting close. I would like to add a test for the new urls to make sure they actually trigger the action. Will probably look into that tomorrow. |
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.
Noticed a couple more things.
Ctrl+click
on a directory in a public share doesn't go into that directory and opens a new tab with the same page.Ctrl+click
on a file in a public share to be opened by thetext
app doesn't work as expected. This is more of an edge case though so I'd say it'd be okay to fix this in a follow up.
Found another issue... @jancborchardt what would the desired behavior be when:
I see a number of options:
I guess 2. would be the right thing to do. But maybe i am missing something. tracking this here: #20394 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Okay... i figured it out and updated the pull request accordingly. The |
In a file list files with a default action have an href that will trigger the action. This way ctrl-click and middle button click open the default action in a new tab. In order to achieve this a new param `openfile` was introduced to the files app. It will make the files app trigger the default action for the file in question. This also allows linking to file content rather than just the details display. Introduce fileList.getDefaultActionUrl() to create a link with that param set. It's overwritten in the trashbin fileList so that anchors continue to have `#` as a href. Fix the link generation for subfolders of public shares: 58a87d0 was the last commit that touched the linkTo function in public.js. It included the params as arguments to the generateUrl function. Turns out this completely ignores the dir parameter now. The inclusion was reverted in other places so revert it here as well. Also change `dir` to `path` in the param as that is respected when following the link. Add Test for the new link url for files with default action. Remove test for multiple selects with ctrl-click as that is not what we are doing anymore. Signed-off-by: Azul <azul@riseup.net>
I renamed the @gary-kim I would say this is ready to be merged. I'd be okay with adding more tests but i'd prefer to just see this included 😄 . In terms of tests i think it would be nice to test that the generated urls actually do what they are supposed to. I really like that the generated url with However i have no idea about how to write such a test. If you can point me to an example i'd also be happy to take it from there. |
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.
LGTM at this point
Rebase and whitespace fix of #16530
New pull request to see if build succeeds.