-
-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement per application volume control #253
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's strange at least for Videos and Music they load for me, I'll look into that. For some apps though they just aren't because the "application name" prop is all over the place (e.g. for me for videos it's io.elementary.videos and for firefox it's Firefox although both are flatpaks). Furthermore the "application id" and "application icon" prop were unfortunately never set as well in my testing.
I think that's actually correct because they are two different sink inputs (for example with firefox and I guess with chrome too two different windows show up as two inputs). I think that might actually be quite useful. I could add the media name to differentiate between them but that's sometimes not meaningful.
AFAIK there isn't a good way to filter them out by default because the prop "media role" most of the time isn't set. We would have to hardcode exceptions which doesn't seem to be a very good solution. |
It now tries pretty much everything to get good app names and icons even a very basic way of using DesktopAppInfo.search. We could consider not showing applications for which no DesktopAppInfo was found but a lot of testing to see whether it reliably works is needed for this because otherwise we might hide some apps that should be shown. |
Yeah, I think the most sustainable thing to do is only support apps that are doing things correctly, setting the correct IDs, the correct media roles, etc. Trying to support misbehaving apps is a dark path that has caused a lot of issues in the past in other components |
I'm not sure but I think the worst that can happen is that a user changes the volume of an app they don't recognize and then maybe wonder why a certain app is silent. And IMHO it's better to accept this and show e.g. a "Firefox" app without an icon instead of not showing it at all especially because in some more testing I did pretty much no app did things anywhere near entirely correct. I personally think if we don't show apps that do everything right it would make this feature partly/pretty much useless and very confusing for the user when some apps that play audio show up but others don't. |
@leolost2605 I mean we already kind of have that situation with notifications and with MPRIS and it has really sucked when we've tried to do all the workarounds to make misbehaving apps show up correctly. I think that's okay if an app doesn't show up at all because we can just point to the way for the developer to fix their app. Some app developers will never fix their apps and we have to accept that because the alternative of trying to special case and workaround broken apps is just not sustainable in the long run |
I think the difference with MPRIS is that to me MPRIS is a nice to have feature but it's not required because you can always do the same in the app itself. With notifications we are guaranteed to always have a app id because it's a mandatory field and for flatpaks it's also always valid, whereas here it's optional and if present (which I've never seen yet) it might be invalid. |
Hm yeah I think this situation will change very rapidly because of Pipewire, but I could be wrong there. It would probably be worth making sure we're building for what we should expect of Flatpak apps using pipewire and if we more consistently get IDs there etc. Unfortunately these cross platform apps are the usual suspects of bad behavior 😅 but the case we have right now is things that are definitely not apps are showing up for a second while using Canberra etc and people will definitely file bugs about Spotify not having an icon or something so it comes across as glitchy and buggy and makes a bad impression about the quality and reliability of the feature |
Unfortunately in my testing pipewire doesn't improve the situation. I'm currently using it and get the same names/app ids as with pulse. Maybe I've got it setup wrong because I only switched to it for some testing and didn't bother going back but everything else works fine 🤷
We could reliably match native apps because their PID is always given and this way filter out system sounds. Unfortunately the PIDs of flatpaks are only the ones used in the sandbox.
One could also argue that people will file (more?) bugs about some apps not showing up at all instead of "only" a missing icon. 😅 Maybe we can keep it as is with the search and multiple ways of trying to get an app info while filtering out the ones for which we don't find one? This way we can support most applications while only having the IMHO acceptable and pretty small risk of getting a wrong AppInfo for an app (e.g. notification sounds show up as Notification Demo) |
In the future, the idea is having the device portal control the pipewire connection to the host, and i believe it will be setting the right client name and roles, but that is only implemented for the camera portal right now. However, if the application is using the pulseaudio API anyway (like libcanberra does), then it's fully in control of the properties like a host application would be, because pulseaudio support happens via a sandbox hole.
We don't try do to this in neither the MPRIS or notification interfaces, why differentiate?
Then we give the response "${APP} doesn't identifies itself correctly, please ask for the ${APP} developers to correct identify itself within pulseaudio". I agree with danielle in this, it's better to work with upstream to fix they issues than "having they back" here. |
… setting to show all
Ok ok you outvoted me :)
That sounds really good maybe we should just wait for that to merge this feature? In the meantime I changed it to show only apps for which a DesktopAppInfo can be found either by using the app name or, if it is set, the application id property. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do it and we can iterate etc as we go :)
Allow controlling volume on a per app basis and muting single applications. A third tab called "applications" was added for this. It's already prepared for an eventual move to GTK4 ListView (performance wise it might not be necessary here but I think better safe than sorry right?) that's why we are binding an app to listboxrow and not constructing it with it. We also allow resetting all active applications to their default volume (100%) but it would be better to do this also with not active/remembered apps but I would leave this to a follow up PR.
The placeholder phrasing is currently not very good ("No applications currently emittings sounds") better suggestions are welcome :)
I've seen that it had existed before here not sure what happened to it though? IMHO it's a very useful feature.
We could also add it to the wingpanel indicator which I think would be great so if somebody has design suggestions for this I'd be happy to implement them :)