Give users the ability to disable the microphone indicator
AbandonedPublic

Authored by kurmikon on May 22 2020, 7:16 PM.

Details

Reviewers
ngraham
Group Reviewers
VDG
Plasma
Summary

Plasma is an high customizable desktop environment. When it comes to system tray, you can disable battery indicator, volume applet, touchpad indicator, notification applet, etc. Some of these actions are pointless, why someone would disable the volume applet not showing it, not only in the tray, but also in the dropdown menu, since it's crucial to system?

But Plasma gives you this opportunity because it's customizable. However, when it come to microphone indicator: no, you can't do nothing. It stays there, you can only hide it, but it's still present in the dropdown menu, even when you don't want it, even when you don't need it, even when it's frustrating while you're using PulseEffects and it tells you something wrong (see here and here).

Maybe you want to disable it, but unfortunately who made it didn't think to add this ability. The funny thing is that the MicrophoneIndicator class has an enabledChanged signal unused, so maybe they were planning to add the feature to enable/disable it, but that was never done.

So this diff adds this option showing a checkbox inside general configuration.

Test Plan

I made different attempts to achieve this. The class was imagined like a singleton, but it's not really a singleton, anyway it's initialized by the volume applet and it stays there having only one instance.

Exposing it inside the configuration window made some issues because multiple instances where showing and, consequently, more indicators appeared in the tray.

So i modified it like a real singleton. Registering it inside the config window was correctly done but, unfortunately, the engine was destroyng the class when the window was closed, leaving the other one in the applet pointing to nowhere and the system crashed.

Then I made an interface to act on it being able to enable/disable the indicator. The engine could construct and destroy the interface many times, but the singleton is always alive.

I tested it on my system and it's working. Please, test on yours. If there's a better method to do this, discuss here, Thanks.

Diff Detail

Repository
R115 Plasma Audio Volume Applet
Lint
Lint Skipped
Unit
Unit Tests Skipped
kurmikon created this revision.May 22 2020, 7:16 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMay 22 2020, 7:16 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
kurmikon requested review of this revision.May 22 2020, 7:16 PM

Thanks for the patch! KDE has since moved to GitLab at invent.kde.org/ and we would prefer not to use Phabricator for new patches. Could you re-submit this as a merge request at https://invent.kde.org/plasma/plasma-pa/-/merge_requests, and then Abandon this?

Thanks!

@ngraham I'm sending the email, but the system reject the request.

What email? Huh?

What email? Huh?

The page you linked say I have to send an email.

Anyway, I'm able to use git, but I don't get how to push my modification in my branch on my personal repository. Tried to clone on my system and push, but the commit is rejected.

What email? Huh?

The page you linked say I have to send an email.

Anyway, I'm able to use git, but I don't get how to push my modification in my branch on my personal repository. Tried to clone on my system and push, but the commit is rejected.

Emailing a merge request it just one option; you can do the standard fork-and-branch thing too, if you're familiar with the GitHub workflow. Anyway, here's some documentation: https://community.kde.org/Infrastructure/GitLab

@ngraham I'm following instructions, but push is rejected:

remote: Push declined - commits failed audit

You need to push to your fork of the plasma-pa repo, not the main location. git push fork should do it if you've set up your fork with the name fork as described in https://community.kde.org/Infrastructure/GitLab#Add_the_fork_to_your_source_checkout

You need to push to your fork of the plasma-pa repo, not the main location. git push fork should do it if you've set up your fork with the name fork as described in https://community.kde.org/Infrastructure/GitLab#Add_the_fork_to_your_source_checkout

It's exactly what I'm doing, but I keep getting rejected. I'm correctly using github, but this new environment has something weird or I miss something that is not reported inside the guide.

I get:

remote: Audit failure - Commit 8d3868ba226207636e4dd8cf56902314fba01190 - Non-full name: Giusy
remote: Audit failure - Commit 8d3868ba226207636e4dd8cf56902314fba01190 - Non-full name: Giusy
remote: Push declined - commits failed audit
remote: error: hook declined to update refs/heads/micindicator

Also tried to delete config and repush. It adds the host to the list of known hosts, but the commit fails.

I'm using Arch Linux and pushing on github never gave issues.

You are being rejected because those commits were made using a name that appears to just be a first name - you will need to amend your user.name in your Git configuration and amend the commits accordingly

You are being rejected because those commits were made using a name that appears to just be a first name - you will need to amend your user.name in your Git configuration and amend the commits accordingly

git config user.name kurmikon
git commit --amend --author="kurmikon"
remote: Audit failure - Commit 17bb3a3c8f0730073e100437b3b63e96f8d7a7c0 - Non-full name: kurmikon
remote: Audit failure - Commit 17bb3a3c8f0730073e100437b3b63e96f8d7a7c0 - Non-full name: kurmikon

I gave up. Review here, if you want.

ngraham added a comment.EditedMay 23 2020, 1:25 AM

The name needs to be your full name ("Firstname Lastname"). This was a pre-existing requirement, it's just now enforced before patch submission, rather than afterwards.

The name needs to be your full name ("Firstname Lastname"). This was a pre-existing requirement, it's just now enforced before patch submission, rather than afterwards.

I was using the full name @ngraham, but it was rejecting the same. I will retry later, but this process shouldn't be simplified to new users? It's not stated inside the guide and it's useless enforce this requirement since the commit is done with an account where the full name is set in the profile.

Am I the only one experiencing this issue?

meven added a subscriber: meven.May 23 2020, 9:04 AM

The name needs to be your full name ("Firstname Lastname"). This was a pre-existing requirement, it's just now enforced before patch submission, rather than afterwards.

I was using the full name @ngraham, but it was rejecting the same. I will retry later, but this process shouldn't be simplified to new users? It's not stated inside the guide and it's useless enforce this requirement since the commit is done with an account where the full name is set in the profile.

Did you git commit --amend --author "FirstName LastName <email>" ?
You simply need to update the author in the commit itself.

Am I the only one experiencing this issue?

Everybody would yes.
Your problem was just your gitconfig had a pseudonym and kde git policy (for legal reasons) expect full names.

Sorry it seems our documentation is not very clear about this fact currently.

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


I made different attempts to achieve this. The class was imagined like a singleton, but it's not really a singleton, anyway it's initialized by the volume applet and it stays there having only one instance.

The class was a singleton within the given QML context, The configuration UI is another context, which is why it instantiated a new one.


One major comment about the config that I don't understand.

applet/contents/ui/ConfigGeneral.qml
114

I don't see why we're doing this.

Firstly it's a bit messy with regards to when this binds and updates vs when kcfg overwrites this binding.

Secondly and much more importantly why should whether this is currently enabled or not ultimately change the config value stored of whether we show the indicator when something is recording?

Once that's gone the entire singleton aspect is a lot less relevant.

src/qml/microphoneindicator.cpp
320

setStatus(bool) is confusing naming

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.

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'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.

IMO the initial idea is good - add an option to plasma-pa settings.

In D29827#673734, @kmaterka wrote: the system tray icon (some applications have such option).

If you don't want the icon, do not start an app in the first place

Yeah that's not gonna be an option here.

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.

IMO the initial idea is good - add an option to plasma-pa settings.

All right, let's continue with this then.