D25762: Don't bother serializing window icon pixmap for known services

[Kai Uwe Broulik]

broulik planned changes to this revision.
broulik added a comment.


  > That means you can limit serializing to cases where the window is in 
usingFallbackIcon.
  
  Good idea, I'll give it a try.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D25762

To: broulik, #plasma, hein
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25762: Don't bother serializing window icon pixmap for known services

[Eike Hein]

hein added a comment.


  Ah right, I didn't look at the code context and forgot we don't just load the 
custom pixmap in the same function body but also in Private::icon. But we also 
do this there:
  
  `usingFallbackIcon.insert(window)`
  
  That means you can limit serializing to cases where the window is in 
`usingFallbackIcon`.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D25762

To: broulik, #plasma, hein
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25762: Don't bother serializing window icon pixmap for known services

[Kai Uwe Broulik]

broulik added a comment.


  > why don't we just change the check to QIcon::isNull?
  
  We still want to serialize custom pixmap data icons which won't be null.
  What we want to do is tell between "icon on disk" and "icon from pixmap data" 
which we can't really at the moment.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D25762

To: broulik, #plasma, hein
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25762: Don't bother serializing window icon pixmap for known services

[Eike Hein]

hein added a comment.


  If the concern is that QIcon::name isn't good enough but we actually have an 
icon inside, why don't we just change the check to QIcon::isNull?

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D25762

To: broulik, #plasma, hein
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, zachus, fbampaloukas, 
GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D25762: Don't bother serializing window icon pixmap for known services

[Kai Uwe Broulik]

broulik created this revision.
broulik added reviewers: Plasma, hein.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
broulik requested review of this revision.

REVISION SUMMARY
  A service usually has a proper icon somewhere on disk.
  `QIcon::name()` only works with theme icon names (e.g. `kate`) but not icons 
created from absolute path, e.g. `/some/special/icon/location/kate.png`.
  The latter is usually the case for containerized apps which have their icon 
set to some path within the application image or some container daemon location.

TEST PLAN
  I noticed my Telegram Snap had its icon data serialized despite having found 
the correct desktop file/KService. With this patch it checks that it has a 
service which likely has a proper icon and doesn't bother serializing the 
window pixmap.
  
  The service might not have a proper icon after all but then it also won't 
have one in the launcher to begin with, so I think this is a valid 
optimization, albeit only for master branch.
  
  There is a KF6 task to address the `QIcon` issue: T12155 


REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D25762

AFFECTED FILES
  libtaskmanager/xwindowtasksmodel.cpp

To: broulik, #plasma, hein
Cc: plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, 
ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, ahiemstra, mart