[SystemTray] Support for AttentionIcon
ClosedPublic

Authored by kmaterka on Oct 22 2019, 12:51 PM.

Details

Summary

Adding support for Attention Icon to StatusNotifier tray icons.
Binding is working correctly only when "Status" is used, not derived "status".

BUG: 341255

Test Plan

As described in bug report, Konversation can be used for tests.

Diff Detail

Repository
R120 Plasma Workspace
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18521
Build 18539: arc lint + arc unit
kmaterka created this revision.Oct 22 2019, 12:51 PM
Restricted Application added a project: Plasma. · View Herald TranscriptOct 22 2019, 12:51 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
kmaterka requested review of this revision.Oct 22 2019, 12:51 PM

PlasmaCore.IconItem has a status property, so you probably need to explicitly check taskIcon.status then it should work with the proper enum

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)

kmaterka updated this revision to Diff 68543.Oct 22 2019, 12:59 PM

Use enum for status check

Tested, works. +1.

broulik accepted this revision.Oct 22 2019, 1:02 PM

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)

This revision is now accepted and ready to land.Oct 22 2019, 1:02 PM

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;
}

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.

kmaterka updated this revision to Diff 68566.Oct 22 2019, 7:10 PM

Fallback to Icon if there is no AttentionIcon.

kmaterka added inline comments.Oct 22 2019, 7:13 PM
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?

kmaterka requested review of this revision.Oct 22 2019, 7:15 PM

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?

kmaterka updated this revision to Diff 68598.Oct 23 2019, 11:49 AM

Get rid of ugly and ridiculous nullness check.

+1 to my untrained eye.

broulik added inline comments.Oct 29 2019, 8:57 AM
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?

kmaterka added inline comments.Oct 29 2019, 10:16 AM
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
value.isValid() || value.isNull() is added for the same reason, to be explicit.

358 ↗(On Diff #68598)

Yeah, this is tricky, probably requires a few words of comment.
I'm checking if there is a name OR a pixmap assigned to be in parity with IconItem implementation. This code is from PlasmaCore.IconItem::setSource():

// 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 "Icon" is not just a pixmap passed from the client app, it is enhanced in the "StatusNotifierItemSource". Even if there is only "IconName" provided, StatusNotifierItemSource loads the icon from "IconName" (using KIconEngine), applies OverlayIcon* (if available) and passes it as "Icon".

The specification says that "IconName" should be preferred over the "Icon" pixmap: freedesktop::StatusNotifierItem
"StatusNotifierItemSource" loads icon from "IconName" (using KIconEngine) into "Icon" and then applies OverlayIcon* (if available).
StatusNotifierItemSource takes IconName as a first source, but then in the presentation view (QML) it is the opposite - only "Icon" is useful.

As a side note, there a two way to improve it:

  1. simplify the model and move all logic to presentation view, so: use "IconName" as a primary source, not "Icon" and apply any overlay icons in the presentation view (QML).
  2. get rid of the "IconName" entirely, because it is already handled in the model (StatusNotifierItemSource). PlasmaCore.IconItem is probably don't needed as well, theme is (?) already handled in the model.

Anyway, the situation is quite messy.

davidedmundson accepted this revision.Nov 5 2019, 12:18 PM

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
90: setData(QStringLiteral("AttentionIcon"), QVariant());
332: QStringLiteral("AttentionIcon"), attentionIcon.isNull() ? QVariant(), attentionIcon);

Then the regular QML tests will work just fine.

This revision is now accepted and ready to land.Nov 5 2019, 12:18 PM
kmaterka marked an inline comment as done.Nov 5 2019, 12:53 PM

Alternative below. IMHO it might be cleaner.

Otherwise, this is fine.

I will check this.

kmaterka updated this revision to Diff 69321.EditedNov 5 2019, 1:44 PM
kmaterka marked an inline comment as done.

Simplify the check, thanks for a tip!

davidedmundson accepted this revision.Nov 5 2019, 1:46 PM
davidedmundson added inline comments.
applets/systemtray/package/contents/ui/items/StatusNotifierItem.qml
59

Do we need the same thing for this check?

59

Edit. ignore me.

kmaterka marked 2 inline comments as done.Nov 5 2019, 1:54 PM

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.
The initialization needs to set empty icon:

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.
QML will see valid icon or "undefined" (if invalid QVariant() passed).

This revision was automatically updated to reflect the committed changes.
kmaterka marked an inline comment as done.