Updating the wallpaper cache only when the configuration file settles
AbandonedPublic

Authored by ivan on Sep 20 2016, 3:26 PM.

Details

Reviewers
davidedmundson
mart
Group Reviewers
Plasma
Summary

The file changed even can happen often, we should wait a bit before
reacting.

Diff Detail

Repository
R119 Plasma Desktop
Branch
Plasma/5.8
Lint
No Linters Available
Unit
No Unit Test Coverage
ivan updated this revision to Diff 6834.Sep 20 2016, 3:26 PM
ivan retitled this revision from to Updating the wallpaper cache only when the configuration file settles.
ivan updated this object.
ivan edited the test plan for this revision. (Show Details)
ivan added reviewers: Plasma, davidedmundson, mart.
Restricted Application added a project: Plasma. · View Herald TranscriptSep 20 2016, 3:26 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik added inline comments.
imports/activitymanager/sortedactivitiesmodel.cpp
49

Not needed?

mart added inline comments.Sep 21 2016, 9:29 AM
imports/activitymanager/sortedactivitiesmodel.cpp
65

is a blind timer the only way to wait enough? :/

imports/activitymanager/sortedactivitiesmodel.cpp
197

Now it's a sharedconfig (a *very* sensible change) - you don't even need to call reparseConfiguration().

that means it's fairly cheap to call reload.

that means you don't need the timer.

ivan added a comment.Sep 22 2016, 10:53 AM

The timer is here so that we don't react if a couple of changes happen one after another, and go over the config to extract the wallpapers (just like having delayed config syncs in the fist place).

While config (when shared) reloading is fast, going through the config is still not free.

mart edited edge metadata.Sep 28 2016, 9:15 AM

any update on that?

ivan added a comment.Sep 28 2016, 9:26 PM

Well, I'm waiting for the response.

I said that the point of the timer is not only to be able to tell that the file has been written, it is (like all the delay-timer tactics we are using everywhere) to flatten out multiple consecutive events.

imports/activitymanager/sortedactivitiesmodel.cpp
49

Not needed. Was hoping I'll find a secret constructor that sets everything in the QTimer :)

As I said last time.

Now it's a sharedconfig you don't need to call reparseConfiguration().

You still are.
This will have a far bigger impact than a timer to compress comparing two small lists...

Also, as you've pointed out, we have a timer compressing writes to the file. In theory, it's not going to be updated multiple times in a short succession..and if it is, you need to find out why.

ivan added a comment.Sep 29 2016, 11:17 AM

It works without the timer, but not without reparseConfiguration.

I need to see why.

ivan abandoned this revision.Sep 29 2016, 11:43 AM

New patch is here: https://phabricator.kde.org/D2886

(it is no longer about config file settling, so I opened a separate one)