Review Request 112312: Make thumbnail generation more responsive to scrolling/paging

[Frank Reininghaus]

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


Thanks for your investigation, Christoph! I wasn't aware at all that the 'LongInterval' is also being used when the icon size is not changed at all.

I have neither written this code nor worked with it yet, so please forgive me if my questions are stupid and/or do not make much sense.


(a) I see that the intention is to use 'LongInterval' when the icon size changes. This certainly makes sense if previews are enabled, to prevent that new preview jobs are started and killed all the time when zooming with, e.g., Ctrl+Wheel quickly. However, if I read the code and your patch correctly, the 'LongInterval' is set only when m_updateIconSizeTimer has expired for the first time, right? This means that the first icon size change will only have a small delay, and also that the long delay remains after zooming is finished (because the 'ShortInterval' is set only when the m_updateVisibleIndexRangeTimer has expired the next time). This means that the next scroll operation will only result in new previews after a rather long delay, which is what you are actually trying to fix here. Or 
 am I getting something wrong here?


(b) Maybe even worse: when changing the zoom level, both KFileItemListView::onItemSizeChanged() and KFileItemListView::onStyleOptionChanged() are called, which trigger the two timers m_updateVisibleIndexRangeTimer and m_updateIconSizeTimer, respectively, both with the same delay. The order in which they expire is then undefined, right? But depending on which timer fires first, the effects will be quite different:

KFileItemListView::updateVisibleIndexRange() will stop the other timer and set the interval to 'ShortInterval'.

KFileItemListView::updateIconSize() will stop the other timer and set the interval to 'LongInterval'.

If my analysis is correct, the length of the delay for subsequent icon/preview updates will be random.


Again, I'm not familiar with this code, so my conclusions might be wrong, but it seems to me that your patch might not guarantee that the effect that we want is achieved. I don't have a good idea for a proper fix at the moment, but I'll think about it...

- Frank Reininghaus


On Aug. 27, 2013, 3:03 p.m., Christoph Feck wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112312/
> -----------------------------------------------------------
> 
> (Updated Aug. 27, 2013, 3:03 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> The thumbnail generation is delayed by several timers to make the user interface more responsive. During scrolling, however, it delayed the generation by 300 ms, which I felt to long.
> 
> Looking at the code, this long interval is supposed to be used for zooming, where previously generated thumbnails are recreated at a new size, and moving the zoom slider would otherwise recreated them too often.
> 
> But the long interval is also used for scrolling, because the thumbnail generation is broken up into multiple chunks, and only the first chunk used the short interval. If you scroll down to the place where no thumbnails have been generated yet, it waits for a long time before resuming.
> 
> Suggested patch fixes this.
> 
> 
> This addresses bug 322093.
>     http://bugs.kde.org/show_bug.cgi?id=322093
> 
> 
> Diffs
> -----
> 
>   dolphin/src/kitemviews/kfileitemlistview.h 49ff773 
>   dolphin/src/kitemviews/kfileitemlistview.cpp 1f0fcbd 
> 
> Diff: http://git.reviewboard.kde.org/r/112312/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Christoph Feck
> 
>

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