Bug 383017 - Improve behavior of window icon fallback codepath
Summary: Improve behavior of window icon fallback codepath
Alias: None
Product: plasmashell
Classification: Plasma
Component: Task Manager and Icons-Only Task Manager (show other bugs)
Version: 5.9.5
Platform: Other Linux
: NOR normal
Target Milestone: 1.0
Assignee: Eike Hein
Depends on:
Reported: 2017-08-01 19:24 UTC by kdebuac.rhn
Modified: 2017-08-08 12:42 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:

Example where task manager icon doesn't match window manager icon. (10.13 KB, image/png)
2017-08-01 19:24 UTC, kdebuac.rhn

Note You need to log in before you can comment on or make changes to this bug.
Description kdebuac.rhn 2017-08-01 19:24:08 UTC
Created attachment 107017 [details]
Example where task manager icon doesn't match window manager icon.

When icon of a window changes, Task manager widget doesn't update it.

Happens always.

To reproduce:
1. Open a window (e.g. chat to a gajim contact)
2. Cause the window icon to change (e.g. contact changes status)

What happens:
Icon changes in window manager.

Icon changes in window manager, and also in task manager.

System: Fedora 25, Intel 3000 graphics
Comment 1 Eike Hein 2017-08-02 08:38:09 UTC
It doesn't use window manager icons.

*** This bug has been marked as a duplicate of bug 369658 ***
Comment 2 kdebuac.rhn 2017-08-02 14:13:14 UTC
This is not exactly the same problem as described in bug 369658.

The other bug entry describes that if there is no .desktop file, the window manager icons will be used.

This is not happening consistently - the icon is picked when the window is opened, but it's not used thereafter.

For the sake of consistency, I believe these icons should get updated.

Is there any other mechanism planned for windows to indicate their state at a glance, allowing for more states than "nominal"/"needs attention"?
Comment 3 Eike Hein 2017-08-02 14:50:17 UTC
No, currently not.
Comment 4 kdebuac.rhn 2017-08-02 15:18:17 UTC
Just to make it clear, in the "no .desktop" case, the icons *do* eventually change - but not immediately, and I don't know what triggers them.

If they are not meant to ever change after opening, this is a bug.
If they are meant to update as always, this is also a bug.

Either way, please reopen.
Comment 5 kdebuac.rhn 2017-08-02 18:04:30 UTC
I first noticed this problem after switching from F24 to F25 last week. That means the change (regression?) was introduced somewhere between

Comment 6 Eike Hein 2017-08-02 18:34:29 UTC
This isn't really a regression (and frankly not really that much of a bug, and low-priority as it's an edge case of a fallback codepath).

Basically, the data model internally keeps a cache of information about the application owning a window. Which application that is is figured out based on window metadata. When relevant window metadata changes, the cache is wiped; the next request for data from the model will rebuild the cache from scratch, running the app identification heuristic in the process.

If a request for the icon comes into the model, it asks for the icon from cache. If the cache doesn't yield an icon, it asks the window manager instead as a fallback - and adds the icon into the cache so it can be found there next time.

In other words, if the cache is wiped due to a metadata change, the WM will be asked again for the icon when the UI frontend decides it needs it. This explains why you see the icon change "sometimes": The metadata changes causing cache evictions are unrelated to the icon and just evict the icon as a side-effect (WM icon changes are not monitored as relevant metadata), and when exactly the UI frontend asks again for the icon is also situationally complex (e.g. when recreating delegates, or in cases where the model tells it to re-request /all/ data for a task).

The way to fix this is to keep track of which windows currently have their icon provided by the WM fallback, and monitor WM icon metadata changes for those windows until the fallback is no longer needed (which could happen if a window fixes its metadata at runtime so the app matching heuristic succeeds, or the app is (re-)installed correctly on the system and can now be found).

This should be done, but the priority is usually to find out why a window hits this fallback path in the first place and fix the app/install.
Comment 7 kdebuac.rhn 2017-08-02 18:54:43 UTC
I understand that it's the application's responsibility to provide a correct .desktop entry.

I don't have enough knowledge in that regard, but doesn't switching priorities to fixing installations eliminate classes of applications, like those locally installed? Not to mention those served remotely, having no installation data at all.

While those are uncommon cases, the fallback seems important for them.
Comment 8 Eike Hein 2017-08-02 18:57:55 UTC
Locally installed apps can still add .desktop files to the system.

Remote apps are true.

Which is why I wrote "this should be done" :)
Comment 9 Eike Hein 2017-08-03 12:14:32 UTC
Patch under review @ https://phabricator.kde.org/D7092
Comment 10 Eike Hein 2017-08-07 10:29:46 UTC
Git commit 75b7f5c2fa63bc1ef13ac6fb5f82c7d52bfabd6c by Eike Hein.
Committed on 07/08/2017 at 10:24.
Pushed by hein into branch 'master'.

Keep fallback icon updated

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

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.

Reviewers: #plasma, davidedmundson

Subscribers: plasma-devel

Tags: #plasma

Differential Revision: https://phabricator.kde.org/D7092

M  +10   -2    libtaskmanager/xwindowtasksmodel.cpp

Comment 11 kdebuac.rhn 2017-08-07 17:17:09 UTC
Thank you, I hope the patch makes it to Fedora soon.
Comment 12 Eike Hein 2017-08-08 12:42:04 UTC
I comitted it to master, so currently it's slated for 5.11. I may backport it to 5.10.x in a bit after making sure it didn't blow up for anyone.

Thanks for reporting and making our stuff better!