Improve Ux for the places panel's hidden items
ClosedPublic

Authored by rizzitello on Nov 8 2018, 9:16 PM.

Details

Summary

BUG: 400860
FIXED-IN: 18.12.0

Clean up the context menu for the places panel.

  • Change Text "Show All Entries" -> "Show Hidden Places"
  • Use State dependent icon (like hidden files)
  • Disable instead of hide if not places are hidden.
  • Toggle to unchecked if last item of group is unhidden.

Create a copy of this "Show Hidden Places" entry in the main dolphin menu View->Places.

Test Plan

With Hidden Places



Context Menu:

Without Hidden Places.


Diff Detail

Repository
R318 Dolphin
Branch
showHiddenPlaces
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 5033
Build 5051: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
veqz added a comment.Nov 8 2018, 10:18 PM

Awesome work! :)

elvisangelaccio requested changes to this revision.Nov 8 2018, 10:25 PM
elvisangelaccio added a subscriber: elvisangelaccio.

Makes sense, thanks

src/panels/places/placespanel.cpp
67

Why drop "Show" from the label though?

src/panels/places/placespanel.h
45

Please create the action/checkbox in DolphinMainWindow and just add a public slot here in the panel.

This revision now requires changes to proceed.Nov 8 2018, 10:25 PM
rizzitello updated this revision to Diff 45144.Nov 8 2018, 11:15 PM
rizzitello marked 2 inline comments as done.
  • Elvis' Suggestion
rizzitello added inline comments.Nov 8 2018, 11:19 PM
src/panels/places/placespanel.cpp
67

This puts its text in line with the hidden files entry that is just "Hidden Files".

cfeck added a subscriber: cfeck.Nov 8 2018, 11:20 PM
cfeck added inline comments.
src/panels/places/placespanel.cpp
67

What Elvis meant was Hidden PlacesShow Hidden Places.

cfeck added inline comments.Nov 8 2018, 11:23 PM
src/panels/places/placespanel.cpp
67

Maybe. Places Panel's context menu says "Show All Entries", not "All Entries", though.

rizzitello added inline comments.Nov 8 2018, 11:24 PM
src/panels/places/placespanel.cpp
67

It no longer does with this diff. They actions match (as they were one before my last update)

rizzitello edited the summary of this revision. (Show Details)Nov 8 2018, 11:28 PM
rizzitello edited the test plan for this revision. (Show Details)
cfeck added inline comments.Nov 8 2018, 11:29 PM
src/panels/places/placespanel.cpp
67

Oh, you are re-using the action, that's nice. I will retract all my comments and let maintainer decide.

I would prefer 'Show Hidden Files" and "Show Hidden Places" for clarity over brevity. We also say "Show Menubar", "Show Filter Bar", etc.

rizzitello updated this revision to Diff 45145.Nov 8 2018, 11:54 PM
rizzitello marked 4 inline comments as done.

clean up older diff code

rizzitello edited the test plan for this revision. (Show Details)Nov 8 2018, 11:56 PM

+1 for making the text say "Show Hidden Places".

Very impressive speed on this patch. :)

rizzitello updated this revision to Diff 45146.Nov 9 2018, 12:12 AM
  • Use "Show Hidden Places"
rizzitello edited the test plan for this revision. (Show Details)Nov 9 2018, 12:16 AM
ngraham accepted this revision as: VDG.Nov 9 2018, 12:18 AM

Lovely. Consider it VDG-approved! I'll let @elvisangelaccio handle the remainder of the code review.

rizzitello marked an inline comment as done.Nov 9 2018, 11:51 AM
rizzitello edited the summary of this revision. (Show Details)Nov 9 2018, 8:36 PM
elvisangelaccio requested changes to this revision.Nov 11 2018, 10:58 AM
elvisangelaccio added inline comments.
src/dolphinmainwindow.cpp
1338

I don't think this comment adds any value, code is already self-explanatory. I'd just remove it.

1339

Please add this as parent, otherwise it will leak (see D16817).

1345

Same, please remove it

1346

We don't need to capture this here.

src/panels/places/placespanel.h
60

The slotXXX prefix is usually used for private slots. We can just call it showHiddenEntries().

This revision now requires changes to proceed.Nov 11 2018, 10:58 AM
src/panels/places/placespanel.cpp
291

One more thing: the new action needs to be added only if we have hidden entries, like we do here. This means we need one more public method in PlacesPanel

rizzitello updated this revision to Diff 45280.Nov 11 2018, 1:18 PM
rizzitello marked 6 inline comments as done.
rizzitello edited the test plan for this revision. (Show Details)
  • Elvis' suggestions
ngraham requested changes to this revision.Nov 11 2018, 4:28 PM
ngraham added inline comments.
src/dolphinmainwindow.cpp
1351

We don't make menu items conditionally disappear. Please only disable it when it can't be used, don't hide it.

This revision now requires changes to proceed.Nov 11 2018, 4:28 PM
src/dolphinmainwindow.cpp
1351

Well, we already do for the action in the places panel context menu (same in the KIO one). Shouldn't we be consistent with that?

ngraham added inline comments.Nov 11 2018, 4:44 PM
src/dolphinmainwindow.cpp
1351

Ah yes, we should fix those as well. We always try to disable inapplicable items rather than hiding them. The HIG says this, but it's kind of hidden away in https://hig.kde.org/patterns/content/settings.html?highlight=inapplicable. I'll make that more obvious on the https://hig.kde.org/components/navigation/menubar.html and https://hig.kde.org/components/navigation/contextmenu.html pages, too.

veqz added a comment.Nov 11 2018, 5:01 PM

I'm fully behind Nate on this one.

I found the disappearing item in the context menu quite confusing myself when looking into this, even as I understood logically why it was probably not there. The combination of an on-again, off-again context menu, the Places items I removed _or_ hid, and the ability to also hide the Places part of the panel itself was really playing some tricks on me.

I would strongly prefer that the options are disabled, but still visible in the menus.

src/panels/places/placespanel.h
53

This signal is a bit confusing. I'd prefer using:

void hiddenCountChanged(int hiddenCount);

since it's how you are actually using it.

rizzitello updated this revision to Diff 45320.Nov 11 2018, 6:59 PM
  • disable instead of hide.
rizzitello edited the summary of this revision. (Show Details)Nov 11 2018, 7:06 PM
rizzitello edited the test plan for this revision. (Show Details)
rizzitello edited the summary of this revision. (Show Details)

ONE PROBLEM HERE: Can not get a count of hidden Items on start up. the count for hidden items is always 0. So the Menu entry always starts enabled. Any help here would be great.

Try to delay the menu initialization using a QTimer::singleShot(0, ...). The places model gets populated asynchronously.

rizzitello updated this revision to Diff 45323.Nov 11 2018, 7:48 PM
  • Fix issues, thanks for the idea nate
rizzitello edited the summary of this revision. (Show Details)Nov 11 2018, 7:49 PM

ONE PROBLEM HERE: Can not get a count of hidden Items on start up. the count for hidden items is always 0. So the Menu entry always starts enabled. Any help here would be great.

Try to delay the menu initialization using a QTimer::singleShot(0, ...). The places model gets populated asynchronously.

Nate had suggested using the QMenu::aboutToShow to enable and disable the item. Other then a crash if the panel was not created yet this worked perfectly from the start. I have added a check to see if our model if valid to avoid crashing while preserving the disable behavior.

This should be ready for a final test now.

rizzitello updated this revision to Diff 45324.Nov 11 2018, 8:12 PM
rizzitello edited the summary of this revision. (Show Details)
  • Clean commits
rizzitello retitled this revision from Add option to toggle all hidden panel item to view menu to Improve Ux for the places panel's hidden items.Nov 11 2018, 8:18 PM
rizzitello edited the summary of this revision. (Show Details)
rizzitello marked 4 inline comments as done.
rizzitello updated this revision to Diff 45328.Nov 11 2018, 9:04 PM
rizzitello edited the summary of this revision. (Show Details)
  • Use correct signing info
ngraham accepted this revision as: VDG, ngraham.Nov 12 2018, 4:13 AM

Thanks, this is great UI-wise now! Big thumbs up.

elvisangelaccio accepted this revision.Nov 17 2018, 10:49 AM
elvisangelaccio added inline comments.
src/panels/places/placespanel.cpp
558

Coding style: space before/after the parentheses

This revision is now accepted and ready to land.Nov 17 2018, 10:49 AM
  • Rebase
  • Adjust incorrectly styled line
rizzitello marked an inline comment as done.Nov 17 2018, 12:23 PM
rizzitello updated this revision to Diff 45662.Nov 17 2018, 1:37 PM
  • prep to land
This revision was automatically updated to reflect the committed changes.

This should have been pushed on master, Applications/18.12 is feature frozen since 15th November.
Given that we are only two days late, I think we can ask for an exception, but next time please be careful.