Revamp of the activity switcher backend
ClosedPublic

Authored by ivan on Feb 18 2016, 4:19 PM.

Details

Summary
  • Activities are sorted based on the last time used (an often requested feature, that makes meta+tab order activities like alt-tab orders windows - allows easy switching to the previous activity)
  • The data models are created once, and reused instead of allowing QML to instantiate them
  • Sorting logic is not duplicated in C++ and QML anymore (we now have a proper C++-instantiated model instead of having to rely on the QML one provided by KActivities QML module)
  • Activity starting/stopping/switching methods are now in the backend module, no need to go through the model

Since Plasma 5.6 can not depend on KF5.20, some parts of KActivities
code are copied in the backport subfolder. This is to be removed after
5.6 is released, and Plasma will use the new
KActivities::ActivitiesModel class.

This change is important enough to warrant this trickery.

Diff Detail

Repository
R119 Plasma Desktop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ivan updated this revision to Diff 2388.Feb 18 2016, 4:19 PM
ivan retitled this revision from to Revamp of the activity switcher backend.
ivan updated this object.
ivan edited the test plan for this revision. (Show Details)
ivan added reviewers: mart, davidedmundson, sebas.
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 18 2016, 4:19 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
mart accepted this revision.Feb 18 2016, 5:17 PM
mart edited edge metadata.
This revision is now accepted and ready to land.Feb 18 2016, 5:17 PM
This revision was automatically updated to reflect the committed changes.
davidedmundson added inline comments.Feb 18 2016, 5:50 PM
imports/activitymanager/backport/model_updaters.h
3

year.

imports/activitymanager/backport/switcheractivitiesmodel.cpp
281

just use shownActivities.contains() it's a normal QVector after all

323

why not state too?

364

This is would fail modelTest.

to ensure it's a flat model you need

if parent.isValid()
return 0;

even for an AbstractList

372

just write .at(row)

imports/activitymanager/sortedactivitiesmodel.cpp
239

this won't work.

there is no dataChanged() emitted from the source model when the lastUsedTime changes - so it won't resort

ivan marked 2 inline comments as done.Feb 18 2016, 11:24 PM
ivan added inline comments.
imports/activitymanager/backport/model_updaters.h
3

It is a really old file from KActivities. I didn't want to change the year just because it was copied here.

imports/activitymanager/backport/switcheractivitiesmodel.cpp
281

.contains would search for the pointer value, and not for any pointer that represents the same activity like it does now (the collection is sorted by activity name and id).

Now, this might be faster with std::find_if since it is a small collection, and serial search is faster than binary for small ranges, but I don't want to optimize prematurely.

323

The state change influences whether the activity is shown in the model or not, it has a special handler.

372

Heh, switching from boost classes...

imports/activitymanager/sortedactivitiesmodel.cpp
239

The source model calls dataChanged because it changes the current property of activities. And it works. Now, this might be made more robust by calling sort when lasttimeused is changed.

imports/activitymanager/sortedactivitiesmodel.cpp
228

We should cache the config as a member var and only reparse when in ::currentActivity changes. Otherwise you're parsing an entire config file (in fact 3, see last comment) lots of times.

If this gets used by a QWidget API that hammers data() on every repaint. If this is always purely QtQuick it's generally OK as the delegate's have to internally cache - but it's still once per activity rather than just once.

for a speedup open this to SimpleConfig. That saves it also loading in kglobals and the rest of the cascading.

239

Yep, you're right.
For the time order to have changed, current activity would have changed dataChanged().

This is absolutely fine then.

354

should check it's >= 0 (in both cases)

ivan marked an inline comment as done.Feb 19 2016, 8:53 PM
ivan added inline comments.
imports/activitymanager/sortedactivitiesmodel.cpp
228

This will go away - we are planning to make wallpaper exposed via dbus api because basing this code on config files is not really robust.

354

rowChanged checks that (it is technically not a signal, but semantically it is - that is why I'm using emit rowChanged)