D28208: Move sni icon handling logic from data engine to applet

davidre updated this revision to Diff 82519. davidre added a comment. - fix REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28208?vs=82492=82519 BRANCH sni (branched from master) REVISION DETAIL https://phabricator.kde.org/D28208 AFFECTED

D28208: Move sni icon handling logic from data engine to applet

davidre updated this revision to Diff 82492. davidre added a comment. - Rebase without test REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28208?vs=78982=82492 BRANCH sni (branched from master) REVISION DETAIL

D28208: Move sni icon handling logic from data engine to applet

kmaterka added a comment. @davidre can you extract your test improvement to a separate patch? it would help me in another issue and I don't want to "steal" your code :) REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D28208 To: davidre, kmaterka, broulik,

D28208: Move sni icon handling logic from data engine to applet

kmaterka added a comment. I looked at the `IconItem` implementation. Before add theme directory support I would like to perform some refactoring - D28470 . REPOSITORY R120 Plasma Workspace REVISION DETAIL https://phabricator.kde.org/D28208 To:

D28208: Move sni icon handling logic from data engine to applet

kmaterka added a comment. In D28208#638846 , @davidre wrote: > Yes it's not pretty but I had no other Idea. One option would be trying to manually search for the icon in the path? Not a good idea, theme path only point to root of more

D28208: Move sni icon handling logic from data engine to applet

davidre updated this revision to Diff 78982. davidre added a comment. - Revert "Also handle IconThemePath" REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28208?vs=78852=78982 BRANCH sni (branched from master) REVISION DETAIL

D28208: Move sni icon handling logic from data engine to applet

davidre added a comment. Yes it's not pretty but I had no other Idea. One option would be trying to manually search for the icon in the path? Or if all else fails for now use the old icon property in case `IconThemePath` is set. REPOSITORY R120 Plasma Workspace REVISION DETAIL

D28208: Move sni icon handling logic from data engine to applet

kmaterka added a comment. The best would be to add theme path support to `PlasmaCore.IconItem`. I will take a look at this tomorrow. INLINE COMMENTS > davidre wrote in systemtraymodel.cpp:331 > This works now, but I don't know how to manage the KIconLoader. The > KIconEngines are managed

D28208: Move sni icon handling logic from data engine to applet

