GNOME Bugzilla – Bug 645708
when audio server isn't available don't show microphone
Last modified: 2011-08-25 22:59:43 UTC
I just tried to disable pulseaudio to work around a hang. I noticed this had a strange side effect of adding a microphone slider to the sound menu. Also, the sliders pretend like they can change the levels when they can't. We should probably indicate that to the user by making them insensitive at least. For 3.2 maybe we can show a message too.
Created attachment 184302 [details] [review] VolumeStatus: hide when PulseAudio is not available When connection to pulseaudio fails or goes down, hide the volume indicator and associated menu.
I don't think this is a priority issue for 3.0 - it only comes up in the case where PulseAudio is misconfigured; I think we should wait for 3.0.1 for this fix. The patch looks reasonable. My only question is that it appears to me that in the case where any of the individual requests - pa_context_get_server_info(), etc, fail that we emit neither 'ready' nor 'failed' will emitted. But I'd like Bastien to review this (set to accepted-commit-after-freeze if you like it), and also he should probably look if this needs to be merged back to the GVC orginal sources.
Patch looks good to me, but I wonder whether a property and using a g_object_notify() to replace the "ready"/"not ready" signals would be enough. Or even re-using this single signal renamed as "status-changed" with a "gboolean ready" member. I would also need similar patches to the gvc-applet and gvc panel as well.
Created attachment 184456 [details] [review] GnomeVolumeControl: track PulseAudio connection state and expose it Adds get_state() and ::state-changed signals, that replace connecting and ready, as well as providing indication of when the object was closed or the connection to PulseAudio failed. I did not make it a realy GObject property because I didn't want to add all the glib-mkenums infrastructure. Can rework if you think it is cleaner. Most of the patch are whitespace changes, as I tried to keep everything aligned as it was before. If you prefer to have useful git blame, I'll remove those changes. I will prepare a patch for gnome-control-center and gnome-volume-control-applet as soon as this is approved.
Created attachment 184457 [details] [review] VolumeStatus: track PulseAudio state and hide when disconnected Only show the menu when the associated GvcMixerControl is ready, as the connection can fail or PulseAudio may not be installed.
Ping?
Review of attachment 184456 [details] [review]: Loads of unrelated line changes making the patch too busy. The code itself looks fine. ::: src/gvc/gvc-mixer-control.c @@ +61,3 @@ + int n_outstanding; + guint reconnect_id; + GvcMixerControlState state; You're only adding _one_ line. I'd rather have it not aligned and the patch be readable. Simply add it at the bottom of the struct so it doesn't look too out of place, or leave it on the same line). ::: src/gvc/gvc-mixer-control.h @@ +55,3 @@ GObjectClass parent_class; + void (*state_changed) (GvcMixerControl *control, A single line change. @@ +77,3 @@ +gboolean gvc_mixer_control_open (GvcMixerControl *control); +gboolean gvc_mixer_control_close (GvcMixerControl *control); +GvcMixerControlState gvc_mixer_control_get_state (GvcMixerControl *control); Again, a single line change.
Can't comment on the gnome-shell patch, looks correct. I'd still need to have the patches for the control-center panel, panel applet and gnome-settings-daemon (eg. I want the cut'n'paste code to be committed first to its original location in gnome-control-center).
Created attachment 194124 [details] [review] GnomeVolumeControl: track PulseAudio connection state and expose it Adds get_state() and ::state-changed signals, that replace connecting and ready, as well as providing indication of when the object was closed or the connection to PulseAudio failed. Patch against gnome-control-center
Created attachment 194125 [details] [review] GnomeVolumeControl: track PulseAudio connection state and expose it Adds get_state() and ::state-changed signals, that replace connecting and ready, as well as providing indication of when the object was closed or the connection to PulseAudio failed. Patch without without whitespace changes.
Created attachment 194129 [details] [review] Update volume-control applet for GVC API changes. "ready" and "connecting" signals were replaced by a single "state-changed".
Review of attachment 184457 [details] [review]: If the rest of the patches are right, this looks good to commit to me ::: js/ui/status/volume.js @@ +110,3 @@ + this.actor.show(); + } else + this.actor.hide(); If one branche of an if has {}, so should the other - this looks ugly
Looks fine to me.
Review of attachment 194125 [details] [review]: Bastien said on IRC this looks good
Comment on attachment 184457 [details] [review] VolumeStatus: track PulseAudio state and hide when disconnected Attachment 184457 [details] pushed as a64e0e1 - VolumeStatus: track PulseAudio state and hide when disconnected Attachment 194125 [details] pushed as 270e82e - GnomeVolumeControl: track PulseAudio connection state and expose it Waiting approval on the sound applet before committing to gnome-control-center
Attachment 194125 [details] pushed as a57f32a - GnomeVolumeControl: track PulseAudio connection state and expose it Attachment 194129 [details] pushed as 1488246 - Update volume-control applet for GVC API changes.