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/
Details
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 24166 Build 24184: arc lint + arc unit
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.
| applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml | ||
|---|---|---|
| 84 | Good to know! That was a straight copy from https://cgit.kde.org/kiconthemes.git/tree/src/kiconloader.cpp#n478 :D | |
| applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml | ||
|---|---|---|
| 58 | Should overlay pixmaps be supported in PlasmaCore.IconItem? I guess that would require changes in KIconLoader as well. | |
| 72 | In StatusNotifierItemSource::overlayIcon() it is "width <= 32". There is a pre-existing inconsistency between StatusNotifierItemSource and IconItem/KIconLoader. | |
| 84 | StatusNotifierItemSource::overlayIcon() also has some sizes hard coded. I quickly checked, looks like the values are the same. if (iconItem.width <= units.iconSize.medium) {
return units.iconSize.small / 2;
}
if (iconItem.width <= units.iconSize.large) {
return units.iconSize.small;
} | |
| 90 | 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). | |
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
| applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml | ||
|---|---|---|
| 90 ↗ | (On Diff #78298) | 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 ↗ | (On Diff #78298) | I will do it in a seperate commit. |
| applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml | ||
|---|---|---|
| 90 ↗ | (On Diff #78298) | Yes, you are correct, my mistake. So StatusNotifierItemSource has no margin and KIconLoader has 5% of icon size - another inconsistency. |
| applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml | ||
|---|---|---|
| 90 ↗ | (On Diff #78298) | 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. |
| applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml | ||
|---|---|---|
| 90 ↗ | (On Diff #78298) | The 0.05 * iconSize in KIconLoader has an implicit math.floor by the int conversion. |
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. | |
| 61 | add parent: iconItem.parent | |
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
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()).
- 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 ↗ | (On Diff #78298) | 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 ↗ | (On Diff #78298) | 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 ↗ | (On Diff #78298) | Beside formatting (spaces, minor thing), I liked the logic before - the presentation layer decided what to render and how, model just provided data. |
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.
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 :)