Merge lp:~dylanmccall/ubuntu/natty/gnome-settings-daemon/fix-748805 into lp:ubuntu/natty/gnome-settings-daemon

[Dylan McCall]

This is a fix for bug #748805. It replaces the code to select an icon for the volume change notification bubble. My replacement uses the same approach as indicator-sound, to resolve consistency issues between these two.

In the same vein, I added support for the notification-audio-volume-off icon, which was available (and is used by indicator-sound's notifications) but it wasn't used here.

[Chris Coulson (chrisccoulson)]

From the bug you said "I decided to change our patch for gnome-settings-daemon, since the existing stuff was a little complex and it wouldn't look pretty inside indicator-sound. (I assume the settings daemon patch inherits that complexity from the original overlay, which had a fancy fading effect)".

Note that this complexity in the code exists to make it easily portable to other applications (we copy and paste the exact same code in gnome-power-manager for example. Even though gnome-power-manager uses a different number of icons, we're able to do this because the thresholds were spaced out in a linear fashion). However, this change actually makes virtually all of gsd-osd-notification.{c/h} pretty pointless now, so you could probably strip most of it out. With the spacing between the icon thresholds being completely uneven, it makes it virtually impossible for this code to be portable anymore.

[Chris Coulson (chrisccoulson)]

With this change, when pressing the volume-down key, doesn't gnome-settings-daemon skip the "notification-audio-volume-off" icon still at zero volume?

review: Needs Information

[Chris Coulson (chrisccoulson)]

Just to clarify, if you press the volume down key whilst current volume < volume step, gnome-settings-daemon sets "muted" internally in addition to setting the new volume to zero. This means that the following code should never evaluate to true when muted == FALSE:

 } else if (value == 0) {
            s = 1;

...which means that you should never see "notification-audio-volume-off", so it will still be out of sync with the sound indicator.

Note, I've not actually tried your patch out yet, so apologies if I'm wrong. But, that's what I think will happen based on my initial review.

To fix this, I think you'd need to also change do_sound_action() so that it only sets muted if the volume down key is pressed AND the current volume is already zero. Does that makes sense?

[Dylan McCall (dylanmccall)]

Ah, thanks for the information! Good to know that about the icon selection code. Do you think this belongs more in the indicators, then? (/me wonders if the power indicator does the same thing…)

The volume == 0 case occurs when we turn the volume to 0, then unmute :)
(Actually I could have sworn seeing it increase the volume a little in that case, but I may have been imagining it; it's doing what I describe right now, anyway).

I do like the idea about the muting behaviour, though, but that's another file to patch. Maybe we should bring it up upstream…

[Chris Coulson (chrisccoulson)]

Remember that the current upstream code is in sync with the volume applet in gnome-media, so they don't really have this problem. It's only a problem for us because our sound indicator defines the thresholds differently

[Dylan McCall (dylanmccall)]

Looks like the mute on zero volume stuff is discussed and patched at bug #332081.

Right now, (I have been dogfooding this for a few days), something higher up tells the notification code that it is muted, even though it isn't. Thanks especially to the proximity between the notification and indicator-sound, this gives us something that looks plain wrong: a big, red Muted icon right next to an icon that (correctly) says the volume is actually not muted; just turned down to zero.

So, I think that should still be fixed in the name of consistency (maybe it can go upstream…), but should I propose the alternative change for icon selection over at indicator-sound instead? I'm happy to do that :)

Unmerged revisions

129. By Dylan McCall on 2011-04-03

* debian/patches/16_use_synchronous_notifications.patch:
  - Choose icons the same way as indicator-sound (LP: #748805)
  - Use notification-audio-volume-off icon

=== modified file 'debian/changelog'
--- debian/changelog	2011-03-31 18:03:24 +0000
+++ debian/changelog	2011-04-04 02:29:23 +0000
@@ -1,3 +1,11 @@
+gnome-settings-daemon (2.32.1-0ubuntu13) UNRELEASED; urgency=low
+
+  * debian/patches/16_use_synchronous_notifications.patch:
+    - Choose icons the same way as indicator-sound (LP: #748805)
+    - Use notification-audio-volume-off icon
+
+ -- Dylan McCall <[e-mail]>  Sat, 02 Apr 2011 22:02:04 -0700
+
 gnome-settings-daemon (2.32.1-0ubuntu12) natty; urgency=low
 
   * debian/patches/93_wait_for_xsettings.patch:

=== modified file 'debian/patches/16_use_synchronous_notifications.patch'
--- debian/patches/16_use_synchronous_notifications.patch	2010-06-10 12:49:47 +0000
+++ debian/patches/16_use_synchronous_notifications.patch	2011-04-04 02:29:23 +0000
@@ -1,11 +1,11 @@
 Description: Use synchronous notifications when they are supported
 Author: ?
 
-Index: gnome-settings-daemon-2.30.0/plugins/common/gsd-osd-notification.c
+Index: gnome-settings-daemon-2.32.1/plugins/common/gsd-osd-notification.c
 ===================================================================
 --- /dev/null	1970-01-01 00:00:00.000000000 +0000
-+++ gnome-settings-daemon-2.30.0/plugins/common/gsd-osd-notification.c	2010-04-23 17:05:08.862133020 +0100
-@@ -0,0 +1,296 @@
++++ gnome-settings-daemon-2.32.1/plugins/common/gsd-osd-notification.c	2011-04-02 21:48:36.418675531 -0700
+@@ -0,0 +1,300 @@
 +/* -*- Mode: C; indent-tabs-mode: nil; c-basic-offset: 8; tab-width: 8 -*- */
 +/*
 + * gsd-osd-notification.c
@@ -58,13 +58,17 @@
 +static const char*
 +_calculate_icon (GsdOsdNotification *notifier, guint value, gboolean muted)
 +{
-+        guint s;
++        guint s = 2;
 +
-+        s = (notifier->priv->icon_array_size -1) * value / 100 + 1;
-+        s = MAX (s, 1);
-+        s = MIN (s, notifier->priv->icon_array_size -1);
-+        if (value <= 0)
-+                s = 0;
++        if (value < 30 && value > 0) {
++                s = 2;
++        } else if (value < 70 && value >= 30) {
++                s = 3;
++        } else if (value >= 70) {
++                s = 4;
++        } else if (value == 0) {
++                s = 1;
++        }
 +
 +        return muted ? notifier->priv->icon_names[0] : notifier->priv->icon_names[s];
 +}
@@ -302,10 +306,10 @@
 +
 +        g_type_class_add_private (klass, sizeof (GsdOsdNotificationPrivate));
 +}
-Index: gnome-settings-daemon-2.30.0/plugins/common/gsd-osd-notification.h
+Index: gnome-settings-daemon-2.32.1/plugins/common/gsd-osd-notification.h
 ===================================================================
 --- /dev/null	1970-01-01 00:00:00.000000000 +0000
-+++ gnome-settings-daemon-2.30.0/plugins/common/gsd-osd-notification.h	2010-04-23 17:05:08.862133020 +0100
++++ gnome-settings-daemon-2.32.1/plugins/common/gsd-osd-notification.h	2011-04-02 21:22:40.306959188 -0700
 @@ -0,0 +1,75 @@
 +/* -*- Mode: C; indent-tabs-mode: nil; c-basic-offset: 8; tab-width: 8 -*- */
 +/*
@@ -382,10 +386,10 @@
 +G_END_DECLS
 +
 +#endif /* _GSD_OSD_NOTIFICATION_H_ */
-Index: gnome-settings-daemon-2.30.0/plugins/media-keys/gsd-media-keys-manager.c
+Index: gnome-settings-daemon-2.32.1/plugins/media-keys/gsd-media-keys-manager.c
 ===================================================================
---- gnome-settings-daemon-2.30.0.orig/plugins/media-keys/gsd-media-keys-manager.c	2010-03-29 09:15:04.000000000 +0100
-+++ gnome-settings-daemon-2.30.0/plugins/media-keys/gsd-media-keys-manager.c	2010-04-23 17:05:08.862133020 +0100
+--- gnome-settings-daemon-2.32.1.orig/plugins/media-keys/gsd-media-keys-manager.c	2011-04-02 21:22:27.974898039 -0700
++++ gnome-settings-daemon-2.32.1/plugins/media-keys/gsd-media-keys-manager.c	2011-04-02 21:22:40.310959209 -0700
 @@ -49,6 +49,7 @@
  #include "eggaccelerators.h"
  #include "acme.h"
@@ -402,12 +406,13 @@
  
          /* Multihead stuff */
          GdkScreen       *current_screen;
-@@ -108,6 +110,13 @@
+@@ -108,6 +110,14 @@
  
  static gpointer manager_object = NULL;
  
 +static const char *volume_icons[] = {
 +        "notification-audio-volume-muted",
++        "notification-audio-volume-off",
 +        "notification-audio-volume-low",
 +        "notification-audio-volume-medium",
 +        "notification-audio-volume-high",
@@ -416,7 +421,7 @@
  
  static void
  init_screens (GsdMediaKeysManager *manager)
-@@ -612,11 +621,13 @@
+@@ -612,11 +622,13 @@
          }
  
          /* Show the dialogue */
@@ -435,7 +440,7 @@
  
          /* Clean up the drive selection and exit if no suitable
           * drives are found */
-@@ -641,23 +652,44 @@
+@@ -641,23 +653,44 @@
          GConfClient *client = manager->priv->conf_client;
          gboolean state = gconf_client_get_bool (client, TOUCHPAD_ENABLED_KEY, NULL);
  
@@ -486,7 +491,7 @@
          vol = CLAMP (vol, 0, 100);
  
          dialog_init (manager);
-@@ -668,12 +700,18 @@
+@@ -668,12 +701,18 @@
                                            GSD_MEDIA_KEYS_WINDOW_ACTION_VOLUME);
          dialog_show (manager);
  
@@ -511,7 +516,7 @@
  }
  
  static void
-@@ -683,6 +721,7 @@
+@@ -683,6 +722,7 @@
          gboolean muted;
          guint vol, norm_vol_step;
          int vol_step;
@@ -519,7 +524,7 @@
          gboolean sound_changed;
  
          if (manager->priv->stream == NULL)
-@@ -722,7 +761,10 @@
+@@ -722,7 +762,10 @@
                          if (gvc_mixer_stream_set_volume (manager->priv->stream, vol) != FALSE) {
                                  gvc_mixer_stream_push_volume (manager->priv->stream);
                                  sound_changed = TRUE;
@@ -530,7 +535,7 @@
                  }
                  break;
          case VOLUME_UP_KEY:
-@@ -750,12 +792,14 @@
+@@ -750,12 +793,14 @@
                                          gvc_mixer_stream_push_volume (manager->priv->stream);
                                          sound_changed = TRUE;
                                  }
@@ -546,7 +551,7 @@
  }
  
  static void
