D24865: [SystemTray] Support for AttentionIcon

kmaterka marked 2 inline comments as done. kmaterka added a comment. I will do more tests. The initialization INLINE COMMENTS > davidedmundson wrote in StatusNotifierItem.qml:59 > Edit. ignore me. I had few worries about this approach but I tested with several combinations. Additional comme

D24865: [SystemTray] Support for AttentionIcon

This revision was automatically updated to reflect the committed changes. kmaterka marked an inline comment as done. Closed by commit R120:2d7b6c2324c3: [SystemTray] Support for AttentionIcon (authored by kmaterka). REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricat

D24865: [SystemTray] Support for AttentionIcon

davidedmundson added inline comments. INLINE COMMENTS > davidedmundson wrote in StatusNotifierItem.qml:59 > Do we need the same thing for this check? Edit. ignore me. REPOSITORY R120 Plasma Workspace BRANCH master REVISION DETAIL https://phabricator.kde.org/D24865 To: kmaterka, #plasma

D24865: [SystemTray] Support for AttentionIcon

davidedmundson added inline comments. INLINE COMMENTS > StatusNotifierItem.qml:59 > +} > +return Icon ? Icon : IconName > +} Do we need the same thing for this check? REPOSITORY R120 Plasma Workspace BRANCH master REVISION DETAIL https://phabricator.kde.o

D24865: [SystemTray] Support for AttentionIcon

kmaterka updated this revision to Diff 69321. kmaterka marked an inline comment as done. kmaterka added a comment. Simply the check, thanks for a tip! REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24865?vs=68598&id=69321 BRANCH master REVISION

D24865: [SystemTray] Support for AttentionIcon

kmaterka marked an inline comment as done. kmaterka added a comment. In D24865#558933 , @davidedmundson wrote: > Alternative below. IMHO it might be cleaner. > > Otherwise, this is fine. I will check this. REPOSITORY R120 Plasma Wo

D24865: [SystemTray] Support for AttentionIcon

davidedmundson accepted this revision. davidedmundson added a comment. This revision is now accepted and ready to land. Alternative below. IMHO it might be cleaner. Otherwise, this is fine. INLINE COMMENTS > kmaterka wrote in StatusNotifierItem.qml:52 > I know... Do you know another way

D24865: [SystemTray] Support for AttentionIcon

kmaterka added inline comments. INLINE COMMENTS > broulik wrote in systemtray.cpp:354 > I think you don't need any of that. If you convert it to a `QIcon` which it > is not, it will return a default-constructed (null) `QIcon` You are of course correct, check will work without this. I wanted to

D24865: [SystemTray] Support for AttentionIcon

broulik added inline comments. INLINE COMMENTS > systemtray.cpp:354 > +{ > +if (!value.isValid() || value.isNull() || !value.canConvert()) { > +return false; I think you don't need any of that. If you convert it to a `QIcon` which it is not, it will return a default-constructed (nul

D24865: [SystemTray] Support for AttentionIcon

lbeltrame added a comment. +1 to my untrained eye. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24865 To: kmaterka, #plasma, broulik, #plasma_workspaces Cc: lbeltrame, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, Zren

D24865: [SystemTray] Support for AttentionIcon

kmaterka updated this revision to Diff 68598. kmaterka added a comment. Get rid of ugly and ridiculous nullness check. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24865?vs=68566&id=68598 BRANCH master REVISION DETAIL https://phabricator.kd

D24865: [SystemTray] Support for AttentionIcon

kmaterka requested review of this revision. kmaterka added a comment. I added ugly way of checking if passed QVariant(QIcon) is null. Should I create native C++ method for that check or this can be done in QML, but in nicer way? REPOSITORY R120 Plasma Workspace REVISION DETAIL https://p

D24865: [SystemTray] Support for AttentionIcon

kmaterka added inline comments. INLINE COMMENTS > StatusNotifierItem.qml:52 > +if (taskIcon.status === PlasmaCore.Types.NeedsAttentionStatus) { > +if (AttentionIcon && AttentionIcon != "QVariant(QIcon, > QIcon(null))") { > +return AttentionIcon I

D24865: [SystemTray] Support for AttentionIcon

kmaterka updated this revision to Diff 68566. kmaterka added a comment. Fallback to Icon if there is no AttentionIcon. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24865?vs=68543&id=68566 BRANCH master REVISION DETAIL https://phabricator.kd

D24865: [SystemTray] Support for AttentionIcon

kmaterka added a comment. In D24865#552202 , @broulik wrote: > You might want to split that into a proper if statement otherwise it becomes somewhat hard to read. Something like > > source: { > if (taskIcon.status === PlasmaCore.Typ

D24865: [SystemTray] Support for AttentionIcon

broulik added a comment. You might want to split that into a proper if statement otherwise it becomes somewhat hard to read. Something like source: { if (taskIcon.status === PlasmaCore.Types.NeedsAttentionStatus && (AttentionIcon || AttentionIconName)) { return Atte

D24865: [SystemTray] Support for AttentionIcon

kmaterka added a comment. I need to add one more check: when AttentionIcon is not set, it should fallback to Icon(Name) REPOSITORY R120 Plasma Workspace BRANCH master REVISION DETAIL https://phabricator.kde.org/D24865 To: kmaterka, #plasma, broulik, #plasma_workspaces Cc: lbeltrame,

D24865: [SystemTray] Support for AttentionIcon

broulik accepted this revision. broulik added a comment. This revision is now accepted and ready to land. Thanks for following up! (Out of curiosity, do we not need to fall back to the regular icon in case it doesn't have an attention icon in needs attention state? Spec only says "can be

D24865: [SystemTray] Support for AttentionIcon

lbeltrame added a comment. Tested, works. +1. REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24865 To: kmaterka, #plasma, broulik, #plasma_workspaces Cc: lbeltrame, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot,

D24865: [SystemTray] Support for AttentionIcon

kmaterka updated this revision to Diff 68543. kmaterka added a comment. Use enum for status check REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24865?vs=68541&id=68543 BRANCH master REVISION DETAIL https://phabricator.kde.org/D24865 AFFECTE

D24865: [SystemTray] Support for AttentionIcon

kmaterka added a comment. In D24865#552191 , @broulik wrote: > `PlasmaCore.IconItem` has a `status` property, so you probably need to explicitly check `taskIcon.status` then it should work with the proper enum Oh! Now it looks obvious! I

D24865: [SystemTray] Support for AttentionIcon

broulik added a comment. `PlasmaCore.IconItem` has a `status` property, so you probably need to explicitly check `taskIcon.status` then it should work with the proper enum REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D24865 To: kmaterka, #plasma, broulik,

D24865: [SystemTray] Support for AttentionIcon

kmaterka created this revision. kmaterka added reviewers: Plasma, broulik. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. kmaterka requested review of this revision. REVISION SUMMARY Adding support for Attention Icon to StatusNotifier tray icons. Binding is working co