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

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

Details

Reviewers
kmaterka
broulik
mart
Group Reviewers
Plasma
VDG
Frameworks
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.Mar 22 2020, 8:48 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 22 2020, 8:48 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
davidre requested review of this revision.Mar 22 2020, 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.Mar 22 2020, 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.Mar 22 2020, 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)Mar 23 2020, 4:12 PM
davidre updated this revision to Diff 78298.Mar 23 2020, 5:15 PM

Use units iconsizes

davidre marked an inline comment as done.Mar 23 2020, 5:16 PM
davidre marked an inline comment as done.Mar 23 2020, 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.Mar 23 2020, 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.Mar 23 2020, 5:26 PM
davidre marked an inline comment as done and an inline comment as not done.
kmaterka added inline comments.Mar 23 2020, 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.Mar 23 2020, 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.Mar 23 2020, 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.Mar 24 2020, 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.Mar 25 2020, 4:40 PM
davidre marked an inline comment as done.

rebase

davidre updated this revision to Diff 78481.Mar 25 2020, 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()).

davidre updated this revision to Diff 78852.Mar 30 2020, 8:19 AM
  • 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.
applets/systemtray/systemtraymodel.cpp
332

This works now, but I don't know how to manage the KIconLoader. The KIconEngines are managed by QIcon (they take ownership of the engine). One idea would be to add it to the item maybe?

The best would be to add theme path support to PlasmaCore.IconItem. I will take a look at this tomorrow.

applets/systemtray/systemtraymodel.cpp
332

I don't like this idea. Now it is just a copy from one model to another without much of improvements - in fact now it complicates it even more. KIconLoader is already created in StatusNotifierItemSource, it has little justification to create another one in data layer.

340

Beside formatting (spaces, minor thing), I liked the logic before - the presentation layer decided what to render and how, model just provided data.
In addition, *Icon role can have two different types now - it should work with dynamicRoles enabled, but sometimes it has unexpected consequences. I had a bad experience with dynamic data type changes.

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.

davidre updated this revision to Diff 78982.Mar 31 2020, 12:29 PM
  • Revert "Also handle IconThemePath"

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 complex structure like hicolor/22x22/apps/.... This is also code/logic duplication.

Or if all else fails for now use the old icon property in case IconThemePath is set.

As a temporary workaround - yes. Final goal is to add custom icon loader support to PlasmaCore.IconItem. I checked code, I see great potential for additional refactoring there :)

I looked at the IconItem implementation. Before add theme directory support I would like to perform some refactoring - D28470.

@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 :)

davidre updated this revision to Diff 82492.May 11 2020, 7:44 AM
  • Rebase without test