Differential D13793

[Icons KCM] Load preview pixmaps for animation on-demand and cache them
ClosedPublic
Actions

Authored by broulik on Jun 29 2018, 9:51 AM.

Details

Reviewers
mart
davidedmundson
Group Reviewers
Plasma
Commits
R119:d8b6f90e2ddb: [Icons KCM] Load preview pixmaps for animation on-demand and cache them
Summary

No need to load 18 preview pixmaps of which 12 can only be seen during the animation.
Furthermore, cache the pixmaps as ListView deletes and recreates delegates as you scroll, leading to us recreating the icon pixmaps as you scroll up and down with many themes installed

Test Plan

5.13 branch

  • Pixmaps are cached
  • KCM opens a lot faster now that only 6 pixmaps are loaded per theme
  • The animation still works fine, even when icons are missing

Diff Detail

Repository
R119 Plasma Desktop
Lint
Lint Skipped
Unit
Unit Tests Skipped
broulik created this revision.Jun 29 2018, 9:51 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 29 2018, 9:51 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik requested review of this revision.Jun 29 2018, 9:51 AM
broulik planned changes to this revision.

TODO evict cache when reloading model (installing new theme)

broulik updated this revision to Diff 36876.Jun 29 2018, 9:54 AM
  • Evict cache when installing new theme
broulik updated this revision to Diff 36878.Jun 29 2018, 10:05 AM
broulik retitled this revision from [Icons KCM] Cache preview pixmaps to [Icons KCM] Load preview pixmaps for animation on-demand and cache them.
broulik edited the summary of this revision. (Show Details)
broulik edited the test plan for this revision. (Show Details)
broulik edited the test plan for this revision. (Show Details)
mart added a subscriber: mart.Jun 29 2018, 10:22 AM
mart added inline comments.
kcms/icons/main.h
110

leftovers?

kcms/icons/package/contents/ui/main.qml
156–157

reason for 6?
should be at least documented

broulik updated this revision to Diff 36881.Jun 29 2018, 10:28 AM
  • Add comment explaining the number 6
broulik updated this revision to Diff 36882.Jun 29 2018, 10:31 AM
  • Explain why QVariantList
mart accepted this revision.Jun 29 2018, 10:31 AM
This revision is now accepted and ready to land.Jun 29 2018, 10:31 AM
davidedmundson added a subscriber: davidedmundson.Jun 29 2018, 10:32 AM
davidedmundson added inline comments.
kcms/icons/main.cpp
484

so if you request previewIcons with a limit of 6 then request again with a limit of 12 you're duplicating the first 6 in another cache.

If you're going to have this limit, would having a cache per icon then building the list on every invocation would be cleander?

(would also allow use of QPixmapCache which has a nice auto-expunge)

broulik updated this revision to Diff 37173.Jul 5 2018, 8:16 AM
broulik edited the test plan for this revision. (Show Details)
broulik added a reviewer: davidedmundson.
  • Use QPixmapCache to cache on a per-icon basis and avoid duplicating the first 6 icons when all are loaded
Closed by commit R119:d8b6f90e2ddb: [Icons KCM] Load preview pixmaps for animation on-demand and cache them (authored by broulik). · Explain WhyJul 9 2018, 1:34 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 36881

View Options

kcms/icons/main.h

Loading...
View Options

kcms/icons/main.cpp

Loading...
View Options

kcms/icons/package/contents/ui/main.qml

Loading...