D16767: Add option to toggle all hidden panel item to view menu

[Chris Rizzitello]

rizzitello created this revision.
Herald added a project: Dolphin.
Herald added a subscriber: kfm-devel.
rizzitello requested review of this revision.

REVISION SUMMARY
Solve https://bugs.kde.org/show_bug.cgi?id=400860
by exporting the action from the places widget and adding it to the view -> panels menu as well.

REPOSITORY
R318 Dolphin

BRANCH
showAllPlacesMenu

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

AFFECTED FILES
src/dolphinmainwindow.cpp
src/panels/places/placespanel.cpp
src/panels/places/placespanel.h

To: rizzitello
Cc: kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello added reviewers: Dolphin, VDG.

REPOSITORY
R318 Dolphin

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

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

[Nathaniel Graham]

ngraham added a comment.


Cool, thanks! Will test later. In the meantime, please hange `Solve https://bugs.kde.org/show_bug.cgi?id=400860` to:

BUG: 400860
FIXED-IN: 18.12.0

(See https://community.kde.org/Infrastructure/Phabricator#Formatting_your_patch)

A screenshot in the Test Plan section would be nice too. :)

REPOSITORY
R318 Dolphin

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

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

[Chris Rizzitello]

rizzitello edited the summary of this revision.
rizzitello edited the test plan for this revision.

REPOSITORY
R318 Dolphin

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

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

[Chris Rizzitello]

rizzitello updated this revision to Diff 45136.
rizzitello added a comment.


- Remove unneeded function

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16767?vs=45135&id=45136

BRANCH
showAllPlacesMenu

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

AFFECTED FILES
src/dolphinmainwindow.cpp
src/panels/places/placespanel.cpp
src/panels/places/placespanel.h

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

[Chris Rizzitello]

rizzitello updated this revision to Diff 45139.
rizzitello added a comment.


- Match Hidden files style

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16767?vs=45136&id=45139

BRANCH
showAllPlacesMenu

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

AFFECTED FILES
src/dolphinmainwindow.cpp
src/panels/places/placespanel.cpp
src/panels/places/placespanel.h

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

[Chris Rizzitello]

rizzitello edited the summary of this revision.
rizzitello edited the test plan for this revision.

REPOSITORY
R318 Dolphin

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

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

[Chris Rizzitello]

rizzitello edited the summary of this revision.

REPOSITORY
R318 Dolphin

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

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

[David C]

davidc added a comment.


The turnaround time on this was amazing. This will make managing Places entries so much easier! Great work!

REPOSITORY
R318 Dolphin

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

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

[Chris Rizzitello]

rizzitello updated this revision to Diff 45140.
rizzitello added a comment.


- remove more unneeded code

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16767?vs=45139&id=45140

BRANCH
showAllPlacesMenu

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

AFFECTED FILES
src/dolphinmainwindow.cpp
src/panels/places/placespanel.cpp
src/panels/places/placespanel.h

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

[Tore Havn]

veqz added a comment.


Awesome work! :)

REPOSITORY
R318 Dolphin

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

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

[Elvis Angelaccio]

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


Makes sense, thanks

INLINE COMMENTS

