[KStatusNotifierItem] use fallback sizes when none is available
Needs ReviewPublic

Authored by pino on Feb 5 2019, 9:52 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

When converting a QIcon to a vector of images to D-Bus, the list of
available sizes is used to extract the pixmaps of the icon. In case
the engine of the icon advertizes no sizes, then no pixmaps are sent:
this is the case of the SVG icon engine shipped with QtSvg, so creating
a QIcon from an SVG file means nothing is sent for it.

As solution, use a list of few well-known sizes in case no size is
available: this way there is the possibility to have some pixmaps for
that icon.

Test Plan
  • create a KStatusNotifierItem
  • create a QIcon from a SVG file
  • set that QIcon as pixmap for the KStatusNotifierItem, e.g. using setIconByPixmap

Without this change, no icon is shown in the Plasma tray.

Diff Detail

Repository
R289 KNotifications
Branch
ksni-fallback-icons (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 7929
Build 7947: arc lint + arc unit
pino created this revision.Feb 5 2019, 9:52 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptFeb 5 2019, 9:52 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
pino requested review of this revision.Feb 5 2019, 9:52 PM
bruns added a subscriber: bruns.Feb 20 2019, 6:22 PM

While this may be necessary in some cases, I wonder if there shouldn't be some more emphasis on using the IconName instead of a pixmap:

  • The icon stays scalable
  • Less data to serialize/deserialize over DBus

Additionaly, the StatusNotifierHost could be to extended with a property denoting its current Icon size, so only sizes in use are serialized. This would help the cases where an application actually paints the Icon and can not use IconName + OverlayIconName.

apol added a subscriber: apol.Mar 5 2019, 1:24 PM

Makes sense to me +1

Would it be possible to add a unit test?