Add "Add to Places" action to file menu
ClosedPublic

Authored by ngraham on Jun 28 2019, 7:38 PM.

Details

Summary

It's recommended that actions available in context menus be available in the main menu
as well for discoverability's sake. This patch does so for the "Add to Places" action.

The action is moved over to the main window, and accessed in the context menu via the
actionCollection it lives in.

BUG: 390757
FIXED-IN: 19.08.0

Test Plan
  • Action still works
  • Action still appears in context menu when relevant
  • Action in the File menu only becomes enabled when only a single directory is selected or nothing is selected

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ngraham created this revision.Jun 28 2019, 7:38 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptJun 28 2019, 7:38 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
ngraham requested review of this revision.Jun 28 2019, 7:38 PM
ngraham updated this revision to Diff 60807.Jun 28 2019, 7:40 PM

Bump the version on the rc file

I only noticed after finishing the work that there was already an open patch for this: D12605. :(

Nonetheless, I think my patch here is the way forward because it is more technically correct than D12605, does not set a controversial default shortcut, and because the author of that patch has since disappeared.

meven edited the test plan for this revision. (Show Details)Jun 29 2019, 7:32 AM
ngraham edited the summary of this revision. (Show Details)Jul 24 2019, 8:14 PM
ngraham added a subscriber: elvisangelaccio.

@elvisangelaccio Ping. As it has a string change, this has now missed 19.08. :(

elvisangelaccio requested changes to this revision.Jul 28 2019, 2:48 PM

Code looks good, but I spotted a regression: 'Add to Places' is now disabled if you don't select any folder. Currently it is possible to add the current folder as place without the need for any selection.

This revision now requires changes to proceed.Jul 28 2019, 2:48 PM
ngraham updated this revision to Diff 63007.Aug 2 2019, 10:23 PM
  • Re-enable action when nothing is selected
  • Display name of item that will be added to Places for clarity, to support the above
ngraham edited the test plan for this revision. (Show Details)Aug 2 2019, 10:24 PM
elvisangelaccio requested changes to this revision.Aug 3 2019, 10:33 AM

There seems to be another issue: if I run dolphin <some folder> from cmd line I always get Add 'file' to place, no matter what the folder is.

src/dolphincontextmenu.cpp
385–386

The action variable is no longer used, please remove it.

This revision now requires changes to proceed.Aug 3 2019, 10:33 AM
ngraham updated this revision to Diff 63750.Aug 14 2019, 5:36 PM

I hate you arc

ngraham updated this revision to Diff 63751.Aug 14 2019, 5:38 PM
ngraham marked an inline comment as done.

Remove unused variable

There seems to be another issue: if I run dolphin <some folder> from cmd line I always get Add 'file' to place, no matter what the folder is.

I traced this down to a pre-existing issue in DolphinViewContainer::url() returns a URL without the trailing slash stripped, and url().fileName() as called in DolphinViewContainer::placesText() returns an empty string if a trailing slash in the URL (this is a documented behavior in QUrl::fileName(): https://doc.qt.io/qt-5/qurl.html#fileName). When you navigate around Dolphin normally, the URL returned by url() has its trailing slash stripped already, but when you open Dolphin with a path, the trailing slash is not stripped, so the behavior is different and the text is wrong.

I think this should be fixed in another patch because it is a pre-existing issue. I would have submitted a patch to fix it already, but I wanted to discuss with you where you think the appropriate fix should be made. DolphinViewContainer::url() could always strip the trailing slash before returning the URL, or perhaps command-line input could have its trailing slash stripped before being passed around in Dolphin. Or both, for safety. What do you think?

Either way I think this patch is ready to land. We can do it now, or else we can wait for the other issue to be fixed first.

elvisangelaccio accepted this revision.Sun, Sep 1, 8:37 PM

There seems to be another issue: if I run dolphin <some folder> from cmd line I always get Add 'file' to place, no matter what the folder is.

I traced this down to a pre-existing issue in DolphinViewContainer::url() returns a URL without the trailing slash stripped, and url().fileName() as called in DolphinViewContainer::placesText() returns an empty string if a trailing slash in the URL (this is a documented behavior in QUrl::fileName(): https://doc.qt.io/qt-5/qurl.html#fileName). When you navigate around Dolphin normally, the URL returned by url() has its trailing slash stripped already, but when you open Dolphin with a path, the trailing slash is not stripped, so the behavior is different and the text is wrong.

I think this should be fixed in another patch because it is a pre-existing issue. I would have submitted a patch to fix it already, but I wanted to discuss with you where you think the appropriate fix should be made. DolphinViewContainer::url() could always strip the trailing slash before returning the URL, or perhaps command-line input could have its trailing slash stripped before being passed around in Dolphin. Or both, for safety. What do you think?

I'm a bit scared to strip trailing slashes from the url() of the view.

I think we should fix placesText() instead, see D23654.

Either way I think this patch is ready to land. We can do it now, or else we can wait for the other issue to be fixed first.

Yes, feel freel to land it.

src/dolphinui.rc
2

Needs rebase.

This revision is now accepted and ready to land.Sun, Sep 1, 8:37 PM
ngraham marked an inline comment as done.Sun, Sep 1, 9:04 PM
This revision was automatically updated to reflect the committed changes.