StatusItemNotifier: fix overlays by name with icons by name
ClosedPublic

Authored by pino on Jan 5 2019, 10:33 AM.

Details

Summary

Setting an overlay by name results in a QIcon for it created; OTOH,
this icon is never used to create the final image in case the
main/attention icons are set by name too.

Since KIconEngine supports overlays natively, directly pass the list of
overlays (with just one element -- the overlay set) to it. As result,
main/attention icons by name are created directly with the requested
overlay by name.

Test Plan
  • builds fine
  • main/attention icon by name + overlay by name works now, showing the requested overlay
  • main/attention icon by name + overlay by pixmap still works as before
  • main/attention icon by pixmap is unaffected, no matter whether/which overlay is set
  • the system tray icons of juk, and amarok now have the play/pause overlays

Diff Detail

Repository
R120 Plasma Workspace
Branch
sni-overlays (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6663
Build 6681: arc lint + arc unit
pino created this revision.Jan 5 2019, 10:33 AM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 5 2019, 10:33 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
pino requested review of this revision.Jan 5 2019, 10:33 AM
apol added a subscriber: apol.Jan 7 2019, 3:33 PM

+1
Makes a lot of sense overall, I'd just like to make sure we're not missing something.

dataengines/statusnotifieritem/statusnotifieritemsource.cpp
296

Can this bit be removed then?

pino added inline comments.Jan 7 2019, 3:47 PM
dataengines/statusnotifieritem/statusnotifieritemsource.cpp
296

No, this is needed for the "overlay by pixmap" case: overlayNames is empty (because no overlay name is available), while overlay is not null (as read by the pixmap)

apol accepted this revision.Jan 7 2019, 3:49 PM
This revision is now accepted and ready to land.Jan 7 2019, 3:49 PM
pino added a comment.Jan 7 2019, 3:58 PM

Thanks -- which branch should I push this to? Plasma/5.12, Plasma/5.14, or only master for now?

pino added a comment.Jan 28 2019, 7:18 AM
In D17983#388160, @pino wrote:

Thanks -- which branch should I push this to? Plasma/5.12, Plasma/5.14, or only master for now?

ping?

Thanks -- which branch should I push this to?

Plasma/5.12

If there's no risk of regression, yes. That's a judgement call I think you're in the best position to make.
(IMHO it should be fine, the absolutely worst case is some slightly wrong icons)

Plasma/5.14

5.15 now, but yes.

pino closed this revision.Jan 28 2019, 8:52 PM