In that context menu, the 'Edit' entry (when righ-clicking on a 'Place', not on a 'Device') uses the same icon that is elsewhere used for 'Properties'. Could you suggest what to do here, VDG ?

Can you please share a screenshot or before/after image that shows your changes?

I would not be opposed to this change. Any other thoughts?

This fixes an ancient bug and plugs an obvious hole (you right-click on a disk and then wonder "Where's the properties menu item?". +1. Will review in detail later today.

Note that I don't know if it is a good idea to add this 'Properties' entry to Places in addition to Devices.
Maybe it is useful to Devices alone. Plus it adds this problem of identical icon with Edit, as pointed in my first comment.

I would not be opposed to adding a Properties menu item for other Places panel items, too. But you're right: the Edit... menu item is using the wrong icon. It should be changed (in another patch) to edit-entry.

One more thing before I do the formal review: KIO also has Places Panel code. Could this be done there so we don't have code duplication? Or if code duplication is unfortunately necessary, could we get (in another patch obviously) the same feature added to the KIO Places Panel implementation too?

Now you are talking about something I don't know about yet. I will look into it.

Should 'Properties' be last item in the menu?

FWIW, I've filed a task to track the places panel code unification work: T9795: Use Places Panel code from KIO instead of private implementation

Actually, I retract previous comment, it should be the last entry in the first section ('Open in ...'), and the separator that is shown above the 'Edit...' entry should also be visible when the 'Edit...' is hidden.

Open in Window
Open in Tab
Properties ...
-------------------
(Edit....)
Hide Entry
Hide Section

Works great! We should figure out if this should be here or in KIO though.

In D15929#336153, @cfeck wrote:

Actually, I retract previous comment, it should be the last entry in the first section ('Open in ...'), and the separator that is shown above the 'Edit...' entry should also be visible when the 'Edit...' is hidden.

Does this mean that we should also add a separator between Properties and the 'hiding' entries in the Device context menu ?

Unmount
-------------------
Open in Window
Open in Tab
Properties ...
-------------------         <- to be added ?
Hide Entry
Hide Section

I'm okay with doing this here for now as long as we get another patch for KIO as well so that we don't lose any features when we move Dolphin to using KIO's own implementation.

The KIO places panel code is uses everywhere but dolphin, so it can be tested in a file open/save dialog, Gwenview, etc.

Yes, I would prefer if the 'Hide' and 'Edit' entries are always preceeded by the separator, separating the items that act on the actual device and the items that act on the places entry.

But it looks like the separator is a completely unrelated change, so I am fine with a separate diff to fix that.

Maybe I should actually read the code ... I guess what I want is that you move the 'Properties' above the seperator, not after the 'Edit ...' entry.

I'm pretty sure this can be implemented in KFilePlacesModel without the need to duplicate the feature here in dolphin.

In D15929#336818, @cfeck wrote:

Maybe I should actually read the code ... I guess what I want is that you move the 'Properties' above the seperator, not after the 'Edit ...' entry.

Just to be sure I understand you, you mean like this (for Places):

Properties ...
Open in Window
Open in Tab
-------------------
(Edit....)
Hide Entry
Hide Section

And for Devices:

Unmount
-------------------
Properties ...
Open in Window
Open in Tab
Hide Entry
Hide Section

Or should we also add a separtor between 'Open's and 'Hide's in the Devices case ?
(and yes, probably better in another patch, since this one is not sure to make its way in anyway)

No, the see the first 'screenshot' I added.

Sorry, I misplaced the Properties entry indeed.

But then I am coming back to my question (in my first 'screenshot' as well): shouldn't we add a sperator there ?
Sorry, we are going a bit in circle :)

There already is a separator (original line 202), but it is (for whatever reason) not always added. You can remove the if for the separator either in this commit, or in a separate patch, because it is unrelated to the addition of the Properties item.

What I am trying to get fixed before you commit this is the placement of the new Properties item. Order should be:

Open in Window
Open in Tab
(Properties)
--------
(Edit...)
(Remove)
Hide
Hide Section

If I understand your patch, currently the order would be:

Open in Window
Open in Tab
--------
(Edit...)
(Properties)
(Remove)
Hide
Hide Section

(Items in parens are not always visible)

I hope this clarifies the confusion I added by my previous comments.

Very clear, there is indeed something to correct in my patch and I will.
But my question was on a different point. The _Devices_ context menu is slightly different than the _Places_ one. I was wondering if we should add a separator between the 'Open in ..." and the 'Hide ...' entries there.