Only serialize window pixmap if we're using it
ClosedPublic

Authored by broulik on Dec 5 2019, 2:06 PM.

Details

Summary

The previous QIcon::name() check wasn't sufficient as it only works with theme icon names (e.g. kate) but not icons created from an absolute path, e.g. /some/special/icon/location/kate.png.
The latter is usually the case for containerized apps which have a proper application desktop file installed but their icon in some path within the application image or some container daemon location.
Since we already store the information for whether we had to fall back to using the actual window pixmap, check for this before trying to serialize icon pixmap data.

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

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik created this revision.Dec 5 2019, 2:06 PM
Restricted Application added a project: Plasma. · View Herald TranscriptDec 5 2019, 2:06 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik requested review of this revision.Dec 5 2019, 2:06 PM
hein added a comment.Dec 17 2019, 9:48 AM

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?

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.

hein added a comment.Dec 17 2019, 3:43 PM

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.

broulik planned changes to this revision.Dec 17 2019, 3:44 PM

That means you can limit serializing to cases where the window is in usingFallbackIcon.

Good idea, I'll give it a try.

broulik updated this revision to Diff 71789.Dec 18 2019, 2:01 PM
broulik retitled this revision from Don't bother serializing window icon pixmap for known services to Only serialize window pixmap if we're using it.
broulik edited the summary of this revision. (Show Details)
  • Use usingFallbackIcon
hein accepted this revision.Sat, Dec 28, 2:57 AM

Nice :)

This revision is now accepted and ready to land.Sat, Dec 28, 2:57 AM
This revision was automatically updated to reflect the committed changes.