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]

> On July 28, 2013, 12:36 p.m., Emmanuel Pescosta wrote:
> > 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)

Thanks for testing it. Good catch! Hm, I'll try to figure out how to solve this - maybe we could emit a signal "groupsChanged" or something like that if the groups change, but the item positions don't.

But maybe it's better to consider that for master only. I don't want to risk regressions, and considering that I'm apparently the first one who found that bug, it's probably not really urgent ;-)


- Frank


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


On July 26, 2013, 3:37 p.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111721/
> -----------------------------------------------------------
> 
> (Updated July 26, 2013, 3:37 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> 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
> -----
> 
>   dolphin/src/kitemviews/kfileitemmodel.cpp d174cf6 
>   dolphin/src/tests/kfileitemmodeltest.cpp 0ad7a37 
> 
> 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/20130728/bd231685/attachment.htm>