D17111: Do not sort twice when changing role and order at the same time

[Elvis Angelaccio]

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

INLINE COMMENTS

> kfileitemmodel.cpp:841
>  {
> +    qDebug() << "KFileItemModel::resortAllItems";
>      m_resortAllItemsTimer->stop();

Please remove, this will only add noise.

And you could accomplish the same goal by properly setting your `QT_MESSAGE_PATTERN` variable.

> kitemmodelbase.h:84-88
>      /**
>       * Sets the sort-role to \a role. The method KItemModelBase::onSortRoleChanged() will be
>       * called so that model-implementations can react on the sort-role change. Afterwards the
>       * signal sortRoleChanged() will be emitted.
>       */

Please update the apidox. It should say that the implementation should resort only if `resortItems` is true.

> kitemmodelbase.h:262-269
>      /**
>       * Is invoked if the sort role has been changed by KItemModelBase::setSortRole(). Allows
>       * to react on the changed sort role before the signal sortRoleChanged() will be emitted.
>       * The implementation must assure that the items are sorted by the role given by \a current.
>       * Usually the most efficient way is to emit a
>       * itemsRemoved() signal for all items, reorder the items internally and to emit a
>       * itemsInserted() signal afterwards.

Same here, the apidox needs to be updated.

> kitemlistheaderwidget.cpp:223
>              const QByteArray current = m_columns[m_pressedRoleIndex];
> -            m_model->setSortRole(current);
> +            const bool updateSortOrder = m_model->sortOrder() == Qt::DescendingOrder;
> +            m_model->setSortRole(current, !updateSortOrder);

How about calling the variable `resetSortOrder` ?

See 2854a69fcad54d394ebec504af4995dcb5e18ac4 <https://phabricator.kde.org/R318:2854a69fcad54d394ebec504af4995dcb5e18ac4>

REPOSITORY
  R318 Dolphin

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

To: thsurrel, #dolphin, elvisangelaccio
Cc: elvisangelaccio, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20181125/b161f665/attachment.htm>