Keep fallback icon updated
ClosedPublic

Authored by hein on Aug 3 2017, 12:13 PM.

Details

Summary

Windows we can't find an app icon for using the normal means
get the icon used by the windowing system in the Task Manager.
This fallback icon was then not updated when changed on the
window, only occasionally as a side-effect of cache evictions
and model data requests.

This patch notes which windows hit the fallback path, and for
those windows evicts the cache when the icon changes on the
window, causing it to be re-retrieved from the windowing system
as views respond to the dataChanged signal for the decoration
role.

Evicting the entire cache is a little bit costly, but:

  • This is a fallback codepath.
  • Apps conventionally update title and icon in one go, meaning the cache is often already getting evicted anyway.
  • Icons don't change that often.

BUG:383017

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
Lint Skipped
Unit
Unit Tests Skipped
hein created this revision.Aug 3 2017, 12:13 PM
Restricted Application added a project: Plasma. · View Herald TranscriptAug 3 2017, 12:13 PM
davidedmundson requested changes to this revision.Aug 4 2017, 9:38 PM

Concept seems fine.

I'm possibly wrong, but instead of the new set couldn't you
if (appDataCache.contains(window) && appDataCache[window].icon.isValid())

it has the advantage that it can't go out of sync, and that (if written well) we could avoid another hash lookup

libtaskmanager/xwindowtasksmodel.cpp
337

we potentially end up with two DecorationRole changes here

we should remove this line

and the "else" in the next one.

This revision now requires changes to proceed.Aug 4 2017, 9:38 PM
hein added a comment.Aug 4 2017, 10:26 PM

I'm possibly wrong, but instead of the new set couldn't you
if (appDataCache.contains(window) && appDataCache[window].icon.isValid())

I don't understand on multiple levels :)

There's no QIcon::isValid, and even if there was, the icon being in the cache and being valid could be the result of the the preferred codepath (from .desktop file) in which case we don't want to emit DecorationRole based on a NET::WMIcon prop change. Basically this patch is about "react to NET::WMIcon for windows we actually show it for, but only those".

hein updated this revision to Diff 17738.Aug 4 2017, 10:28 PM
hein edited edge metadata.

Fix dumb logic that happened from never actually reading the contents of the else if condition ;)

hein updated this revision to Diff 17739.Aug 4 2017, 10:35 PM

Fix logic. This was not actually doing what I wanted.

davidedmundson accepted this revision.Aug 5 2017, 8:53 AM
This revision is now accepted and ready to land.Aug 5 2017, 8:53 AM
This revision was automatically updated to reflect the committed changes.