Differential D2676

Improved and optimized Pager and Activity Pager
ClosedPublic
Actions

Authored by hein on Sep 6 2016, 10:11 AM.

Details

Reviewers

davidedmundson

Group Reviewers

Plasma

Commits

R119:4b9d8046347b: Improved and optimized Pager and Activity Pager

Summary

Unify Desktop and Activity Pager applets. The latter was an out-of- sync fork of the former, with 99% code copied needlessly. Separate commit to kdeplasma-addons pending that slashes the Activity Pager down to a .desktop file.

Share window state monitoring backend with the Task Manager by porting to libtaskmanager. Reduces the window state monitoring in a typical plasmashell instance from two to one. Saves CPU time, memory use and improves support for Wayland.

Don't destroy and recreate all delegates every time window focus or other window state changes. Saves a lot of CPU time.

Save various items and objects by e.g. creating window icon and
desktop label items only when needed and cleaning up frontend code.

Lazy-create

the data source needed to show the activity manager once it's needed.
Results in much lighter Pager (at least two items per window and one
item per desktop) with default settings.

Avoid doing tons of unnecessary calculations during DND.

Some minor HiDPI scaling fixes.

Other cleanup: More visualization-agnostic backend, coding style.

Depends on D2674, D2675.

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.

hein updated this revision to Diff 6474.Sep 6 2016, 10:11 AM

hein retitled this revision from to Improved and optimized Pager and Activity Pager.

hein updated this object.

hein edited the test plan for this revision. (Show Details)

hein added reviewers: Plasma, davidedmundson.

hein added dependencies: D2675: Add desktop layout rows property., D2674: Add optional filtering by SkipPager window state..

hein added a subscriber: plasma-devel.

Restricted Application added a project: Plasma. · View Herald TranscriptSep 6 2016, 10:11 AM

hein added a dependent revision: D2677: Reduce Activity Pager to .desktop file referencing the Pager package..Sep 6 2016, 10:13 AM

mart mentioned this in D2677: Reduce Activity Pager to .desktop file referencing the Pager package..Sep 6 2016, 11:32 AM

Generally awesome.
One major bug, rest are tiny comments.

Will give it a second review later, as it's a bit patch and I got tired :)

applets/pager/package/contents/ui/main.qml
175–176	add some readonly's over here. It's apparently marginally faster; plus it can make things a bit more readable
402	why?
applets/pager/plugin/pagermodel.cpp
77	you delete them in if (!instanceCount) in the dstror, but you're creating them statically. that will crash on create/remove/create and it's a bit weird.
85	this is effectively duped in refreshDatasSource()
97	I'm a bit confused here. If VirtualDesktop changes you're resetting your own knowledge of if the desktop is shown or not. Why?
251	emit countChanged in both places
430	this is a new feature? It doesn't work in the current version anyway. Note there is a DBus verison of showDashboard which is maybe sensible to use? It cuts out some of the X specific code.

mart added a subscriber: mart.Sep 6 2016, 12:50 PM

mart added inline comments.

applets/pager/plugin/pagermodel.cpp
251	couldn't be instead connected modelreset,rowsinserted,rowsdeleted signals to countchanged signal?
430	the current pager has it (settings, "when clicking on current desktop")

hein marked 9 inline comments as done.Sep 6 2016, 3:51 PM

hein added inline comments.

applets/pager/package/contents/ui/main.qml
175–176	Will do.
402	Using Quick's drag functionality on the delegate breaks the binding that will otherwise cause it to be repositioned based on the model's Geometry role. Resetting the model destroys and recreates the delegate. The old code relied on this happening implicitly (because that model would reset tens of thousands of times throughout a typical session on window state changes), in the new code it needs to be done explicitly. It would be possible to optimize this more, but it's not really worth more complicated code (or extra objects like Bindings) considering DND is rare and operation doesn't impact UX. I'll add a comment though.
applets/pager/plugin/pagermodel.cpp
77	Fixed.
85	No, it's not. refreshDataSource() doesn't set anything on the models. What this lambda does is make sure that in VirtualDesktopsPager mode, the Pager always only shows windows that are on the current activity, by setting the activity filter to the current activity when it changes. Nothing in refreshDataSource() (which also isn't run when the activity changes) or refresh() (ditto) does this.
97	Because this is what the old code did, and I assume whoever wrote it figured out that kwin cancels show-desktop state when changing virtual desktops.
251	It's not needed in the first case because refresh() does it, but good catch on the second.
430	Not new feature, and works just fine for me?

Fixed ActivityInfo/VirtualDesktopInfo instanciation.
Added code comment to post-drag model reset.
Update count property when disabling model.
Mark layout-related properties readonly.

One idea to think about if you think it makes sense, otherwise ship it!

applets/pager/plugin/pagermodel.cpp
85	You're right. Got confused.
97	ok. That makes sense Though: would KWindowSystem::showingDesktopChanged would be a more robust approach to that? or info.setShowingDesktop(!info.showingDesktop()); or ...if you did switch to PlasmaShell showDashboard over the DBus (which Qt won't actually send over DBus) you get toggling done for free.

This revision is now accepted and ready to land.Sep 6 2016, 4:24 PM

Closed by commit R119:4b9d8046347b: Improved and optimized Pager and Activity Pager (authored by hein). · Explain WhySep 6 2016, 4:40 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

		Path
M		applets/pager/CMakeLists.txt (9 lines)
M		applets/pager/package/contents/ui/configGeneral.qml (7 lines)
M		applets/pager/package/contents/ui/main.qml (560 lines)
M		applets/pager/package/metadata.desktop (2 lines)
D	M	applets/pager/plugin/model.h (114 lines)
D	M	applets/pager/plugin/model.cpp (252 lines)
D	M	applets/pager/plugin/pager.h (167 lines)
D	M	applets/pager/plugin/pager.cpp (637 lines)
A	M	applets/pager/plugin/pagermodel.h (109 lines)
A	M	applets/pager/plugin/pagermodel.cpp (526 lines)
M		applets/pager/plugin/pagerplugin.cpp (8 lines)
A	M	applets/pager/plugin/windowmodel.h (52 lines)
A	M	applets/pager/plugin/windowmodel.cpp (120 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	6474	cb47729		Sep 6 2016, 10:11 AM	★	★
Diff 2	6493	cb47729	* Fixed ActivityInfo/VirtualDesktopInfo instanciation.	Sep 6 2016, 3:54 PM	★	★
Diff 3	6496	9c8b7b7	rPLASMADESKTOP4b9d8046347b17ef7337ae31f1c2cdfc73a87781	Sep 6 2016, 4:40 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
6bcfbf9ff3a7	f028cdc5b673	cb477299c8a4	Eike Hein	Improved and optimized Pager and Activity Pager (Show More…)	Sep 6 2016, 10:10 AM

Status	Author	Revision
Closed	hein	D2677 Reduce Activity Pager to .desktop file referencing the Pager package.
Closed	hein	D2676 Improved and optimized Pager and Activity Pager
Closed	hein	D2675 Add desktop layout rows property.
Closed	hein	D2674 Add optional filtering by SkipPager window state.