Code review comment for lp:~yuningdodo/unity-control-center/unity-control-center.lp1396464-prevent-update-sink-input-in-parallel

Revision history for this message
Yu Ning (yuningdodo) wrote :

Thanks for the review and suggestions. To make things more clear let me list all the context where gvc_channel_bar_set_is_muted() is called, you can skip the boring description and go to the summary below.

1) gvc-channel-bar.c: on_scale_button_release_event(): an UI event handler, the adjustment "value-changed" signal is triggered separately, so no need to trigger it again in set_is_muted().

2) gvc-channel-bar.c: gvc_channel_bar_scroll(), which is only called by on_scale_scroll_event(): also an UI event handler, the adjustment "value-changed" signal is triggered separately. Only if the volume value need to be clamped to [min, max] it will adjust the volume again. Anyway, no need to trigger it again in set_is_muted().

3) gvc-channel-bar.c: on_zero_adjustment_value_changed(): volume is also adjusted here.

4) gvc-channel-bar.c: on_mute_check_toggled(): adjustment is switched between priv->adjustment and priv->zero_adjustment, so we should trigger the "value-changed" signal. However in my test the volume can still get updated even without a trigger here, by checking the code we know that a sync with PA is scheduled in GvcMixerStream "is-muted" property callback, so volume is also synced.

5) gvc-mixer-dialog.c: on_stream_is_muted_notify(): this will sync the is-muted status from a GvcMixerStream to the GvcChannelBar, the volume is adjusted in the GvcMixerStream and will also be synced to the GvcChannelBar, so we shouldn't trigger the adjustment "value-changed" signal here.

6) gvc-mixer-dialog.c: bar_set_stream(): volume is also adjusted here.

So as a summary:
* 1, 2, 5: set_volume() is not directly called in the same function, however since it will be called in other signal handlers before or after, no need to trigger the "value-changed" signal in set_is_muted().
* 3, 6: set_volume() is called directly in the same function, no need to trigger the "value-changed" signal in set_is_muted().
* 4: "value-changed" signal should be triggered logically speaking, although it's actually not necessary in current workflow.

For 3 & 6 we can provide a set_volume_and_mute() function, but anyway we should remove the explicit "value-changed" trigger in both set_is_muted() and set_volume_and_mute() since you know the signal will be auto triggered in gtk_adjustment_set_value(). I don't make this change to keep the patch short.

For 4 I have make tests both w/ and w/o the explicit "value-changed" trigger, both works, so I have updated my patch and added the trigger outside the set_is_muted() function to make the logic clear.

« Back to merge proposal