-@@ -900,8 +944,11 @@
+@@ -900,8 +945,11 @@
  
  static gboolean
  do_multimedia_player_action (GsdMediaKeysManager *manager,
@@ -558,7 +563,7 @@
          return gsd_media_player_key_pressed (manager, key);
  }
  
-@@ -972,19 +1019,19 @@
+@@ -972,19 +1020,19 @@
                  execute (manager, "gcalctool", FALSE, FALSE);
                  break;
          case PLAY_KEY:
@@ -583,7 +588,7 @@
                  break;
          default:
                  g_assert_not_reached ();
-@@ -1121,6 +1168,8 @@
+@@ -1121,6 +1169,8 @@
  
          gvc_mixer_control_open (manager->priv->volume);
  
@@ -592,7 +597,7 @@
          gnome_settings_profile_end ("gvc_mixer_control_new");
  #endif /* HAVE_PULSE */
          g_idle_add ((GSourceFunc) start_media_keys_idle_cb, manager);
-@@ -1218,6 +1267,9 @@
+@@ -1218,6 +1268,9 @@
          }
          g_list_free (priv->media_players);
          priv->media_players = NULL;
@@ -602,10 +607,10 @@
  }
  
  static void
-Index: gnome-settings-daemon-2.30.0/plugins/common/Makefile.am
+Index: gnome-settings-daemon-2.32.1/plugins/common/Makefile.am
 ===================================================================
---- gnome-settings-daemon-2.30.0.orig/plugins/common/Makefile.am	2010-03-10 11:59:38.000000000 +0000
-+++ gnome-settings-daemon-2.30.0/plugins/common/Makefile.am	2010-04-23 17:10:43.209632216 +0100
+--- gnome-settings-daemon-2.32.1.orig/plugins/common/Makefile.am	2011-04-02 21:22:28.002898184 -0700
++++ gnome-settings-daemon-2.32.1/plugins/common/Makefile.am	2011-04-02 21:22:40.310959209 -0700
 @@ -7,17 +7,21 @@
  	gsd-keygrab.c		\
  	gsd-keygrab.h		\