GNOME Bugzilla – Bug 689931
Improve icons in the input/output selectors.
Last modified: 2013-02-18 13:44:10 UTC
Currently, for internal devices, pulseaudio gives just audio-card for a pa_card / GvcMixerCard, which means that the input selector shows audio-card for all devices, not caring if they're a microphone, a laptop speaker or headphones. We should take advantage of the GvcMixerUIDevice internal abstraction and find something better.
Created attachment 231073 [details] [review] Add a GIcon accessor for GvcMixerUIDevices This will allow to have different icons for internal audio cards (which are flagged generically as "audio-card"), depending on which port is in use (ie. headphones or speakers).
Created attachment 231074 [details] [review] sound: show a different icon for different input/output ports Use the new API in libgnome-volume-control to create the icon shown next to the port name in the selector. This avoids ugly audio-card icons, and shows a speaker, microphone or headphones as appropriate.
Review of attachment 231073 [details] [review]: See bug 689635. I guess this patch is an improvement over the current situation (even if comparing strings is somewhat hacky), but we really want to get the right icon from PulseAudio in the first place, by reading it from the port properties.
Review of attachment 231073 [details] [review]: Agreed with David here. I also think that we'll want to export the translated name of the port too, so we could show "Headphones" in the volume popup.
Review of attachment 231074 [details] [review]: That would be fine by me, after the problem of exporting port icons is fixed properly.
David, is this ready to go in?
Bastien, as a last-minute patch I was able to get the port icons exported from PulseAudio into 3.0, which is now released. So we should really get the icons from there instead. I'm fine with both merging (this could be a better fallback for older versions, i e 2.0) and not merging (to encourage Giovanni to read the port icons out of PulseAudio instead).
Created attachment 235122 [details] [review] Add a GIcon accessor for GvcMixerUIDevices This will allow to have different icons for internal audio cards (which are flagged generically as "audio-card"), depending on which port is in use (ie. headphones or speakers). This requires the new icon information, which is only exported by PulseAudio 3.0. If it's not available, we fallback to card icons like before.
Review of attachment 235122 [details] [review]: Good work! I haven't done more than a read through, but I wonder if port->icon_name is correctly freed?
Well, I don't know, because I could not find the place where GvcMixerCardPort is freed...
Right. Now that I think of it, I discovered that too, and added that when I wrote my patches for supporting dynamic ports (see https://bugzilla.gnome.org/show_bug.cgi?id=689359 ). I never finished those patches though, as I got busy with other things :-/
Created attachment 236312 [details] [review] GvcMixerCard: fix freeing mixer ports Allocated GvcMixerCardPorts were never freed.
Review of attachment 236312 [details] [review]: Looks good
Review of attachment 235122 [details] [review]: And as David mentioned, the freeing is missing from this patch (the subsequent patch should be merged into this one). ::: gvc-mixer-ui-device.c @@ +33,3 @@ GvcMixerCard *card; gchar *port_name; + char *icon_name; trailing whitespace here.
Review of attachment 236312 [details] [review]: Please make sure to merge it in with the other patch.
Attachment 235122 [details] pushed as 74c0862 - Add a GIcon accessor for GvcMixerUIDevices
Attachment 231074 [details] pushed as 3e71962 - sound: show a different icon for different input/output ports