Ensure that the sorting is correct after renaming if the items are not sorted by name, but the name is used as a fallback
Review Request #111721 - Created July 26, 2013 and submitted
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).
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 for the patch! I have found a regression: 1. Create items: a, b, c, e 2. Sort by name 3. Enable grouping 4. Rename c -> d The group header for the new item "d" is still "c". See KFileItemModel::resortAllItems for the problem: // Don't check whether items have really been moved and always emit a // itemsMoved() signal after resorting: In case of grouped items // the groups might change even if the items themselves don't change their // position. Let the receiver of the signal decide whether a check for moved // items makes sense. emit itemsMoved(KItemRange(0, itemCount), movedToIndexes); We need to find a way to inform the view, that the group headers have changed, so that we get rid of this itemsMoved signal hack (This brings us other benefits - like no complete item size hind cache clear after resorting, because we only need to send the itemsMoved signal if it is really necessary and only for items which really need to be moved)
Review request changed
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.
Revision 2 (+49 -2)