Done in plasma-workspace!836

If there are no objections, I can do that. Just let me know if this task is approved.

Any decision here? IMHO it should be internal to system tray, there is little to none value in having this as DataEngine. AFAIK system tray is the only consumer, having this in system tray should simplify the code. There are also some bugs not solved because SNI DataEngine is considered a public API which should not be change without good reason - then let's remove it, ultimate API breakage 😄

In Ubuntu/Debian, libdbusmenu-qt5-2 package is used not only by Plasma, but also by LXQt and Quassel.

Any update on this? I really like the idea.

In D29827#673720, @ngraham wrote:

In D29827#673615, @meven wrote:

In D29827#673611, @davidedmundson wrote:

It's an SNI, I thought the systemtray could already filter SNIs in the enties tab of the system tray?

Currently it appears in the systray config only when the microphone is activated.
I guess we should make it more permanent.

Yes, that seems like a better option. Then the existing config UI will be used for this, and can be used to disable other SNIs too.

It is not that easy. SNI is just a protocol for system tray icons. Currently it is not possible to disable SNI icon entirely from symtem tray settings - you can only hide it (to the hidden icons view). It is a responsibility of the application to decide if icon is needed or not and give user the option to disable the system tray icon (some applications have such option).
I'm not sure if having an option to disable SNI icon is a good idea. If you don't want the icon, do not start an app in the first place or hide it or ask author of an app for an option to disable tray icon.
In case of Microphone Indicator, it will run and take resources, it will try to create an icon and will take the resources. If we give users an option to disable SNI icons in system tray settings it may look like disabling the service entirely, which is not true.

Can we have it reviewed before beta 5.19 release?

Firstly, AttentionIcon support was introduced in D24865. It had a bug, fixed for 5.18 in D29386. When I merged branch 5.18 (with D29386) into master, by mistake I forgot about AttentionIcon handling... This change fixes the incomplete merge.

plasma-workspace compiles using cmake 3.10 without issues now.

In D29302#666138, @ahiemstra wrote:

Which version of CMake are you using? I needed to promote the imported KSysGuard targets to "global" using set_target_property. Without that, I saw similar errors. The IMPORTED_GLOBAL property was added in CMake 3.11.

With this change, plasma-workspace fails to build on my system (Neon User, kdesrc-build)

@davidre can you extract your test improvement to a separate patch? it would help me in another issue and I don't want to "steal" your code :)

Rebase

In D29386#663325, @ngraham wrote:

Great thanks! Don't forget to merge Plasma/5.18 into master, fixing the merge conflicts.

Additional check for real JavaScript null - to avoid conversion errors in native method.

In D29386#662960, @ngraham wrote:

Great catch. The logic seems sane to me. Note that if you plan to land this on the stable branch, there will be merge conflicts that you'll need to resolve carefully, since it looks like this code has changed a bit between 5.18 and 5.19.

Separate Bug 420993 created for Media Player icon not rendering correctly.

Change warning message

In D29344#662586, @ngraham wrote:

5.19 hasn't branched yet, so code landed in master will still make it there.

Maybe there was code freeze for frameworks? Or 5.18.5? Never mind :)

This patch is for "Plasma/5.18" branch only.
Can we have it merged before 5.18.5?

OK, accepted

The same treatment is needed in HiddenItemsView to. Just move z: -1 to CurrentItemHighLight component so that your fix will cover both cases.

Fix exctracted from D29344, to make things clear - one commit, one change.

Extract unrelated fix to a separate patch: D29356

This fix should go with Plasma 5.19, if it is not too late (code freeze).

Regression fixed in D29314.

In D28470#660492, @ngraham wrote:

git bisect says this caused https://bugs.kde.org/show_bug.cgi?id=420801.

IMO it is the best not to add new status in System Tray - it is already quite confusing with "Disabled" state, the 3 other (Always hidden, Shown when relevant and Always shown) are OK and self-explanatory.
"Shown when relevant" might be tricky and we will never please everyone. As stated in previous comment, this is tricky.

Rebase to master (includes D29102)

Any other comment? Is it OK and can be approved?

Do not inherit from QObject

In D28470#647757, @davidedmundson wrote:

Note there's a unit test for IconItem worth running if you haven't already.

I've checked that already, these test were really useful!

ping, review needed :)

In D28631#643325, @ngraham wrote:

Remove no-longer-relevant reference to icon in AbstractItem.qml

In D28631#643116, @davidre wrote:

I would argue that the implementation is free to ignore it
Data structure that describes extra information associated to this item, that can be visualized for instance by a tooltip (or by any other mean the visualization consider appropriate.
The appropriate one being not at all in this case ;)

OK, makes sense.

Is it OK now? Any other review comments?

I looked at the IconItem implementation. Before add theme directory support I would like to perform some refactoring - D28470.

For SNI, maybe we should consider showing an icon, but only if it is explicitly set? It is part of the specification after all. From the other side it will be inconsistent...

Change class name: *Strategy -> *Source

In D28470#641230, @davidre wrote:

In D28470#640468, @cblack wrote:

Splitting into multiple classes seems like a good idea, but "strategy"? Seems like an odd choice of name to me.

I had assumed it's because of https://en.m.wikipedia.org/wiki/Strategy_pattern

be consistent with old implementation

fix some silly errors

Do you think it is a good direction?

In D28208#638846, @davidre wrote:

Yes it's not pretty but I had no other Idea. One option would be trying to manually search for the icon in the path?

Not a good idea, theme path only point to root of more complex structure like hicolor/22x22/apps/.... This is also code/logic duplication.

The best would be to add theme path support to PlasmaCore.IconItem. I will take a look at this tomorrow.