[KItemListView] Check if KItemListStyleOption actually changed before emitting a change
ClosedPublic

Authored by broulik on Mar 19 2018, 12:24 PM.

Details

Summary

This avoids work being done when it doesn't need to be.
For instance, the preview generator waits for everything to have settled using a 200ms timer before generating a preview. This timer fired also in response to onStyleOptionChanged and needlessly delayed preview generation when navigating between folders despite the style option (e.g. icon size, view mode) not having changed.

Test Plan
  • Previews generate noticeably faster when moving between folders with the same icon size and settings.
  • Switching between view modes still works
  • Navigating between folders of varying view modes still triggers the old delay

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik requested review of this revision.Mar 19 2018, 12:24 PM
broulik created this revision.
markg added a subscriber: markg.Mar 19 2018, 1:04 PM

I wonder if the fix is really fixing an issue here or just masking it?
You're not setting the style if it's already set. That indicates something somewhere tries to set it when it was already set and that something should not do that.

Also, the commit message is a bit odd. It talks about a timer yet the change does not touch any timers. It merely prevents a style change when the style is already changed.

broulik updated this revision to Diff 29915.Mar 19 2018, 1:58 PM
broulik edited the summary of this revision. (Show Details)
  • Apply check in all places KItemListStyleOption is used

Code looks good, but I have the same question as @markg: what is this timer you mentioned in the commit message?

void KFileItemListView::onStyleOptionChanged(const KItemListStyleOption& current, const KItemListStyleOption& previous)
{
    KStandardItemListView::onStyleOptionChanged(current, previous);
    triggerIconSizeUpdate();
}

This is executed when the style option changes, triggerIconSizeUpdate() starts a m_updateIconSizeTimer of 300ms and during that time preview generation is paused as to not repeatedly recreate icons when you e.g. change the icon size with a slider. However, this would also needlessly delay icon generation when moving between folders as without this patch it always signals a style option change when moving between folders even if both folders use the same icon size and view mode.

markg accepted this revision.Mar 28 2018, 3:35 PM

It makes sense to me now. Thank you for elaborating on the timer.

This revision is now accepted and ready to land.Mar 28 2018, 3:35 PM
This revision was automatically updated to reflect the committed changes.