D8367: Hidding place groups implementation in KFilePlacesModel

[Kevin Ottens]

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

INLINE COMMENTS

> kfileplacesmodeltest.cpp:504
>      QList<QVariant> args;
> -    QSignalSpy spy_inserted(m_places, SIGNAL(rowsInserted(QModelIndex,int,int)));
> -    QSignalSpy spy_removed(m_places, SIGNAL(rowsRemoved(QModelIndex,int,int)));
> -    QSignalSpy spy_changed(m_places, SIGNAL(dataChanged(QModelIndex,QModelIndex)));
> +    QSignalSpy spy_inserted(m_places, &KFilePlacesModel::rowsInserted);
> +    QSignalSpy spy_removed(m_places, &KFilePlacesModel::rowsRemoved);

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

> kfileplacesitem.cpp:104
>  
> -    const GroupType type = groupType();
> +    const KFilePlacesModel::GroupType type = groupType();
>      switch (type) {

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

> kfileplacesitem.cpp:122-123
> +    case KFilePlacesModel::UnknownType:
> +        Q_ASSERT(false);
> +        break;
>      default:

A fallthrough would be better here.

> kfileplacesitem.cpp:202
>      case Qt::BackgroundRole:
> -        if (b.metaDataItem(QStringLiteral("IsHidden")) == QLatin1String("true")) {
> +        if (isHidden()) {
>              return QColor(Qt::lightGray);

Very welcome refactoring... in a separate commit

> kfileplacesmodel.cpp:62-74
> +static const QHash<QString, KFilePlacesModel::GroupType> groupStateNameToType()
> +{
> +    static const QHash<QString, KFilePlacesModel::GroupType> gpStateNameToType =
> +    {
> +        { QStringLiteral("GroupState-Places-IsHidden"), KFilePlacesModel::PlacesType },
> +        { QStringLiteral("GroupState-RecentlySaved-IsHidden"), KFilePlacesModel::RecentlySavedType },
> +        { QStringLiteral("GroupState-SearchFor-IsHidden"), KFilePlacesModel::SearchForType },

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

> kfileplacesmodel.cpp:160
>  
> +        root.setMetaDataItem(groupStateNameToType().key(PlacesType), (areGroupsHiddenByDefault ? QStringLiteral("true") : QStringLiteral("false")));
> +        root.setMetaDataItem(groupStateNameToType().key(DevicesType), (areGroupsHiddenByDefault ? QStringLiteral("true") : QStringLiteral("false")));

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

> kfileplacesmodel.cpp:875
> +    const QHash<GroupType, bool> gpStateHidden = groupStateHidden(d->bookmarkManager->root());
> +    const bool hiddingChildOnShownParent = hidden && !gpStateHidden.value(item->groupType());
> +    const bool showingChildOnShownParent = !hidden && !gpStateHidden.value(item->groupType());

Typo: hiding

> kfileplacesmodel.h:55
> +    enum GroupType
> +    {
> +        PlacesType,

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.

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

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