D17012: Hide the Rename context menu entry when impossible

[Thomas Surrel]

thsurrel created this revision.
thsurrel added reviewers: Dolphin, VDG.
Herald added a project: Dolphin.
Herald added a subscriber: kfm-devel.
thsurrel requested review of this revision.

REVISION SUMMARY
We already hide the 'Move to Trash' or 'Delete' entries
when selection is not deletable. This patch does the same
for renaming.

TEST PLAN
Right click on /home. The context menu should not contained
the 'Rename' entry.

REPOSITORY
R318 Dolphin

BRANCH
arc_hide_rename (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D17012

AFFECTED FILES
src/dolphincontextmenu.cpp

To: thsurrel, #dolphin, #vdg
Cc: kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Thomas Surrel]

thsurrel updated this revision to Diff 45801.
thsurrel added a comment.


Fix

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17012?vs=45800&id=45801

BRANCH
arc_hide_rename (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D17012

AFFECTED FILES
src/dolphincontextmenu.cpp

To: thsurrel, #dolphin, #vdg
Cc: kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Roman Gilg]

romangg added a comment.


From a Ux point of view Ui elements should be disabled instead of hidden if they are not available but the user expects them to be there. The "Rename" entry currently gets disabled. You think it makes more sense to hide it instead?

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D17012

To: thsurrel, #dolphin, #vdg
Cc: romangg, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Thomas Surrel]

thsurrel added a comment.


'Delete' and 'Rename' entries are in the same section, but one gets disabled when the other one is hidden. I think they should behave the same way.
But should we disable (but show) 'Delete' or hide 'Rename', I can fix the patch to go one way or the other.

I agree it is usually better to only disable an entry that is not available. One argument in favor of hiding is that lot of things are already disabled usually in that situation (Cutting, Create new), it makes the context menu a bit cluttered.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D17012

To: thsurrel, #dolphin, #vdg
Cc: romangg, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added a comment.
This revision now requires changes to proceed.


Yeah, we prefer to disable inapplicable menu items rather than hiding them. See https://hig.kde.org/components/navigation/menubar.html#appearance

My vote is to re-work this patch to do that instead.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D17012

To: thsurrel, #dolphin, #vdg, ngraham
Cc: ngraham, romangg, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Roman Gilg]

romangg added a comment.

Post by Nathaniel Graham
My vote is to re-work this patch to do that instead.

Dolphin does this already. Might be an idea to only disable the Delete or Move to trash entry though. Not sure.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D17012

To: thsurrel, #dolphin, #vdg, ngraham
Cc: ngraham, romangg, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Thomas Surrel]

thsurrel updated this revision to Diff 45813.
thsurrel added a comment.


Show Delete entry instead of hiding Rename

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17012?vs=45801&id=45813

BRANCH
arc_hide_rename (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D17012

AFFECTED FILES
src/dolphincontextmenu.cpp

To: thsurrel, #dolphin, #vdg, ngraham
Cc: ngraham, romangg, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Thomas Surrel]

thsurrel retitled this revision from "Hide the Rename context menu entry when impossible" to "Show the Delete context menu entry even when disabled".
thsurrel edited the summary of this revision.
thsurrel edited the test plan for this revision.

REPOSITORY
R318 Dolphin

REVISION DETAIL
https://phabricator.kde.org/D17012

To: thsurrel, #dolphin, #vdg, ngraham
Cc: ngraham, romangg, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Nathaniel Graham]

ngraham accepted this revision.
ngraham added a comment.
This revision is now accepted and ready to land.


Perfect, much better now!

REPOSITORY
R318 Dolphin

BRANCH
arc_hide_rename (branched from master)

REVISION DETAIL
https://phabricator.kde.org/D17012

To: thsurrel, #dolphin, #vdg, ngraham
Cc: ngraham, romangg, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Thomas Surrel]

This revision was automatically updated to reflect the committed changes.
Closed by commit R318:1340e9854875: Show the Delete context menu entry even when disabled (authored by thsurrel).

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17012?vs=45813&id=45815

REVISION DETAIL
https://phabricator.kde.org/D17012

AFFECTED FILES
src/dolphincontextmenu.cpp

To: thsurrel, #dolphin, #vdg, ngraham
Cc: ngraham, romangg, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov