Skip to content
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

Merged
merged 26 commits into from
Mar 19, 2024
Merged

Conversation

leolost2605
Copy link
Member

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 :)

@leolost2605 leolost2605 requested a review from a team August 26, 2023 09:22
Copy link
Member

@lenemter lenemter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issues I noticed:

  1. No application icons are loaded
  2. Sometimes "Google Chrome" gets duplicated
  3. System sounds are shown in the list (libcanberra), but they probably shouldn't be

image

@leolost2605
Copy link
Member Author

No application icons are loaded

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.

Sometimes "Google Chrome" gets duplicated

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.

System sounds are shown in the list (libcanberra), but they probably shouldn't be

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.

@leolost2605
Copy link
Member Author

leolost2605 commented Aug 26, 2023

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.

@leolost2605 leolost2605 requested a review from lenemter August 26, 2023 17:56
@danirabbit
Copy link
Member

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

@leolost2605
Copy link
Member Author

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.

@danirabbit
Copy link
Member

danirabbit commented Aug 29, 2023

@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

@leolost2605
Copy link
Member Author

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.
I would still argue it's best to try to get a DesktopAppInfo and if it's not found just show the name. I think it's not that ugly that it should be hidden from the user or that they can't be trusted with access to that functionality. GNOME Control Center for example always just shows the pulseaudio provided name as well. Because for me (and I can imagine for a lot of others) it would be very (and probably only) useful if Firefox/Chrome, Discord, Spotify and steam games are supported which all wouldn't be the case if we required a DesktopAppInfo.
But I'm not sure, that's only my opinion :)

@danirabbit
Copy link
Member

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

@leolost2605
Copy link
Member Author

leolost2605 commented Sep 4, 2023

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 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 🤷

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

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.

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

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)
Or if that's really a no-go we could like you said only support apps that give their correct App ID and I'll look into some more ways of getting an App ID with what we know (although the only way I think that currently might lead somewhere is using the binary name but I don't really know if we can do anything with it 🤷)

@Marukesu
Copy link

Marukesu commented Sep 5, 2023

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 🤷

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

We don't try do to this in neither the MPRIS or notification interfaces, why differentiate?

One could also argue that people will file (more?) bugs about some apps not showing up at all instead of "only" a missing icon. 😅

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.

@leolost2605
Copy link
Member Author

Ok ok you outvoted me :)

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.

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.
I also added a hidden setting to show all apps and I would really like to keep it because IMHO it doesn't go against design when it is hidden in a way only technical users can enable it and it would be really helpful e.g. for debugging or just for advanced users who know that it's not perfect and they shouldn't report issues for it.

@lenemter lenemter added the Conflicts Conflicts with master label Jan 8, 2024
Copy link
Member

@danirabbit danirabbit left a 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 :)

@danirabbit danirabbit dismissed lenemter’s stale review March 19, 2024 16:53

changes were made

@danirabbit danirabbit merged commit b425b91 into main Mar 19, 2024
3 of 4 checks passed
@danirabbit danirabbit deleted the application-volume-control branch March 19, 2024 16:54
@lenemter lenemter removed the Conflicts Conflicts with master label Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants