Move sni icon handling logic from data engine to applet
Needs ReviewPublic

Authored by davidre on Sun, Mar 22, 8:48 PM.

Details

Reviewers
kmaterka
broulik
mart
Group Reviewers
Plasma
VDG
Summary

The engine does complicated logic in order to provide a pre rendered icon.
However in combination with IconItem this also resulted in bugs causing
overlay icons to effectively not work (correctly) [1, 2]. This exposes the name and pixmap
properties in the data engine as in the specification [3]. Displaying of the data
is now done at the correct layer. The statusnotifertest is additionally extended
to make testing of all combinations of icon properties easier. For now the old
combined properties are kept for backwards compatibility but can be removed in
a later commit or in Plasma 6.
[1] https://phabricator.kde.org/D28107
[2] https://phabricator.kde.org/D27617#630440
[3] https://www.freedesktop.org/wiki/Specifications/StatusNotifierItem/StatusNotifierItem/

Test Plan

use statusnotifiertest

Diff Detail

Repository
R120 Plasma Workspace
Branch
sni (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24223
Build 24241: arc lint + arc unit
davidre created this revision.Sun, Mar 22, 8:48 PM
Restricted Application added a project: Plasma. · View Herald TranscriptSun, Mar 22, 8:48 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidre requested review of this revision.Sun, Mar 22, 8:48 PM

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.

ngraham added inline comments.
applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
77

Most of these numbers correspond to standard icon sizes (e.g. units.iconSize.smallMedium == 22); maybe use those where possible

83

always round when multiplying by a non-integer value

davidre added inline comments.Sun, Mar 22, 8:58 PM
applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
77

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 think this is to small, for sure.

kmaterka added inline comments.Sun, Mar 22, 9:49 PM
applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
51

Should overlay pixmaps be supported in PlasmaCore.IconItem? I guess that would require changes in KIconLoader as well.

65

In StatusNotifierItemSource::overlayIcon() it is "width <= 32". There is a pre-existing inconsistency between StatusNotifierItemSource and IconItem/KIconLoader.

77

StatusNotifierItemSource::overlayIcon() also has some sizes hard coded. I quickly checked, looks like the values are the same.
You can use the same syntax here:

if (iconItem.width <= units.iconSize.medium) {
    return units.iconSize.small / 2;
}
if (iconItem.width <= units.iconSize.large) {
    return units.iconSize.small;
}
83

In StatusNotifierItemSource::overlayIcon() it has a different logic to position overlay. Margin is always the size of overlay icon. Another pre-existing inconsistency. Which one to choose? I think that KIconLoader is more important.

applets/systemtray/systemtraymodel.h
97

You can safely remove all *Changed as well. Or maybe better in a separate commit?

dataengines/statusnotifieritem/statusnotifieritemsource.cpp
90

I would suggest to extend removal to all *Changed roles. These are not used (were in KDE/Plasma 4).

davidre edited the summary of this revision. (Show Details)Mon, Mar 23, 4:12 PM
davidre updated this revision to Diff 78298.Mon, Mar 23, 5:15 PM

Use units iconsizes

davidre marked an inline comment as done.Mon, Mar 23, 5:16 PM
davidre marked an inline comment as done.Mon, Mar 23, 5:20 PM

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

davidre marked 3 inline comments as done and an inline comment as not done.Mon, Mar 23, 5:25 PM
davidre added inline comments.
applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
83

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), QRect(0,0,size,size)); wrong?

applets/systemtray/systemtraymodel.h
97

I will do it in a seperate commit.

davidre marked an inline comment as not done.Mon, Mar 23, 5:26 PM
davidre marked an inline comment as done and an inline comment as not done.
kmaterka added inline comments.Mon, Mar 23, 8:45 PM
applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
83

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 non-integer value'? I don't know exactly why, but AFAIK not-rounded number can cause inconsistent behavior in QML.
@ngraham can you give a word of explanation?

ngraham added inline comments.Mon, Mar 23, 10:59 PM
applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
83

Yes, when you have a positional value that isn't an integer, the engine can end up positioning the item on half-pixels and everything looks blurry.

bruns added a subscriber: bruns.Mon, Mar 23, 11:26 PM
bruns added inline comments.
applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
83

The 0.05 * iconSize in KIconLoader has an implicit math.floor by the int conversion.

davidre updated this revision to Diff 78384.Tue, Mar 24, 5:23 PM
davidre marked 6 inline comments as done.

Use Math.floor

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 anchor to an item that isn't a parent or sibling.
file:///home/konrad/kde/usr/share/plasma/plasmoids/org.kde.plasma.private.systemtray/contents/ui/items/StatusNotifierItem.qml:57:5: QML IconItem: Cannot anchor to an item that isn't a parent or sibling.

You need to rebase, in latest change parent of the icon changed to parent: taskIcon.iconContainer, overlay icon should have the same parent:

id: overlayIconItem
parent: iconItem.parent

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 somewhere, but I can't recall where...

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 somewhere, but I can't recall where...

Found it! You need to handle IconThemePath, which can be difficult... :/

applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
30

You need to change here as well.

54

add parent: iconItem.parent

davidre updated this revision to Diff 78479.Wed, Mar 25, 4:40 PM
davidre marked an inline comment as done.

rebase

davidre updated this revision to Diff 78481.Wed, Mar 25, 4:43 PM
davidre marked an inline comment as done.

Also change it in the tooltip icon

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++ side again?
For example
Arguments: [Variant(QString): "/tmp/.org.chromium.Chromium.3WvReV/icons"] as IconThemepath
and inside is /tmp/.org.chromium.Chromium.3WvReV/icons/hicolor/22x22/apps/chrome_app_indicator2_91c0c89ea21003eb35a74c9caa4718cc.png

This comment was removed by kmaterka.

Do you know if IconThemePath is specified somewhere?

Initial commit:
https://github.com/KDE/kde-workspace/commit/4c553dbf39f4189290387bba35589200ca051084
Someone mentioned this property here:
https://lists.freedesktop.org/archives/xdg/2015-December/013620.html
https://mail.kde.org/pipermail/plasma-devel/2015-December/047019.html
But he tried to create new specification. What we need is to update existing one to reflect all the changes...

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++ side again?

Currently it is implemented in statusnotifieritemsource.cpp, it reconfigures KIconLoder to use themePath as additional icon location.
The best would be to have a similar implementation in PlasmaCore.IconItem, but this is a lot to change. IconItem is using global instance only KIconLoader::global(), while StatusNotifierItemSource has custom one (when needed) and fallbacks to the global one (StatusNotifierItemSource::iconLoader()).