D24865: [SystemTray] Support for AttentionIcon

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> broulik wrote in systemtray.cpp:354
> 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`

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.

> broulik wrote in systemtray.cpp:358
> I am not sure you should check for `name` being empty, since it could have just pixmaps inside, not a themed icon?

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 <https://www.freedesktop.org/wiki/Specifications/StatusNotifierItem/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.

REPOSITORY
  R120 Plasma Workspace

REVISION DETAIL
  https://phabricator.kde.org/D24865

To: kmaterka, #plasma, broulik, #plasma_workspaces
Cc: lbeltrame, plasma-devel, LeGast00n, The-Feren-OS-Dev, jraleigh, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, alexeymin, himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20191029/ef50d4ba/attachment-0001.html>