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
