D28107: Fix overlayIcon sometimes not visible

2020-04-07 Thread David Redondo
davidre abandoned this revision.
davidre added a comment.


  See D28208  for making things less 
complicated

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


D28107: Fix overlayIcon sometimes not visible

2020-03-21 Thread David Redondo
davidre added a comment.


  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 

D28107: Fix overlayIcon sometimes not visible

2020-03-21 Thread Konrad Materka
kmaterka added a comment.


  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 

D28107: Fix overlayIcon sometimes not visible

2020-03-18 Thread David Redondo
davidre added a comment.


  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

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


D28107: Fix overlayIcon sometimes not visible

2020-03-17 Thread David Redondo
davidre added a comment.


  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

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


D28107: Fix overlayIcon sometimes not visible

2020-03-17 Thread Konrad Materka
kmaterka requested changes to this revision.
kmaterka added a comment.
This revision now requires changes to proceed.


  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?

INLINE COMMENTS

> systemtraymodel.cpp:321
> + */
> +if(!icon.value().name().isEmpty()) {
> +updateItemData(dataItem, data, Role::OverlayIconName);

code formatting (missing space after if)

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


D28107: Fix overlayIcon sometimes not visible

2020-03-17 Thread David Redondo
davidre updated this revision to Diff 77866.
davidre added a comment.


  Only set the role conditionally

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D28107?vs=77853=77866

BRANCH
  workaround (branched from master)

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

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
  applets/systemtray/systemtraymodel.cpp

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


D28107: Fix overlayIcon sometimes not visible

2020-03-17 Thread David Redondo
davidre added a comment.


  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!

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


D28107: Fix overlayIcon sometimes not visible

2020-03-17 Thread Anthony Fieroni
anthonyfieroni added a comment.


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

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


D28107: Fix overlayIcon sometimes not visible

2020-03-17 Thread David Redondo
davidre added a comment.


  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.

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, kmaterka, #plasma, broulik, davidedmundson
Cc: 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


D28107: Fix overlayIcon sometimes not visible

2020-03-17 Thread David Redondo
davidre created this revision.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
davidre requested review of this revision.

REVISION 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.

REPOSITORY
  R120 Plasma Workspace

BRANCH
  workaround (branched from master)

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

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
  applets/systemtray/systemtraymodel.cpp
  applets/systemtray/systemtraymodel.h

To: davidre
Cc: 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