Move "Open" actions to the top of the context menu for files
ClosedPublic

Authored by ngraham on Apr 2 2018, 2:27 PM.

Details

Summary

This patch moves the "Open", "Open With", and "Open in" items to the top of the context menu to reap the following benefits:

  • Move the "Open" and "Open With" items closer to the top since they're commonly used items, and right now they're buried in the middle of the menu
  • Group related functionality
  • Consistency with other common platforms (macOS Finder and Windows Explorer both have these items at the top of the context menu)

For folders, the "Open With" entries are moved higher, but not all the way to the top, since the "open in New tab/folder" entries are more useful.

Test Plan

Tested all menu items in the context menu for files, folders, and links; all still work.

Context menu for single file:

Context menu for multiple files:

Context menu for single folder:

Context menu for multiple folders:

Context menu for symlink to folder:

Diff Detail

Repository
R318 Dolphin
Branch
move-open-actions-to-the-top (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
ngraham requested review of this revision.Apr 2 2018, 2:27 PM
ngraham created this revision.
ngraham edited the test plan for this revision. (Show Details)Apr 2 2018, 2:29 PM
elvisangelaccio requested changes to this revision.Apr 8 2018, 11:03 AM
elvisangelaccio added a subscriber: elvisangelaccio.
src/dolphincontextmenu.cpp
201–218

In the folder case I'd put "Open With" after these actions, without a separator.

On Windows "Open With" is always at the end of the first section, before the first separator.

291–292

This is now already called at line 196.

488–490

This refactoring removes the "Open With" submenu from the viewport context menu.

This revision now requires changes to proceed.Apr 8 2018, 11:03 AM
abetts added a subscriber: abetts.Apr 8 2018, 2:25 PM

I like this idea! It is consistent with what most people will look to do by right clicking an item.

ngraham updated this revision to Diff 32058.Apr 13 2018, 1:16 PM

Only put the "Open With" entries on the top of the menu for files, not folders

ngraham retitled this revision from Move "Open" actions to the top of the context menu to Move "Open" actions to the top of the context menu for files.Apr 13 2018, 1:20 PM
ngraham edited the summary of this revision. (Show Details)
ngraham edited the test plan for this revision. (Show Details)
ngraham marked 3 inline comments as done.Apr 13 2018, 1:23 PM

Still need to tweak the separators though. The "Open With" entry itself seems to add some all on its own...

abetts accepted this revision.Apr 13 2018, 4:35 PM

@elvisangelaccio, is this acceptable now?

markg added a subscriber: markg.Apr 19 2018, 3:07 PM

+1
I like it :)

markg added a comment.Apr 19 2018, 7:24 PM

I'm increasingly doubting whether i like this change.. Sure, it gives more room.
But the downside is that the actions (who were directly above the view they were controlling) are now above the places.
That might not be ideal as it now can be interpreted that the places are reloaded, or once place back/forward...

Just voicing my concern, not changing my +1 :) (yet)

I'm increasingly doubting whether i like this change.. Sure, it gives more room.
But the downside is that the actions (who were directly above the view they were controlling) are now above the places.
That might not be ideal as it now can be interpreted that the places are reloaded, or once place back/forward...

Just voicing my concern, not changing my +1 :) (yet)

Is this comment intended for this patch, or a different one...?

markg added a comment.Apr 20 2018, 2:17 PM

I'm increasingly doubting whether i like this change.. Sure, it gives more room.
But the downside is that the actions (who were directly above the view they were controlling) are now above the places.
That might not be ideal as it now can be interpreted that the places are reloaded, or once place back/forward...

Just voicing my concern, not changing my +1 :) (yet)

Is this comment intended for this patch, or a different one...?

Hahaha, you're sharp! (i apparently not when i wrote that).
It was for D12333

elvisangelaccio requested changes to this revision.Apr 21 2018, 3:38 PM

UI looks good now, but I think there is still room for improvements in the code. See inline comment.

src/dolphincontextmenu.cpp
493–499

The addServiceActions() function doesn't make sense anymore. We can move the fileItemActions.setParentWidget(m_mainWindow) call after we define the fileItemActions object, so we can even remove this line from the new addOpenWithActions() function.

Then we can just call fileItemActions.addServiceActionsTo(this) instead of calling addServiceActions().

This revision now requires changes to proceed.Apr 21 2018, 3:38 PM
ngraham updated this revision to Diff 32751.Apr 21 2018, 9:34 PM

Simplify this code a bit

ngraham marked an inline comment as done.Apr 21 2018, 9:34 PM
elvisangelaccio accepted this revision.Apr 22 2018, 10:57 AM
This revision is now accepted and ready to land.Apr 22 2018, 10:57 AM

Simplify this code a bit

Are we ready to approve?

Yeah, I'm gonna land this today.

ngraham closed this revision.Apr 22 2018, 3:41 PM

The Open and Open With actions are now missing in the search results view

Restricted Application added a project: Dolphin. · View Herald TranscriptMay 14 2018, 8:32 AM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript

Oops, sorry about that. Will submit a patch that corrects the issue today or tomorrow.

Fixed with https://cgit.kde.org/dolphin.git/commit/?id=56e8e77e683608e101fe8ed79de8668e5db2e223. It was a trivial fix so I just committed directly; hope folks don't mind.