D8367: Hidding place groups implementation in KFilePlacesModel

[Milian Wolff]

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

INLINE COMMENTS

> kfileplacesmodel.cpp:70
> +    {
> +        { KFilePlacesModel::PlacesType, QStringLiteral("GroupState-Places-IsHidden") },
> +        { KFilePlacesModel::RecentlySavedType, QStringLiteral("GroupState-RecentlySaved-IsHidden") },

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();
  }
  }

> kfileplacesmodel.cpp:127
>  
> +    const bool areGroupsHiddenByDefault = false;
> +

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

> kfileplacesmodel.cpp:164
>  
> +        root.setMetaDataItem(s_groupTypeToStateName->value(PlacesType), (areGroupsHiddenByDefault ? QStringLiteral("true") : QStringLiteral("false")));
> +        root.setMetaDataItem(s_groupTypeToStateName->value(DevicesType), (areGroupsHiddenByDefault ? QStringLiteral("true") : QStringLiteral("false")));

introduce a helper lambda for this to share duplicate code

> kfileplacesmodel.cpp:532
>  
> +static const QHash<KFilePlacesModel::GroupType, bool> groupStateHidden(const KBookmark rootBookmark)
> +{

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

> kfileplacesmodel.cpp:596
>      }
> +    // we make sure an item within a hidden group remains hidden too
> +    const QHash<GroupType, bool> groupStates = groupStateHidden(root);

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

> kfileplacesmodel.cpp:878
>  
> -    bookmark.setMetaDataItem(QStringLiteral("IsHidden"), (hidden ? QStringLiteral("true") : QStringLiteral("false")));
> +    const QHash<GroupType, bool> gpStateHidden = groupStateHidden(d->bookmarkManager->root());
> +    const bool hidingChildOnShownParent = hidden && !gpStateHidden.value(item->groupType());

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;

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

To: mlaurent, renatoo, ngraham, franckarrecot, ervin, mwolff
Cc: mwolff, ngraham, mlaurent, #frameworks
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20171115/2d9d0d6a/attachment-0001.html>