BUG: 181880
Details
- Reviewers
ngraham elvisangelaccio - Group Reviewers
Dolphin Plasma VDG - Commits
- R318:3f41cd9c0060: Add a 'Properties' entry in the Places panel context menu
Right-click on a place or on a device in the Places panel now show a Properties entry.
Clicking on it will open its folder property.
Diff Detail
- Repository
- R318 Dolphin
- Branch
- arc_properties (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 3586 Build 3604: arc lint + arc unit
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 ?
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?
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.
src/panels/places/placesitemmodel.h | ||
---|---|---|
52 | Is this necessary? |
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.
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)
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.
src/panels/places/placesitemmodel.cpp | ||
---|---|---|
65 | Not needed | |
313 | Check for device against nullptr again. | |
338 | Don't use parent when it is not needed, Qt::WA_DeleteOnClose is fair enough. | |
342 | Same as above. | |
src/panels/places/placesitemmodel.h | ||
236 | Not needed. | |
src/panels/places/placespanel.cpp | ||
175 | Remove unrelated changes. | |
219 | Remove white-space line |
Fixes per anthonyfieroni comments. Thanks for the review
Moved the new 'Properties' entry above 'Edit'
Simplified filtering by comparing scheme to "file"
Code looks nice and clean now, and it works well. Maybe wait for @elvisangelaccio and/or @anthonyfieroni to give their thumbs-up as well before landing the patch.
Looks like no, unfortunately. D15973 does not automatically result in Dolphin getting the feature.
src/panels/places/placesitemmodel.cpp | ||
---|---|---|
302 ↗ | (On Diff #42983) | The dialog needs a parent, otherwise the window stack will be wrong. |
src/panels/places/placesitemmodel.cpp | ||
---|---|---|
302 ↗ | (On Diff #42983) | On the corresponding line in a previous version, anthonyfieroni had commented: "Don't use parent when it is not needed, Qt::WA_DeleteOnClose is fair enough." |
src/panels/places/placesitemmodel.cpp | ||
---|---|---|
302 ↗ | (On Diff #42983) | Yes, it is needed. In general, any dialog window needs a parent to preserve the window stacking. To check it:
Without parent, the properties dialog will be behind the dolphin window (which is wrong). If you repeat the steps using a properties dialog from the dolphin view instead, the dialog will be still visible. |
dfaure's comment from D15973 also applies here: it is better to show the dialog
from the view instead of the model.