placespanel.cpp:67
{
+ m_showAllAction->setCheckable(true);

Why drop "Show" from the label though?

placespanel.h:45
void proceedWithTearDown();
+ QAction* actionShowAllPlaces();

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

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello updated this revision to Diff 45144.
rizzitello marked 2 inline comments as done.
rizzitello added a comment.


- Elvis' Suggestion

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16767?vs=45140&id=45144

BRANCH
showAllPlacesMenu

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

AFFECTED FILES
src/dolphinmainwindow.cpp
src/panels/places/placespanel.cpp
src/panels/places/placespanel.h

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello added inline comments.

INLINE COMMENTS

elvisangelaccio wrote in placespanel.cpp:67
Why drop "Show" from the label though?

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

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

elvisangelaccio wrote in placespanel.cpp:67
Why drop "Show" from the label though?

What Elvis meant was `Hidden Places` → `Show Hidden Places`.

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

rizzitello wrote in placespanel.cpp:67
This puts its text in line with the hidden files entry that is just "Hidden Files".

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

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello added inline comments.

INLINE COMMENTS

cfeck wrote in placespanel.cpp:67
Maybe. Places Panel's context menu says "Show All Entries", not "All Entries", though.

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

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello edited the summary of this revision.
rizzitello edited the test plan for this revision.

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

rizzitello wrote in placespanel.cpp:67
It no longer does with this diff. They actions match (as they were one before my last update)

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.

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello updated this revision to Diff 45145.
rizzitello marked 4 inline comments as done.
rizzitello added a comment.


clean up older diff code

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16767?vs=45144&id=45145

BRANCH
showAllPlacesMenu

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

AFFECTED FILES
src/dolphinmainwindow.cpp
src/panels/places/placespanel.cpp
src/panels/places/placespanel.h

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello edited the test plan for this revision.

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Nathaniel Graham]

ngraham added a comment.


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

Very impressive speed on this patch. :)

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello updated this revision to Diff 45146.
rizzitello added a comment.


- Use "Show Hidden Places"

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16767?vs=45145&id=45146

BRANCH
showAllPlacesMenu

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

AFFECTED FILES
src/dolphinmainwindow.cpp
src/panels/places/placespanel.cpp
src/panels/places/placespanel.h

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello edited the test plan for this revision.

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Nathaniel Graham]

ngraham accepted this revision as: VDG.
ngraham added a comment.


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

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Nathaniel Graham]

ngraham added a task: T8349: Improve Places panel usability and presentation.

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello marked an inline comment as done.

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello edited the summary of this revision.

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

dolphinmainwindow.cpp:1338
+ //connect the action to show Hidden places.

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

dolphinmainwindow.cpp:1339
+ //connect the action to show Hidden places.
+ actionShowAllPlaces->setCheckable(true);

