User can now hide an entire places group from KFilePlacesView
ClosedPublic

Authored by franckarrecot on Oct 24 2017, 3:01 PM.

Details

Summary

Through the ui the user can now right click on a place group
and request to hide it.

Note: It is not the collapsing solution, which would need a way bigger
effort on the model to be achieve but an intermediary solution that could
be useful for users.

Depends on D8367

Depends on D8948
Depends on D8947
Depends on D8946
Depends on D8945
Depends on D8944
Depends on D8943

BUG: 300247

Test Plan

Manual testing with the ui.

(before hidden section)

(menu for hiding section)

(after hidden section)

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
franckarrecot created this revision.Oct 24 2017, 3:01 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptOct 24 2017, 3:01 PM
Restricted Application added a subscriber: Frameworks. · View Herald Transcript
mlaurent added inline comments.Oct 25 2017, 9:15 AM
src/filewidgets/kfileplacesview.cpp
297

m_disappearingItems.reserve(m_disappearingItems.count() + indexesGroup.count()) ?

431

index.model() can be null ?
otherwise it's not necessary to check "if (placesModel" as you use static_cast<...>

franckarrecot retitled this revision from WIP: User can now hide an entire places group to User can now hide an entire places group.Oct 25 2017, 9:53 AM
franckarrecot retitled this revision from User can now hide an entire places group to User can now hide an entire places group from KFilePlacesView.

updated according to comments

I will add pics soonish

mlaurent commandeered this revision.Oct 31 2017, 9:05 AM
mlaurent edited reviewers, added: franckarrecot; removed: mlaurent.
mlaurent edited the test plan for this revision. (Show Details)Oct 31 2017, 9:10 AM
mlaurent edited the test plan for this revision. (Show Details)
mlaurent added a reviewer: ervin.
mlaurent updated this revision to Diff 21612.Oct 31 2017, 9:23 AM

Minor optimization.

ervin requested changes to this revision.Oct 31 2017, 12:21 PM

At least the unit test would be welcome, I let you decide on the other comment.

src/filewidgets/kfileplacesmodel.cpp
329 ↗(On Diff #21612)

Would be nice to unit test that guy

src/filewidgets/kfileplacesview.cpp
863–866 ↗(On Diff #21612)

This duplicates code from the next if branch below... I wonder if before or after that commit we shouldn't try to refactor that and remove some of the code duplication around the delegate animation use?

This revision now requires changes to proceed.Oct 31 2017, 12:21 PM
mlaurent updated this revision to Diff 21644.Oct 31 2017, 2:18 PM

Autotest GroupIndexes

ervin accepted this revision.Nov 2 2017, 10:46 AM
This revision is now accepted and ready to land.Nov 2 2017, 10:46 AM
mwolff requested changes to this revision.Nov 15 2017, 11:06 AM
mwolff added a subscriber: mwolff.

please use arc, or add more context to your patches in the future

src/filewidgets/kfileplacesview.cpp
298

the reserve + loop should be the same as doing

m_disappearingItems += indexesGroup;
431

i18n puzzle, do something like this:

const QString groupLabel = index.data(KFilePlacesModel::GroupRole).toString();
const bool groupHidden = placesModel->isPlaceGroupHidden(index);
const QString category = groupHidden ? i18n("%1 (hidden)", groupLabel) : groupLabel;
861

shouldn't we disable the hideSection when d->showAll is in effect? makes no sense to hide something when it has no visual effect, no?

This revision now requires changes to proceed.Nov 15 2017, 11:06 AM
franckarrecot added inline comments.Nov 22 2017, 1:13 PM
src/filewidgets/kfileplacesview.cpp
298

I'm filling a list of persistant indexes with regular indexes that is why I use the loop, if you have any other way comment it to me please :)

861

There is a visual effect : the label holds "hidden" or not depending of the state. Plus to be consistent with the place entries we should stick with their behavior I think.

franckarrecot commandeered this revision.Nov 22 2017, 1:16 PM
franckarrecot edited reviewers, added: mlaurent; removed: franckarrecot.
ervin requested changes to this revision.Nov 24 2017, 11:30 AM
ervin added inline comments.
src/filewidgets/kfileplacesview.cpp
298

That smells std::transform instead of the hand crafted loop at least.

863–866 ↗(On Diff #21612)

I still stand by that comment, I'd welcome a patch on top of that one to reduce that duplication please.

This revision now requires changes to proceed.Nov 24 2017, 11:30 AM
franckarrecot marked an inline comment as done.

rebased over in review renato 's patches

src/filewidgets/kfileplacesview.cpp
298

good idea

863–866 ↗(On Diff #21612)

Oki I add that to my list :-)

franckarrecot marked 3 inline comments as done.Nov 24 2017, 2:32 PM
ervin added inline comments.Nov 27 2017, 7:47 AM
src/filewidgets/kfileplacesview.cpp
863–866 ↗(On Diff #21612)

Cool, I'll wait for that extra review to appear before accepting that one. So that we don't forget it. :-)

now related to the refactoring commit coming after

franckarrecot marked an inline comment as done.Nov 27 2017, 11:35 AM
franckarrecot added inline comments.
src/filewidgets/kfileplacesview.cpp
863–866 ↗(On Diff #21612)

Was that the thing you had in mind ?

https://phabricator.kde.org/D9015

franckarrecot marked an inline comment as done.

update, have to wait to be pushed, since it contains i18n translation, next week:
in the first two weeks after the first saturday of the month

ervin accepted this revision.Nov 28 2017, 9:35 AM

rebase to master

mlaurent accepted this revision.Dec 6 2017, 8:39 AM
mlaurent added inline comments.
src/filewidgets/kfileplacesview.cpp
863–866 ↗(On Diff #21612)

Ok for removing duplicate code after this patch. Too hard to add patch before it.

mwolff accepted this revision.Dec 6 2017, 12:26 PM

one minor nit, otherwise lgtm

src/filewidgets/kfileplacesmodel.cpp
431 ↗(On Diff #21612)

only call rowCount once:

for (int row = 0, c = rowCount(); row < c; ++row) {
This revision is now accepted and ready to land.Dec 6 2017, 12:26 PM
renatoo accepted this revision.Dec 6 2017, 1:20 PM

Looks good and work as expected

src/filewidgets/kfileplacesview.cpp
766

I think that you can replace

if (!clickOverHeader && index.isValid()) {

with

else if (index.isValid()) {
....
}

corrected and rebased

This revision was automatically updated to reflect the committed changes.