Review Request 111721: Ensure that the sorting is correct after renaming if the items are not sorted by name, but the name is used as a fallback

[Frank Reininghaus]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/111721/
-----------------------------------------------------------

(Updated Aug. 6, 2013, 9:06 p.m.)


Review request for Dolphin.


Changes
-------

I had already started to write some code that updates the groups in setData() if the items don't have to be resorted. But this is less trivial then it seems: Re-calculating the groups can be expensive because the code that does that goes through all items. This might become a major bottleneck when many items are updated one by one by KFileItemModelRolesUpdater. So my idea was to first check if the changed item is either the first or the last one in a group (if that is not the case, the groups don't change) and then start a timer that invokes a new function updateGroups().

But then I thought that all this effort isn't really needed for what I'm trying to achieve here, and it's doubtful if not doing it that way will ever make a noticeable difference. All I'm trying to achieve is:

(a) Fix the bug that renaming an item in "Sort by Date" mode does not have any effect, and
(b) do not cause a performance regression by resorting every time an item is renamed.

The easiest way to achieve this (and not cause the regression that Emmanuel noticed) is to just keep most of my first patch, and always trigger a resorting when the model is grouped, and the sort role changes. One can still add a more sophisticated solution in the future if it seems necessary.

IMHO, this change is safe enough for 4.11.1. But maybe I'm overlooking something again - any comments are welcome.


Description
-------

1. mkdir test && cd test && touch a b c && dolphin .
2. Sort by size -> the order of the items is "a b c" because the name is used as a fallback (all files have zero size).
3. Rename "a" -> "d".
4. Note that the items are not resorted.

The problem is that KFileItemModel::setData() only re-sorts the model if the "sort role" is changed. However, in cases like the one I described above, also the name matters.

This can be fixed by triggering the re-sorting if either the "sort role" or the name changes. This could in principle cause some unnecessary calls of resortAllItems(). Therefore, I've added a quick check which verifies if the file is still sorted correctly with respect to its neighbors. (A nice side-effect is that renaming files in "Sort by name" mode only triggers resortAllItems() if this is really necessary).


Diffs (updated)
-----

  dolphin/src/kitemviews/kfileitemmodel.cpp 1b4911d 
  dolphin/src/tests/kfileitemmodeltest.cpp 513ecef 

Diff: http://git.reviewboard.kde.org/r/111721/diff/


Testing
-------

Old and new tests pass, no regressions seen so far. Hm, thinking about it again, there is a similar bug in KFileItemModel::slotRefreshItems() - can be seen quite easily by applying the patch, repeating the steps above and splitting the view after step 2. I'll have a look at that too, but it doesn't hurt to do it in two different commits anyway.


Thanks,

Frank Reininghaus

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20130806/e571b95c/attachment.htm>