Please add `this` as parent, otherwise it will leak (see D16817 <https://phabricator.kde.org/D16817>).

dolphinmainwindow.cpp:1345
+ });
+ //connect the panel event to the action
+ connect(m_placesPanel, &PlacesPanel::showHiddenEntriesChanged, this, [actionShowAllPlaces, this] (bool checked){

Same, please remove it

dolphinmainwindow.cpp:1346
+ //connect the panel event to the action
+ connect(m_placesPanel, &PlacesPanel::showHiddenEntriesChanged, this, [actionShowAllPlaces, this] (bool checked){
+ actionShowAllPlaces->setChecked(checked);

We don't need to capture `this` here.

placespanel.h:60
void readSettings() override;
+ void slotShowHiddenEntries(bool shown);

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

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

placespanel.cpp:291
QAction* showAllAction = nullptr;
if (m_model->hiddenCount() > 0) {

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`

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello updated this revision to Diff 45280.
rizzitello marked 6 inline comments as done.
rizzitello edited the test plan for this revision.
rizzitello added a comment.


- Elvis' suggestions

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16767?vs=45146&id=45280

BRANCH
showAllPlacesMenu

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

AFFECTED FILES
src/dolphinmainwindow.cpp
src/panels/places/placespanel.cpp
src/panels/places/placespanel.h

To: rizzitello, #dolphin, #vdg, elvisangelaccio
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Nathaniel Graham]

ngraham requested changes to this revision.
ngraham added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

dolphinmainwindow.cpp:1351
+ connect(m_placesPanel, &PlacesPanel::hiddenListChanged, this, [actionShowAllPlaces](bool show){
+ actionShowAllPlaces->setVisible(show);
+ });

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

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

ngraham wrote in 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.

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?

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Nathaniel Graham]

ngraham added inline comments.

INLINE COMMENTS

elvisangelaccio wrote in 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?

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.

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Tore Havn]

veqz added a comment.


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.

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

placespanel.h:53
+ void showHiddenEntriesChanged(bool shown);
+ void hiddenListChanged(bool empty);

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

void hiddenCountChanged(int hiddenCount);

since it's how you are actually using it.

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello updated this revision to Diff 45320.
rizzitello added a comment.


- disable instead of hide.

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16767?vs=45280&id=45320

BRANCH
showAllPlacesMenu

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

AFFECTED FILES
src/dolphinmainwindow.cpp
src/panels/places/placespanel.cpp
src/panels/places/placespanel.h

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello edited the summary of this revision.
rizzitello edited the test plan for this revision.

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello edited the summary of this revision.

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Elvis Angelaccio]

elvisangelaccio added a comment.

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.

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello updated this revision to Diff 45323.
rizzitello added a comment.


- Fix issues, thanks for the idea nate

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16767?vs=45320&id=45323

BRANCH
showAllPlacesMenu

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

AFFECTED FILES
src/dolphinmainwindow.cpp
src/panels/places/placespanel.cpp
src/panels/places/placespanel.h

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello edited the summary of this revision.

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello added a comment.

Post by Elvis Angelaccio

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.

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello updated this revision to Diff 45324.
rizzitello edited the summary of this revision.
rizzitello added a comment.


- Clean commits

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16767?vs=45323&id=45324

BRANCH
showHiddenPlaces

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

AFFECTED FILES
src/dolphinmainwindow.cpp
src/panels/places/placespanel.cpp
src/panels/places/placespanel.h

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

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".
rizzitello edited the summary of this revision.

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello marked 4 inline comments as done.

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello updated this revision to Diff 45328.
rizzitello edited the summary of this revision.
rizzitello added a comment.


- Use correct signing info

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16767?vs=45324&id=45328

BRANCH
showHiddenPlaces

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

AFFECTED FILES
src/dolphinmainwindow.cpp
src/panels/places/placespanel.cpp
src/panels/places/placespanel.h

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello updated this revision to Diff 45329.
rizzitello added a comment.


- rebase

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16767?vs=45328&id=45329

BRANCH
showHiddenPlaces

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

AFFECTED FILES
src/dolphinmainwindow.cpp
src/panels/places/placespanel.cpp
src/panels/places/placespanel.h

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Nathaniel Graham]

ngraham accepted this revision as: VDG, ngraham.
ngraham added a comment.


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

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Elvis Angelaccio]

elvisangelaccio accepted this revision.
elvisangelaccio added inline comments.
This revision is now accepted and ready to land.

INLINE COMMENTS

placespanel.cpp:558
+{
+ if(!m_model){
+ return 0;

Coding style: space before/after the parentheses

REPOSITORY
R318 Dolphin

BRANCH
showHiddenPlaces

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello updated this revision to Diff 45650.
rizzitello added a comment.


- Rebase
- Adjust incorrectly styled line

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16767?vs=45329&id=45650

BRANCH
showHiddenPlaces

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

AFFECTED FILES
src/dolphinmainwindow.cpp
src/panels/places/placespanel.cpp
src/panels/places/placespanel.h

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello marked an inline comment as done.

REPOSITORY
R318 Dolphin

BRANCH
showHiddenPlaces

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

rizzitello updated this revision to Diff 45662.
rizzitello added a comment.


- prep to land

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16767?vs=45650&id=45662

BRANCH
showHiddenPlaces

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

AFFECTED FILES
src/dolphinmainwindow.cpp
src/panels/places/placespanel.cpp
src/panels/places/placespanel.h

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Chris Rizzitello]

This revision was automatically updated to reflect the committed changes.
Closed by commit R318:c900f7d255aa: Improve Ux for the places panel&#039;s hidden items (authored by rizzitello).

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16767?vs=45662&id=45663

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

AFFECTED FILES
src/dolphinmainwindow.cpp
src/panels/places/placespanel.cpp
src/panels/places/placespanel.h

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Elvis Angelaccio]

elvisangelaccio added a comment.


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.

REPOSITORY
R318 Dolphin

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

To: rizzitello, #dolphin, #vdg, elvisangelaccio, ngraham
Cc: cfeck, elvisangelaccio, veqz, davidc, ngraham, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov