D28208: Move sni icon handling logic from data engine to applet

[Konrad Materka]

kmaterka added inline comments.

INLINE COMMENTS

> StatusNotifierItem.qml:58
>      }
> +    // IconItem.overlays only supports names so we need a second item for the overlay, using the same
> +    // positioning that KIconLoader::drawOverlays uses that IconItem uses internally

Should overlay pixmaps be supported in `PlasmaCore.IconItem`? I guess that would require changes in `KIconLoader` as well.

> StatusNotifierItem.qml:72
> +        width: {
> +            if (iconItem.width < 32) {
> +                return 8;

In StatusNotifierItemSource::overlayIcon() <https://phabricator.kde.org/source/plasma-workspace/browse/master/dataengines/statusnotifieritem/statusnotifieritemsource.cpp$428> it is "width <= 32". There is a pre-existing inconsistency between StatusNotifierItemSource and IconItem/KIconLoader.

> davidre wrote in StatusNotifierItem.qml:84
> 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.

StatusNotifierItemSource::overlayIcon() <https://phabricator.kde.org/source/plasma-workspace/browse/master/dataengines/statusnotifieritem/statusnotifieritemsource.cpp$428> 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;
  }

> ngraham wrote in StatusNotifierItem.qml:90
> always round when multiplying by a non-integer value

In StatusNotifierItemSource::overlayIcon() <https://phabricator.kde.org/source/plasma-workspace/browse/master/dataengines/statusnotifieritem/statusnotifieritemsource.cpp$428> 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.

> systemtraymodel.h:97
>          IconThemePath,
>          IconsChanged,
>          Id,

You can safely remove all *Changed as well. Or maybe better in a separate commit?

> statusnotifieritemsource.cpp:90
>      //by setting everything up-front so that we have all role names when we call the first checkForUpdate()
> +    // TODO Plasma 6 remove combined "*Icon" properties and only expose the raw "*IconName" and "*IconPixmap" properties
>      setData(QStringLiteral("AttentionIcon"), QIcon());

I would suggest to extend removal to all *Changed roles. These are not used (were in KDE/Plasma 4).

REPOSITORY
  R120 Plasma Workspace

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

To: davidre, kmaterka, broulik, mart, #plasma, #vdg
Cc: ngraham, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, 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/20200322/e4a4e080/attachment-0001.html>