D28107: Fix overlayIcon sometimes not visible

[Konrad Materka]

kmaterka added a comment.


  In D28107#630144 <https://phabricator.kde.org/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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D28107

To: davidre, kmaterka, #plasma, broulik, davidedmundson
Cc: anthonyfieroni, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20200321/98de169b/attachment-0001.html>