Hidding place groups implementation in KFilePlacesModel
ClosedPublic

Authored by franckarrecot on Oct 19 2017, 1:04 PM.

Details

Summary

This give an implementation to the places model to hide an entire group
of places. The model is now holding group states as metadata properties
within the bookmarkmanager root object.

Depends on D8243
Depends on D8332
Depends on D8348
Depends on D8366

Depends on D8948
Depends on D8947
Depends on D8946
Depends on D8945
Depends on D8944
Depends on D8943
BUG: 300247

Test Plan

Unit tested

(before remove "recently saved")

(after)

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
There are a very large number of changes, so older changes are hidden. Show Older Changes
mlaurent updated this revision to Diff 21605.Oct 31 2017, 8:44 AM

Remove duplicate code

mlaurent edited the test plan for this revision. (Show Details)Oct 31 2017, 8:56 AM
mlaurent edited the test plan for this revision. (Show Details)
ervin requested changes to this revision.Oct 31 2017, 11:45 AM
ervin added inline comments.
autotests/kfileplacesmodeltest.cpp
504

All those unrelated connect style changes would have been better in a separate commit.

src/filewidgets/kfileplacesitem.cpp
104

Same thing the enum move could have been a separate commit.

122–123

A fallthrough would be better here.

202

Very welcome refactoring... in a separate commit

src/filewidgets/kfileplacesmodel.cpp
62–74

Just make it a const global static then... No need to recreate this hash several times.

169

You always use the hash with .key() which is very inefficient, so why not swap key and value types and rename it groupTypeToStateName?

884

Typo: hiding

src/filewidgets/kfileplacesmodel.h
55

Better have the curly brace on the same line than the enum name to be consistent with the other enum just on top.

Also the enum itself needs to carry over the changes which have been applied to previous commits.

This revision now requires changes to proceed.Oct 31 2017, 11:45 AM
mlaurent marked 6 inline comments as done.Oct 31 2017, 1:41 PM
mlaurent added inline comments.
src/filewidgets/kfileplacesitem.cpp
202

This one is for avoiding duplicate code as we create in this patch isHidden method.

mlaurent updated this revision to Diff 21643.Oct 31 2017, 1:43 PM

updated according to comments

ervin requested changes to this revision.Oct 31 2017, 2:07 PM
ervin added inline comments.
src/filewidgets/kfileplacesitem.cpp
202

That was kind of my point, isHidden could have been introduced on its own with the changes needed to remove the duplication in the same commit.

src/filewidgets/kfileplacesmodel.cpp
62–74

Why the intermediate struct? Could be directly the hash as a global static, should make it easier to use you wouldn't need the function below and the extra indirection.

src/filewidgets/kfileplacesmodel.h
55

You missed a bit, it looks like it wasn't applied on top of the latest iteration from the other patches the GroupType enum gained some values for its entries.

This revision now requires changes to proceed.Oct 31 2017, 2:07 PM
mlaurent marked an inline comment as done.Oct 31 2017, 2:40 PM
mlaurent added inline comments.
src/filewidgets/kfileplacesmodel.h
55

Ah ! Ok I will rebase all my patch against last version.
I will update my patch and I will rebase after that.

mlaurent updated this revision to Diff 21645.Oct 31 2017, 2:41 PM

Fixing create static variable

ngraham added a comment.EditedOct 31 2017, 4:00 PM

Nice! If possible, I'd like a more user-friendly way of hiding and showing categories, though. The context menu is not very discoverable, and if a category is hidden, there's no indication that there's even anything to show. Here's how macOS finder handles this, for comparison (it's an animated gif; click to see it):

I Think we should keep the headers visible so users know that there are hidden sections.

For a show/hide UI, here are some ideas:

  1. Add disclosure triangles to the right of the headers that, when clicked, hide the contents
  2. On hover, the headers could gain an outline like a button and the text would change to "Show/hide [category]"; when clicked, it would hide the contents

Nice! If possible, I'd like a more user-friendly way of hiding and showing categories, though. The context menu is not very discoverable, and if a category is hidden, there's no indication that there's even anything to show. Here's how macOS finder handles this, for comparison (it's an animated gif; click to see it):

I Think we should keep the headers visible so users know that there are hidden sections.

For a show/hide UI, here are some ideas:

  1. Add disclosure triangles to the right of the headers that, when clicked, hide the contents
  2. On hover, the headers could gain an outline like a button and the text would change to "Show/hide [category]"; when clicked, it would hide the contents

This was our first option but due the impossibility of break the ABI, we will need to implement the hole functionality using a QListView which will require a lot of code change and some workarounds.
In my opinion the ideal would be derive the KPlacesView from another view (QTreeView, KCategoryView or something very specific like dolphin does), but as mentioned before we do not want to break the ABI.
In any case this can be done after.

OK, great! Glad you're thinking about it and I'm happy we came to the same conclusion. :) Later is fine.

This is awesome work, BTW.

mlaurent updated this revision to Diff 21752.Nov 2 2017, 10:25 AM

Rebase against master + other top patchs :)

ervin requested changes to this revision.Nov 2 2017, 10:42 AM

Couple more changes needed.

src/filewidgets/kfileplacesmodel.cpp
79–83

This can go away now

src/filewidgets/kfileplacesmodel.h
71–72

Just realized now that it'd better be named "isGroupHidden". Otherwise it'll get confusing (especially since one of the types is "PlacesType"!)

89

Likewise: setGroupHidden.

This revision now requires changes to proceed.Nov 2 2017, 10:42 AM
mlaurent updated this revision to Diff 21755.Nov 2 2017, 11:02 AM
mlaurent marked 2 inline comments as done.

Remove commented code. Rename methods

mwolff requested changes to this revision.Nov 15 2017, 10:56 AM
mwolff added a subscriber: mwolff.
mwolff added inline comments.
src/filewidgets/kfileplacesmodel.cpp
70

instead of a global static and hash, why not simply do the mapping in a function with a simple switch statement?

namespace {
QString stateNameForGroupType(KFilePlacesModel::GroupType type)
{
    switch (type) {
    case KFilePlacesModel::PlacesType:
        return QStringLiteral("GroupState-Places-IsHidden");
    ...
    }
    Q_UNREACHABLE();
}
}
127

why not hardcode this below? we will never hide them by default, will we?

164

introduce a helper lambda for this to share duplicate code

532

misnomer? I'd rename this function to hiddenGroups

also, take the rootBookmark by const &?

actually, remove this whole function and instead introduce a bool isGroupHidden(KFilePlacesModel::GroupType) helper function, see also below

596

why is this needed? i.e. doesn't this mean that once I unhide the group, all child items are still hidden? or what happens when I hide an item, then its group, and then unhide its group - will the item remain hidden or not?

is this b/c the file items don't build a tree with their groups, but rather just a list? a comment would be useful

878

this is overkill, what you want is only a

const bool isGroupHidden = ::isGrouphidden(item->groupType());
const bool hidingChildOnShownParent = hidden && !isGroupHidden;
const bool showingChildOnShownParent = !hidden && !isGroupHidden;
This revision now requires changes to proceed.Nov 15 2017, 10:56 AM
franckarrecot commandeered this revision.Nov 22 2017, 11:32 AM
franckarrecot edited reviewers, added: mlaurent; removed: franckarrecot.

taking ownership back to fix comments, thanks Laurent for taking care of it :-)

src/filewidgets/kfileplacesmodel.cpp
257

it is currently not needed here since each change an object metadata ( eg : item isHidden metadata ) would trigger a loadBookmarkList().
And in this code we make sure to change the hidden state of item if it conflict with their parent.

However I think it could help future reader to get the intention if I added this.

franckarrecot edited the test plan for this revision. (Show Details)

update

precision on useless comment

src/filewidgets/kfileplacesmodel.cpp
257

forget that part, old comment stored I don't know how from old revision...

ervin requested changes to this revision.Nov 24 2017, 11:16 AM

A small nitpick otherwise lgtm.

src/filewidgets/kfileplacesmodel.cpp
870

Why not naming the variable isGroupHidden and use the :: prefix on the function call like milian proposed?

This revision now requires changes to proceed.Nov 24 2017, 11:16 AM
franckarrecot added inline comments.Nov 24 2017, 2:24 PM
src/filewidgets/kfileplacesmodel.cpp
870

Because it wouldn't compile without the whole KFIlePlacesModel:: prefix, so I end up going for variable renaming, seemed cleaner

franckarrecot marked 5 inline comments as done.Nov 24 2017, 2:25 PM

rebased over in review renato 's patches

ervin added inline comments.Nov 27 2017, 7:46 AM
src/filewidgets/kfileplacesmodel.cpp
870

Let's go for naming it "groupHidden" then. It's a weird shortening otherwise.

fix variable name

update - should be landable now

Can you mark the Not Done comments as Done if they're done now?

franckarrecot marked 15 inline comments as done.Nov 27 2017, 3:29 PM
franckarrecot marked 2 inline comments as done.Nov 27 2017, 4:03 PM
ervin accepted this revision.Nov 28 2017, 9:34 AM

@mwolff, is this good now?

rebase to master

mlaurent accepted this revision.Dec 6 2017, 8:40 AM
mwolff accepted this revision.Dec 6 2017, 12:25 PM

lgtm

This revision is now accepted and ready to land.Dec 6 2017, 12:25 PM
renatoo accepted this revision.Dec 6 2017, 1:19 PM

Looks good and works as expected

This revision was automatically updated to reflect the committed changes.