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
