Differential D4600

TaskManager: add icon to cache after painfully querying it over XCB/NETWM.
ClosedPublic
Actions

Authored by dfaure on Feb 13 2017, 6:14 PM.

Details

Reviewers
hein
Commits
R871:1f2a372fd840: TaskManager: add icon to cache after painfully querying it over XCB/NETWM.
R120:1f2a372fd840: TaskManager: add icon to cache after painfully querying it over XCB/NETWM.
Summary

Hitting again a bug where switching desktops shows a delay of several seconds
before the taskbar updates, I noticed that this code path was called 81
times in 7 seconds (!) for kmail (maybe because I start it from cmdline
rather than via a .desktop file).

After the 4 slow calls to KWindowSystem::icon it seems definitely worth
it to cache the result.

Overall the main performance bug is still there though, switching
desktops is still slow, but now all backtraces show QML triggered
by model/view dataChanged signals, no idea yet, other than hoping
for the pending QSFPM patches to help with this (no need to re-sort
when the dataChanged roles are unrelated to the roles used for sorting).

Diff Detail

Repository
R120 Plasma Workspace
Branch
Plasma/5.9
Lint
No Linters Available
Unit
No Unit Test Coverage
dfaure updated this revision to Diff 11304.Feb 13 2017, 6:14 PM
dfaure retitled this revision from to TaskManager: add icon to cache after painfully querying it over XCB/NETWM..
dfaure updated this object.
dfaure edited the test plan for this revision. (Show Details)
dfaure added a reviewer: hein.
Restricted Application added a project: Plasma. · View Herald TranscriptFeb 13 2017, 6:14 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
hein accepted this revision.Feb 13 2017, 6:30 PM
hein edited edge metadata.

This is partially because the KMail team is unable to fix the name of their .desktop file after telling them multiple times over the course of two years (it doesn't match their WM_CLASS).

And yeah, the QSFPM patches will help dramatically.

This revision is now accepted and ready to land.Feb 13 2017, 6:30 PM
dfaure added a comment.Feb 13 2017, 6:33 PM

So what's the suggested fix for kmail? (I sense a communication issue here rather than bad will).
WM_CLASS is obscure to most devels including me. How do we set it, in case renaming the desktop file isn't an option?

hein added a comment.Feb 13 2017, 6:40 PM

I went through it multiple times with different KMail people and they promised to fix it but somehow it doesn't happen ... I think it's inertia from some sort of legacy compat hell they locked themselves into. If memory serves me right their .desktop file is kmail.desktop but their Qt product/component name is kmail2, which is what WM_CLASS for the main window gets derived from ultimately. One of them needs to be changed for things to match up.

dfaure added a comment.Feb 13 2017, 6:50 PM

It is on my TODO list now. It will happen.

dfaure closed this revision.Feb 13 2017, 6:58 PM
dfaure added a comment.Feb 18 2017, 10:41 AM

Looking at the code in qxcbintegration.cpp, it seems that WM_CLASS is in fact derived from

  1. -name xcb-specific commandline argument, let's ignore that
  2. $RESOURCE_NAME env var, no idea what that is, let's ignore that
  3. the basename of argv[0], i.e. the name of the executable. kmail's executable is called kmail.

This is then followed by QCoreApplication::applicationName which is indeed "kmail2".

So we end up with WM_CLASS(STRING) = "kmail", "kmail2", as confirmed by xprop.

I notice that e.g. firefox has WM_CLASS(STRING) = "Navigator", "Firefox". So the second string is the one that matters?

hein added a comment.Feb 20 2017, 9:42 AM

Yep. The first string is called "Instance" and the second is called "Class". I.e. the latter is supposed to represent the application, the former is supposed to represent a unique instance of it.

Revision Contents
Changeset List

Diff 11304

View Options

libtaskmanager/xwindowtasksmodel.cpp

Loading...