D8243: Implement support for categories on KfilesPlacesView

[Kevin Ottens]

ervin requested changes to this revision.
ervin added a comment.
This revision now requires changes to proceed.


  Note that the summary part of the commit is now wrong (it still refers to KCategorizedView).
  
  Also, KFilePlacesModelTest needs to be adjusted to take into account the new behavior.

INLINE COMMENTS

> kfileplacesitem.cpp:179
> +
> +        if (protocol == QLatin1String("bluetooth") || protocol == QLatin1String("obexftp") || protocol == QLatin1String("kdeconnect")) {
> +            return DevicesType;

Long line, would be nice to split it.

> kfileplacesitem_p.h:106
>      QStringList m_emblems;
> +    QString m_group;
>  };

Rename to m_groupName ?

> kfileplacesmodel.cpp:474
> +    // return a sorted list based on groups
> +    qStableSort(items.begin(), items.end(), [](KFilePlacesItem *itemA, KFilePlacesItem *itemB) {
> +       return (itemA->groupType() < itemB->groupType());

Might be worth breaking the line after "items.end(),"

> kfileplacesview.cpp:62
>  public:
> -    KFilePlacesViewDelegate(KFilePlacesView *parent);
> +    KFilePlacesViewDelegate(int sectionRole, KFilePlacesView *parent);
>      virtual ~KFilePlacesViewDelegate();

I don't see the point of that ctor change. KFilePlacesView(Delegate) is already very much coupled to KFilePlacesModel anyway, so could be hardcoded to KFilePlacesModel::GroupRole.

> kfileplacesview.cpp:104
> +
> +    int m_sectionRole;
> +

Likewise, can go away in my opinion.

> kfileplacesview.cpp:342-356
> +    if (index.row() == 0) {
> +        return true;
> +    }
> +
> +    const QAbstractItemModel *model = index.model();
> +    QVariant section = index.data(m_sectionRole);
> +    QModelIndex prevIndex = model->index(index.row() - 1, index.column(), index.parent());

I'd consider refactoring this by splitting it in several helper functions:

- one which returns the groupName from an index (and so an empty string if the passed index is invalid);
- one which returns the previous visible index starting from an index (pretty much encapsulating the loop you got);

The whole block would then turn into something like:
const auto groupName = groupNameFromIndex(index);
const auto previousGroupName = groupNameFromIndex(previousVisibleIndex(index));
return groupName != previousGroupName;

More elegant and readable in my opinion, also make sure the whole method is at the same abstraction level.

> kfileplacesview.cpp:1172
> +
> +    for(int i=0; i < q->model()->rowCount(); i++) {
> +        if (!q->isRowHidden(i)) {

Spaces before and after =

REPOSITORY
  R241 KIO

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

To: renatoo, #frameworks, dfaure, ervin
Cc: ervin, anthonyfieroni, cfeck, #frameworks
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20171012/8f6aff9c/attachment-0001.html>