Review Request 111808: Introduce a new signal for the case that the groups have changed, even though no items have been moved at all

[Frank Reininghaus]

> On Aug. 2, 2013, 2:55 p.m., Emmanuel Pescosta wrote:
> > Crash in KItemListSizeHintResolver::itemsMoved - out of range error! :( 
> > When I change the sorting type if grouping is enabled.
> > 
> > You can fix it by replacing the line "const int newIndex = movedToIndexes.at(i);" with "const int newIndex = movedToIndexes.at(i - range.index);" in KItemListSizeHintResolver::itemsMoved, because the moveToIndexes list starts from 0 and not from range.index.
> > 
> > But everything else works fine :) Ship it after you have fixed the out of range error!

Good catch, thanks! I had already suspected that this patch might show latent bugs. The 'first moved index' is always 0 at the moment, so it seemed plausible that there is code which implicitly assumes that this is always the case. I did check that KItemListSelectionManager does the right thing, but I had somehow missed the problem you've found (even though it's obvious now that it will always happen if the resorting does not affect the first item).


- Frank


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


On July 30, 2013, 10:08 p.m., Frank Reininghaus wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/111808/
> -----------------------------------------------------------
> 
> (Updated July 30, 2013, 10:08 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> This is based on https://git.reviewboard.kde.org/r/111807/.
> 
> This patch aims to remove the need to emit the "itemsMoved" signal if no items have been moved at all. Up to now, the view still needs this signal to adapt to the (possibly changed) groups. I think it's better to emit a new signal in that case.
> 
> The benefits of this patch include:
> * Can improve the performance: no expensive updates needed if no items have been moved after a resorting.
> * Allows to re-enable some unit tests which have been disabled some time ago.
> * Makes it easier to find a better version of https://git.reviewboard.kde.org/r/111721/
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kfileitemmodel.cpp 1b4911d 
>   dolphin/src/kitemviews/kitemlistview.h 6467b8c 
>   dolphin/src/kitemviews/kitemlistview.cpp 0c3183c 
>   dolphin/src/kitemviews/kitemmodelbase.h 70f6883 
>   dolphin/src/tests/kfileitemmodeltest.cpp 0ad7a37 
> 
> Diff: http://git.reviewboard.kde.org/r/111808/diff/
> 
> 
> Testing
> -------
> 
> Unit tests pass. Haven't seen any regressions so far.
> 
> 
> Thanks,
> 
> Frank Reininghaus
> 
>

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