This patch adds an option to show the selected file(s), or the file currently being viewed in the file browser. It opens the default file browser and highlights the files.
Details
Details
- Reviewers
ngraham rkflx - Group Reviewers
KDE Applications - Commits
- R260:1db8e8953835: Add option to show file in File Manager
- Select a file and right click on it, opening the contextual menu. Select the "Show in File Manager" option. The default file manager should open and highlight the file.
- While viewing a file, right click on it, opening the contextual menu. Select the "Show in File Manager" option. The default file manager should open and highlight the file.
It also shows the option under the File menu.
Diff Detail
Diff Detail
- Repository
- R260 Gwenview
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Comment Actions
Hah, you read my mind! I was planning to do this exact thing sometime soon, and then do the same thing for Okular and Kate.
In fact, I'm starting to think we should make this feature into a KStandardAction to reduce code duplication across apps and keep the text and keyboard shortcut consistent. Thoughts?
Comment Actions
Putting aside the KStandardAction idea, can you change the text to "Open containing folder"? That seems to be the standard text for this feature. I prefer "Show in folder", but that's a debate for another time, and there's value in consistency. :)
Comment Actions
I'm glad you like it, and that it is a change that was planned for the future. It is definitely a feature I was missing. I've updated the option's title. Also, the new option is included in the regular File menu, I've attached a screenshot of it.
Comment Actions
There is certainly some overlap or rather confusion in functionality with the Browse folder button. I pondered whether it would make sense to transform this into Open With → Dolphin, but this has also its drawbacks so I guess your proposal is good enough. Also it is debatable whether the menu entry should rather follow the Properties entry regarding its disabled state by using setEnabled(dirUrlIsValid || urlIsValid) (there is already some inconsistency here, so maybe out-of-scope for this patch).
Anyway, code LGTM, just some minor things to fix (see below). I could not break it when testing and commit message reads excellent. Thanks for your contribution to Gwenview :)
Maybe wait some more days to see whether there are any other comments showing up, then @ngraham or I can land this on your behalf to the master branch.
Does not sound too wrong, but maybe there's also some overlap or integration opportunity with the new Share menu powered by the Purpose framework that's popping up in a lot of apps nowadays (e.g. D8244)? I could imagine this replacing Open With and Open Containing Folder in a lot of places instead of cluttering the menus and toolbars even further.
| app/fileopscontextmanageritem.cpp | ||
|---|---|---|
| 251 | This is equivalent to selectionNotEmpty, isn't it? If so, use it. | |
| 424 | Use urlList()directly instead of list? | |
Comment Actions
"Share" is also nice, but something else entirely. Por que no los dos? ;) I'm in favor of merging this, and then if and when we (I?) make this into a KStandardAction, the code can be replaced with that later.
Comment Actions
- Use selectionNotEmpty instead of explicit comparison
- Use urlList() directly instead of using a variable to hold it
Comment Actions
Rodrigo: Thanks :)
Por que no los dos?
Well, there's only a limited amount of menu entries available before it becomes "too much". Anyway, I don't feel too strongly about it. Maybe I thought too much of Android's intents or a potential inter-application-clipboard/sharing system on steroids. But your approach in starting small is probably more effective…
Comment Actions
I'm going to land this, but first, @rpelorosso, I need to know your email address. I couldn't find you on https://identity.kde.org/index.php?r=people/index. Would you mind also updating that too?
Comment Actions
Actually I found it from your message to the kde-devel mailing list. Still, it would be good to update identity.kde.org.