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 ?

In D28107#629446, @anthonyfieroni wrote:

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

Of course thats much easier! Thank you!

In D28107#629521, @kmaterka wrote:

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

In D28107#630144, @davidre wrote:

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:

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.

In D28107#631804, @kmaterka wrote:

In D28107#630144, @davidre wrote:

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 property	In specification	Simple/Complex	DataSource property	Comments
Category	Yes	Simple	Category	Direct copy
Id	Yes	Simple	Id	Direct copy
Title	Yes	Simple	Title	Direct copy
Status	Yes	Simple	Status	Direct copy
WindowId	Yes	Simple	WindowId	Direct copy
ItemIsMenu	Yes	Simple	ItemIsMenu	Direct copy
AttentionMovieName	Yes	Simple	AttentionMovieName	Direct copy
ToolTip	Yes	Complex	ToolTipTitle, ToolTipSubTitle, ToolTipIcon	Converted to separate properties, additional logic
Menu	Yes	Complex	-	Converted to QMenu, special handling
IconThemePath	No!	Simple	IconThemePath	Direct copy
IconName	Yes	Simple	IconName	Only if IconPixmap is empty
IconPixmap	Yes	Complex	Not available	Used as part of Icon
OverlayIconName	Yes	Simple	OverlayIconName	Only if OverlayIconPixmap is empty
OverlayIconPixmap	Yes	Complex	Not available	Used as part of Icon
-	-	Simple	Icon	Complicated logic to create it from all Icon properties
AttentionIconName	Yes	Simple	AttentionIconName	Only if AttentionIconPixmap is empty
AttentionIconPixmap	Yes	Not available	Used as part of AttentionIcon
-	-	Simple	AttentionIcon	Complicated 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!