Adding support for Attention Icon to StatusNotifier tray icons.
Binding is working correctly only when "Status" is used, not derived "status".
BUG: 341255
broulik | |
davidedmundson |
Plasma | |
Plasma: Workspaces |
Adding support for Attention Icon to StatusNotifier tray icons.
Binding is working correctly only when "Status" is used, not derived "status".
BUG: 341255
As described in bug report, Konversation can be used for tests.
No Linters Available |
No Unit Test Coverage |
Buildable 18521 | |
Build 18539: arc lint + arc unit |
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 will switch to enum, as I intended, IMO with enum it looks cleaner (but longer)
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 used", doesn't state it's mandatory - can be a separate patch)
I need to add one more check: when AttentionIcon is not set, it should fallback to Icon(Name)
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 AttentionIcon || AttentionIconName; } return Icon || IconName; }
The problem is that StatusNotifierItemSource always puts empty values for all elements. This is needed to initialize all roleNames (for QML delegate model etc). Anyway, it is always at least: "QVariant(QIcon, QIcon(null))".
In the situation when IconName was passed and not IconPixmap the check:
source: Icon ? Icon : IconName
worked because when there was no IconPixmap (="Icon"), then in StatusNotifierItemSource::refreshCallback Icon was loaded from IconName and replaced empty "Icon". In other works, this check was not needed in QML.
applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml | ||
---|---|---|
52 | I know... Do you know another way how to check if QVariant(QIcon) is null? I know that I can create simple native method in systemtray.cpp for that - will it be better? |
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?
applets/systemtray/systemtray.cpp | ||
---|---|---|
354 ↗ | (On Diff #68598) | 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 |
358 ↗ | (On Diff #68598) | I am not sure you should check for name being empty, since it could have just pixmaps inside, not a themed icon? |
applets/systemtray/systemtray.cpp | ||
---|---|---|
354 ↗ | (On Diff #68598) | You are of course correct, check will work without this. I wanted to be explicit here, I don't like to rely on implicit behavior. If QVariant is not QIcon type it is clear that it is not a valid icon. But if I convert it anyway, I need to know that it will create empty/null icon (default constructor). Documentation suggests to use canConvert: https://doc.qt.io/qt-5/qvariant.html#value |
358 ↗ | (On Diff #68598) | Yeah, this is tricky, probably requires a few words of comment. // If the QIcon was created with QIcon::fromTheme(), try to load it as svg if (source.canConvert<QIcon>() && !source.value<QIcon>().name().isEmpty()) { sourceString = source.value<QIcon>().name(); } if (!sourceString.isEmpty()) { ... } else if (source.canConvert<QIcon>()) { m_icon = source.value<QIcon>(); ... } So even if there is no pixmap assigned (is it even possible? does make sense?) the icon is considered valid. The specification says that "IconName" should be preferred over the "Icon" pixmap: freedesktop::StatusNotifierItem As a side note, there a two way to improve it:
Anyway, the situation is quite messy. |
Alternative below. IMHO it might be cleaner.
Otherwise, this is fine.
applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml | ||
---|---|---|
52 | The easiest alternative is to do it earlier in the source itself. Instead of supplying a valid QVariant with a null icon, we can export a null QVariant. i.e statusnotifiericonsource.cpp Then the regular QML tests will work just fine. |
I will do more tests. The initialization
applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml | ||
---|---|---|
59 | I had few worries about this approach but I tested with several combinations. Additional comment for anyone wondering how it is working. setData(QStringLiteral("AttentionIcon"), QIcon()); setData(QStringLiteral("Icon"), QIcon()); It is required to setup role names - invalid QVariant() removes role name (check DataContainer::setData). All role names must be initialized, if not QML won't recognize it later. Fortunately, on any update all fields, including icons, are updated. Setting QVariant() will remove value entirely. In fact, it removes the role name, but as roles are cached, it is not making any harm. |