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

[Thomas Surrel]

thsurrel created this revision.
thsurrel added a reviewer: Dolphin.
Herald added a project: Dolphin.
Herald added a subscriber: kfm-devel.
thsurrel requested review of this revision.

REVISION SUMMARY
When using the list header to change the role and order, if one
changes the order to descending and then changes role, dolphin
also changes the order back to ascending. This results in sorting
the list of files twice in a row. This patch removes the first
(useless) sort.

REPOSITORY
R318 Dolphin

BRANCH
arc_sort_optim (branched from master)

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

AFFECTED FILES
src/kitemviews/kfileitemmodel.cpp
src/kitemviews/kfileitemmodel.h
src/kitemviews/kitemmodelbase.cpp
src/kitemviews/kitemmodelbase.h
src/kitemviews/private/kitemlistheaderwidget.cpp

To: thsurrel, #dolphin
Cc: kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[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

[Thomas Surrel]

thsurrel updated this revision to Diff 46276.
thsurrel added a comment.


Fixes as per elvisangelaccio comments
Thanks for the review!

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17111?vs=46041&id=46276

BRANCH
arc_sort_optim (branched from master)

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

AFFECTED FILES
src/kitemviews/kfileitemmodel.cpp
src/kitemviews/kfileitemmodel.h
src/kitemviews/kitemmodelbase.cpp
src/kitemviews/kitemmodelbase.h
src/kitemviews/private/kitemlistheaderwidget.cpp

To: thsurrel, #dolphin, elvisangelaccio
Cc: elvisangelaccio, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Thomas Surrel]

thsurrel marked 4 inline comments as done.

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

[Thomas Surrel]

thsurrel updated this revision to Diff 46287.
thsurrel added a comment.


Fix docx

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17111?vs=46276&id=46287

BRANCH
arc_sort_optim (branched from master)

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

AFFECTED FILES
src/kitemviews/kfileitemmodel.cpp
src/kitemviews/kfileitemmodel.h
src/kitemviews/kitemmodelbase.cpp
src/kitemviews/kitemmodelbase.h
src/kitemviews/private/kitemlistheaderwidget.cpp

To: thsurrel, #dolphin, elvisangelaccio
Cc: elvisangelaccio, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Elvis Angelaccio]

elvisangelaccio accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
R318 Dolphin

BRANCH
arc_sort_optim (branched from master)

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

To: thsurrel, #dolphin, elvisangelaccio
Cc: elvisangelaccio, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov

[Thomas Surrel]

This revision was automatically updated to reflect the committed changes.
Closed by commit R318:b714604a6795: Do not sort twice when changing role and order at the same time (authored by thsurrel).

REPOSITORY
R318 Dolphin

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D17111?vs=46287&id=46648

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

AFFECTED FILES
src/kitemviews/kfileitemmodel.cpp
src/kitemviews/kfileitemmodel.h
src/kitemviews/kitemmodelbase.cpp
src/kitemviews/kitemmodelbase.h
src/kitemviews/private/kitemlistheaderwidget.cpp

To: thsurrel, #dolphin, elvisangelaccio
Cc: elvisangelaccio, kfm-devel, alexde, sourabhboss, feverfew, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov