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
