Fix overlayIcon sometimes not visible
Needs RevisionPublic

Authored by davidre on Tue, Mar 17, 6:10 PM.

Details

Reviewers
kmaterka
broulik
davidedmundson
Group Reviewers
Plasma
Summary

Workaround a specific behavior in IconItem if the source is a QIcon with a name
and pixmaps. IconItem will use the name to find the original svg in any case
and render that. This causes the rendered pixmaps that include the overlay inside
the icons to be ignored by IconItem. This happens only when the service has a
IconName but no IconPixmap (because the data engine prefers that) and
a OverlayIconName.

Test Plan

kstatusnotifiertest from knotifications looks like this:

Diff Detail

Repository
R120 Plasma Workspace
Branch
workaround (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 23843
Build 23861: arc lint + arc unit
davidre created this revision.Tue, Mar 17, 6:10 PM
Restricted Application added a project: Plasma. · View Herald TranscriptTue, Mar 17, 6:10 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidre requested review of this revision.Tue, Mar 17, 6:10 PM
davidre edited the test plan for this revision. (Show Details)Tue, Mar 17, 6:11 PM

I don't think the described behavior is not a bug in IconItem but rather a quirk in the interaction of all three (data engine, IconItem and KIconEngine) components. SO nothing to fix on that side.

Can we just set/clear OverlayIconName depends of icon presence then overlays: model.OverlayIconName ?

Can we just set/clear OverlayIconName depends of icon presence then overlays: model.OverlayIconName ?

Of course thats much easier! Thank you!

davidre updated this revision to Diff 77866.Tue, Mar 17, 9:58 PM

Only set the role conditionally

kmaterka requested changes to this revision.Tue, Mar 17, 10:14 PM

Yep, icons in SNI are a mess...
Icons are processed in StatusNotifierItemSource, including overlay - that's were this should be fixed. From you comment I see it won't be easy...
This is a workaround, it works, but makes the code even more messy.

So... if you are brave enough, you can rewrite icon handling in SNI. Idea is to:

  • remove custom icon handling code from StatusNotifierItemSource
  • pass data directly to QML
  • move icon display logic to QML

What do you think?

applets/systemtray/systemtraymodel.cpp
321

code formatting (missing space after if)

This revision now requires changes to proceed.Tue, Mar 17, 10:14 PM

Yep, icons in SNI are a mess...
Icons are processed in StatusNotifierItemSource, including overlay - that's were this should be fixed. From you comment I see it won't be easy...
This is a workaround, it works, but makes the code even more messy.

So... if you are brave enough, you can rewrite icon handling in SNI. Idea is to:

  • remove custom icon handling code from StatusNotifierItemSource
  • pass data directly to QML
  • move icon display logic to QML

    What do you think?

That was my initial idea but @broulik said

cant really change that though
data engines are basically public api
i.e. no behavior changes

So we can't just remove Icon from the engine. We could keep Icon around for legacy and introduce IconPixmap and friends would be an indea

It seems it is possible to do this (removing stuff from the data engine) after all. I will try to work on this in the next time

It seems it is possible to do this (removing stuff from the data engine) after all. I will try to work on this in the next time

IMO, ideally StatusNotifierItemSource should just expose most of properties from this specification without any changes:
https://www.freedesktop.org/wiki/Specifications/StatusNotifierItem/StatusNotifierItem/
For some complex types it should be allowed to do conversion etc.

Summary of properties:

SNI propertyIn specificationSimple/ComplexDataSource propertyComments
CategoryYesSimpleCategoryDirect copy
IdYesSimpleIdDirect copy
TitleYesSimpleTitleDirect copy
StatusYesSimpleStatusDirect copy
WindowIdYesSimpleWindowIdDirect copy
ItemIsMenuYesSimpleItemIsMenuDirect copy
AttentionMovieNameYesSimpleAttentionMovieNameDirect copy
ToolTipYesComplexToolTipTitle, ToolTipSubTitle, ToolTipIconConverted to separate properties, additional logic
MenuYesComplex-Converted to QMenu, special handling
IconThemePathNo!SimpleIconThemePathDirect copy
IconNameYesSimpleIconNameOnly if IconPixmap is empty
IconPixmapYesComplexNot availableUsed as part of Icon
OverlayIconNameYesSimpleOverlayIconNameOnly if OverlayIconPixmap is empty
OverlayIconPixmapYesComplexNot availableUsed as part of Icon
--SimpleIconComplicated logic to create it from all Icon properties
AttentionIconNameYesSimpleAttentionIconNameOnly if AttentionIconPixmap is empty
AttentionIconPixmapYesNot availableUsed as part of AttentionIcon
--SimpleAttentionIconComplicated logic to create it from both AttentionIcon properties

I skipped: IconsChanged, StatusChanged, TitleChanged, ToolTipChanged, these are not relevant (and not used anymore).

I would suggest to:

  • In StatusNotifierItemSource:
    • Always set IconName, OverlayIconName, AttentionIconName, regardless if IconPixmap is set or not
    • Add new properties for all pixmaps, convert them to proper type (there is function for that: imageVectorToPixmap())
  • Icon logic should be in StatusNotifierItem.qml
    • Ignore "Icon" property
    • Implement similar logic in QML, everything is available in PlasmaCore.IconItem
    • Use icon name first, then pixmap (align to specification: "Visualizations are encouraged to prefer icon names over icon pixmaps if both are available")

Then, is a separate diff, remove unused properties/logic from StatusNotifierItemSource (in Plasma 6?).

This ensures backward compatibility. I'm pretty sure StatusNotifierItemSource is not used outside of Plasma, but if it this is part of the public API then needs to stay for now.
Presentation logic is moved to correct layer. That should simplify it a little bit, I had hard time understanding it for the first time.

It seems it is possible to do this (removing stuff from the data engine) after all. I will try to work on this in the next time

IMO, ideally StatusNotifierItemSource should just expose most of properties from this specification without any changes:
https://www.freedesktop.org/wiki/Specifications/StatusNotifierItem/StatusNotifierItem/
For some complex types it should be allowed to do conversion etc.

Summary of properties:

SNI propertyIn specificationSimple/ComplexDataSource propertyComments
CategoryYesSimpleCategoryDirect copy
IdYesSimpleIdDirect copy
TitleYesSimpleTitleDirect copy
StatusYesSimpleStatusDirect copy
WindowIdYesSimpleWindowIdDirect copy
ItemIsMenuYesSimpleItemIsMenuDirect copy
AttentionMovieNameYesSimpleAttentionMovieNameDirect copy
ToolTipYesComplexToolTipTitle, ToolTipSubTitle, ToolTipIconConverted to separate properties, additional logic
MenuYesComplex-Converted to QMenu, special handling
IconThemePathNo!SimpleIconThemePathDirect copy
IconNameYesSimpleIconNameOnly if IconPixmap is empty
IconPixmapYesComplexNot availableUsed as part of Icon
OverlayIconNameYesSimpleOverlayIconNameOnly if OverlayIconPixmap is empty
OverlayIconPixmapYesComplexNot availableUsed as part of Icon
--SimpleIconComplicated logic to create it from all Icon properties
AttentionIconNameYesSimpleAttentionIconNameOnly if AttentionIconPixmap is empty
AttentionIconPixmapYesNot availableUsed as part of AttentionIcon
--SimpleAttentionIconComplicated logic to create it from both AttentionIcon properties

I skipped: IconsChanged, StatusChanged, TitleChanged, ToolTipChanged, these are not relevant (and not used anymore).

I would suggest to:

  • In StatusNotifierItemSource:
    • Always set IconName, OverlayIconName, AttentionIconName, regardless if IconPixmap is set or not
    • Add new properties for all pixmaps, convert them to proper type (there is function for that: imageVectorToPixmap())
  • Icon logic should be in StatusNotifierItem.qml
    • Ignore "Icon" property
    • Implement similar logic in QML, everything is available in PlasmaCore.IconItem
    • Use icon name first, then pixmap (align to specification: "Visualizations are encouraged to prefer icon names over icon pixmaps if both are available")

      Then, is a separate diff, remove unused properties/logic from StatusNotifierItemSource (in Plasma 6?).

      This ensures backward compatibility. I'm pretty sure StatusNotifierItemSource is not used outside of Plasma, but if it this is part of the public API then needs to stay for now. Presentation logic is moved to correct layer. That should simplify it a little bit, I had hard time understanding it for the first time.

Thanks for the comprehensive write up! I fully agree with you!