Merge lp:~binli/unity-control-center/1291862 into lp:unity-control-center

Proposed by Bin Li
Status: Merged
Approved by: Iain Lane
Approved revision: 12757
Merged at revision: 12760
Proposed branch: lp:~binli/unity-control-center/1291862
Merge into: lp:unity-control-center
Diff against target: 83 lines (+28/-2)
2 files modified
debian/changelog (+6/-0)
panels/sound/gvc-mixer-dialog.c (+22/-2)
To merge this branch: bzr merge lp:~binli/unity-control-center/1291862
Reviewer Review Type Date Requested Status
Iain Lane Approve
Bin Li (community) Needs Resubmitting
Sebastien Bacher Needs Fixing
David Henningsson Pending
PS Jenkins bot Pending
Review via email: mp+211258@code.launchpad.net

Description of the change

Disable the input bar when there are no input devices.

To post a comment you must log in.
Revision history for this message
Sebastien Bacher (seb128) wrote :

Thank you for the work, the change seems fine to me and it looks like it should apply to upstream GNOME (the code seems similar in https://git.gnome.org/browse/gnome-control-center/plain/panels/sound/gvc-mixer-dialog.c)

Could you forward the change to them (https://bugzilla.gnome.org/enter_bug.cgi?product=gnome-control-center)

It also looks like you could apply the same fix for the output as well (not sure how frequent it is to get a machine without audio ouput but it looks like it would lead to the same issue)

review: Needs Fixing
Revision history for this message
Iain Lane (laney) wrote :

With this patch, I noticed that if you have multiple inputs (> 1) then the slider is made insensitive when you remove one of them, meaning that you can't update the remaining input's volume.

There's a similar piece of code for adding inputs and outputs that only fires if it's the first one - it makes sense to do that check here too, and fixes that bug. You only need to consider making the slider insensitive if we are handling the removal of the last input.

I did this in lp:~laney/unity-control-center/make-insensitive-on-last-removal-only - please have a look and consider merging into this branch.

review: Needs Fixing
12753. By Bin Li

Merge Iain Lane patch to check if it's the last device

12754. By Bin Li

merge from upstream

Revision history for this message
Bin Li (binli) wrote :

> With this patch, I noticed that if you have multiple inputs (> 1) then the
> slider is made insensitive when you remove one of them, meaning that you can't
> update the remaining input's volume.
>
> There's a similar piece of code for adding inputs and outputs that only fires
> if it's the first one - it makes sense to do that check here too, and fixes
> that bug. You only need to consider making the slider insensitive if we are
> handling the removal of the last input.
>
> I did this in lp:~laney/unity-control-center/make-insensitive-on-last-removal-
> only - please have a look and consider merging into this branch.

Iain,

 Thanks, merged it and tested it. :)

review: Needs Resubmitting
Revision history for this message
Bin Li (binli) wrote :

> Thank you for the work, the change seems fine to me and it looks like it
> should apply to upstream GNOME (the code seems similar in
> https://git.gnome.org/browse/gnome-control-center/plain/panels/sound/gvc-
> mixer-dialog.c)
>
> Could you forward the change to them
> (https://bugzilla.gnome.org/enter_bug.cgi?product=gnome-control-center)
>
> It also looks like you could apply the same fix for the output as well (not
> sure how frequent it is to get a machine without audio ouput but it looks like
> it would lead to the same issue)
Sebastien,

 For output there should be same issue, I'll try to fix it. And the upstream have a lot of version for it, I'll compare the source code and push it into latest version.

 Thanks a lot!

12755. By Bin Li

Disable output bar when there are no output devices

12756. By Bin Li

update changelog

Revision history for this message
Bin Li (binli) wrote :

> Thank you for the work, the change seems fine to me and it looks like it
> should apply to upstream GNOME (the code seems similar in
> https://git.gnome.org/browse/gnome-control-center/plain/panels/sound/gvc-
> mixer-dialog.c)
>
> Could you forward the change to them
> (https://bugzilla.gnome.org/enter_bug.cgi?product=gnome-control-center)
>
> It also looks like you could apply the same fix for the output as well (not
> sure how frequent it is to get a machine without audio ouput but it looks like
> it would lead to the same issue)
Sebastien,

I've finished the code for output bar.

review: Needs Resubmitting
Revision history for this message
David Henningsson (diwic) wrote :

Looked at the code. It seems a bit weird perhaps to call active_input_update with a device that is being removed, but hey, if it solves the problem...

But at least we should probably change the g_warning ("The tree is empty => we have no devices... into a g_debug if this is in fact a supported and expected scenario.

(Same for output)

12757. By Bin Li

change g_warning to g_debug cause we have a supported and expected scenario.

Revision history for this message
Bin Li (binli) wrote :

> Looked at the code. It seems a bit weird perhaps to call active_input_update
> with a device that is being removed, but hey, if it solves the problem...
>
> But at least we should probably change the g_warning ("The tree is empty => we
> have no devices... into a g_debug if this is in fact a supported and expected
> scenario.
>
> (Same for output)
David,

 Thanks for your review. I've resubmitted again. :)

Revision history for this message
Iain Lane (laney) wrote :

Thanks, it looks good now and works for me.

Please forward the change to GNOME, as Seb suggested earlier.

review: Approve
Revision history for this message
Bin Li (binli) wrote :

> Thanks, it looks good now and works for me.
>
> Please forward the change to GNOME, as Seb suggested earlier.
Done for it.
https://bugzilla.gnome.org/show_bug.cgi?id=726686

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'debian/changelog'
2--- debian/changelog 2014-03-17 13:46:59 +0000
3+++ debian/changelog 2014-03-18 08:35:41 +0000
4@@ -1,3 +1,9 @@
5+unity-control-center (14.04.3+14.04.20140317-0ubuntu2) trusty; urgency=low
6+
7+ * Disable the input/output bar when no input/output devices (LP: #1291862)
8+
9+ -- Bin Li <bin.li@canonical.com> Mon, 18 Mar 2014 12:11:03 +0800
10+
11 unity-control-center (14.04.3+14.04.20140317-0ubuntu1) trusty; urgency=low
12
13 [ Robert Ancell ]
14
15=== modified file 'panels/sound/gvc-mixer-dialog.c'
16--- panels/sound/gvc-mixer-dialog.c 2014-03-14 14:46:32 +0000
17+++ panels/sound/gvc-mixer-dialog.c 2014-03-18 08:35:41 +0000
18@@ -727,7 +727,11 @@
19 model = gtk_tree_view_get_model (GTK_TREE_VIEW (dialog->priv->input_treeview));
20
21 if (gtk_tree_model_get_iter_first (model, &iter) == FALSE){
22- g_warning ("The tree is empty => we have no devices so cannot set the active input");
23+ /* When there are no device we need disable the input bar and set the original name */
24+ gtk_label_set_label (GTK_LABEL(dialog->priv->selected_input_label),
25+ _("Settings for the selected device"));
26+ gtk_widget_set_sensitive (dialog->priv->input_bar, FALSE);
27+ g_debug ("The tree is empty => we have no devices so cannot set the active input");
28 return;
29 }
30
31@@ -850,7 +854,11 @@
32 model = gtk_tree_view_get_model (GTK_TREE_VIEW (dialog->priv->output_treeview));
33
34 if (gtk_tree_model_get_iter_first (model, &iter) == FALSE){
35- g_warning ("The tree is empty => we have no devices in the tree => cannot set the active output");
36+ /* When there are no device we need disable the output bar and set the original name */
37+ gtk_label_set_label (GTK_LABEL(dialog->priv->selected_output_label),
38+ _("Settings for the selected device"));
39+ gtk_widget_set_sensitive (dialog->priv->output_bar, FALSE);
40+ g_debug ("The tree is empty => we have no devices in the tree => cannot set the active output");
41 return;
42 }
43
44@@ -1398,6 +1406,7 @@
45 GvcMixerDialog *dialog)
46 {
47 GtkWidget *bar;
48+ gboolean is_last_device;
49 gboolean found;
50 GtkTreeIter iter;
51 GtkTreeModel *model;
52@@ -1421,6 +1430,11 @@
53 if (found) {
54 gtk_list_store_remove (GTK_LIST_STORE (model), &iter);
55 }
56+
57+ /* Disable output bar when all output devices removed */
58+ is_last_device = !gtk_tree_model_get_iter_first (model, &iter);
59+ if (is_last_device)
60+ active_output_update (dialog, out);
61 }
62
63
64@@ -1432,6 +1446,7 @@
65 {
66 GtkWidget *bar;
67 gboolean found;
68+ gboolean is_last_device;
69 GtkTreeIter iter;
70 GtkTreeModel *model;
71
72@@ -1454,6 +1469,11 @@
73 if (found) {
74 gtk_list_store_remove (GTK_LIST_STORE (model), &iter);
75 }
76+
77+ /* Disable input bar when all input devices removed */
78+ is_last_device = !gtk_tree_model_get_iter_first (model, &iter);
79+ if (is_last_device)
80+ active_input_update (dialog, in);
81 }
82
83 static void

Subscribers

People subscribed via source and target branches