davidre added a comment. INLINE COMMENTS > systemtraymodel.cpp:331 > +if (!iconThemePath.isEmpty()) { > + loader = new KIconLoader(sourceName, {iconThemePath}); > + loader->addAppDir(sourceName, iconThemePath); This works now, but I don't know how to manage the

D28208: Move sni icon handling logic from data engine to applet

davidre updated this revision to Diff 78852. davidre added a comment. - Also handle IconThemePath We need the KIconEngine thing like the dataengine did. To make it as not as complicated I moved the "name ? name : pixmap" into the model. REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST

D28208: Move sni icon handling logic from data engine to applet

kmaterka added a comment. In D28208#634593 , @davidre wrote: > Do you know if `IconThemePath` is specified somewhere? Initial commit: https://github.com/KDE/kde-workspace/commit/4c553dbf39f4189290387bba35589200ca051084 Someone

D28208: Move sni icon handling logic from data engine to applet

kmaterka added a comment. In D28208#634593 , @davidre wrote: > Do you know if `IconThemePath` is specified somewhere? Initial commit: What I could observe is that some apps create a folder hierachy under this path and palce their

D28208: Move sni icon handling logic from data engine to applet

davidre added a comment. Do you know if `IconThemePath` is specified somewhere? What I could observe is that some apps create a folder hierachy under this path and palce their icon there. I currently don't have any idea how to handle this in qml. Maybe we need to do this in the model / c++

D28208: Move sni icon handling logic from data engine to applet

davidre updated this revision to Diff 78481. davidre marked an inline comment as done. davidre added a comment. Also change it in the tooltip icon REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28208?vs=78479=78481 BRANCH sni (branched from

D28208: Move sni icon handling logic from data engine to applet

davidre updated this revision to Diff 78479. davidre marked an inline comment as done. davidre added a comment. rebase REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28208?vs=78384=78479 BRANCH sni (branched from master) REVISION DETAIL

D28208: Move sni icon handling logic from data engine to applet

kmaterka added a comment. > Some of the icons are not rendering now, for example: > Discord - iconName: chrome_app_indicator2_83406baa15c6e4f2766a6e3714fbee20 > Skype1 - iconName: chrome_app_indicator2_01cdeaaa54803d9c9c158a5cdcb1fbfb > I'm pretty sure I saw a workaround for that

D28208: Move sni icon handling logic from data engine to applet

kmaterka added a comment. Two problems: After running `plasmashell --replace &` I have these messages in the console output: > file:///home/konrad/kde/usr/share/plasma/plasmoids/org.kde.plasma.private.systemtray/contents/ui/items/StatusNotifierItem.qml:57:5: QML IconItem: Cannot

D28208: Move sni icon handling logic from data engine to applet

davidre updated this revision to Diff 78384. davidre marked 6 inline comments as done. davidre added a comment. Use Math.floor REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28208?vs=78298=78384 BRANCH sni (branched from master) REVISION

D28208: Move sni icon handling logic from data engine to applet

bruns added inline comments. INLINE COMMENTS > kmaterka wrote in StatusNotifierItem.qml:90 > Yes, you are correct, my mistake. So `StatusNotifierItemSource` has no margin > and `KIconLoader` has 5% of icon size - another inconsistency. > What about 'always round when multiplying by a

D28208: Move sni icon handling logic from data engine to applet

ngraham added inline comments. INLINE COMMENTS > kmaterka wrote in StatusNotifierItem.qml:90 > Yes, you are correct, my mistake. So `StatusNotifierItemSource` has no margin > and `KIconLoader` has 5% of icon size - another inconsistency. > What about 'always round when multiplying by a

D28208: Move sni icon handling logic from data engine to applet

kmaterka added inline comments. INLINE COMMENTS > davidre wrote in StatusNotifierItem.qml:90 > Hmm to me it looks it doesn't even have an margin. Or am I reading ` > p.drawPixmap(QRect(m_iconPixmap.width()-size, m_iconPixmap.height()-size, > size, size), overlay->pixmap(size, size),

D28208: Move sni icon handling logic from data engine to applet

davidre marked 3 inline comments as done and an inline comment as not done. davidre added inline comments. INLINE COMMENTS > StatusNotifierItem.qml:90 > +bottom: iconItem.bottom > +bottomMargin: 0.05 * iconItem.width > +rightMargin: anchors.bottomMargin Hmm

D28208: Move sni icon handling logic from data engine to applet

davidre marked an inline comment as done. davidre added a comment. I think for now we should just recreate the old behaviour from statusnotifieritemsource as this commit is about moving the icon handling. And in a follow up commit we can then change it REPOSITORY R120 Plasma Workspace

D28208: Move sni icon handling logic from data engine to applet

davidre updated this revision to Diff 78298. davidre added a comment. Use units iconsizes REPOSITORY R120 Plasma Workspace CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D28208?vs=78253=78298 BRANCH sni (branched from master) REVISION DETAIL

D28208: Move sni icon handling logic from data engine to applet

kmaterka added inline comments. INLINE COMMENTS > StatusNotifierItem.qml:58 > } > +// IconItem.overlays only supports names so we need a second item for > the overlay, using the same > +// positioning that KIconLoader::drawOverlays uses that IconItem uses > internally Should

D28208: Move sni icon handling logic from data engine to applet

davidre added inline comments. INLINE COMMENTS > StatusNotifierItem.qml:84 > +} > +return 64; > +} Good to know! That was a straight copy from https://cgit.kde.org/kiconthemes.git/tree/src/kiconloader.cpp#n478 :D I'm sure we can find better sizing though, I

D28208: Move sni icon handling logic from data engine to applet

ngraham added inline comments. INLINE COMMENTS > StatusNotifierItem.qml:84 > +} > +return 64; > +} Most of these numbers correspond to standard icon sizes (e.g. `units.iconSize.smallMedium` == 22); maybe use those where possible > StatusNotifierItem.qml:90 > +

D28208: Move sni icon handling logic from data engine to applet

davidre added a comment. For now I tried to replicate `KIconLoader::drawOverlays` that is used in IconItem but that results in very small overlays. Because of this I added #vdg for opinions on better sizing. REPOSITORY R120 Plasma Workspace REVISION

D28208: Move sni icon handling logic from data engine to applet

davidre created this revision. davidre added reviewers: kmaterka, broulik, mart, Plasma, VDG. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. davidre requested review of this revision. REVISION SUMMARY The engine does complicated logig in order to provide a pre rendered