Merge lp:~3v1n0/indicator-sound/notify-osd-on-scroll into lp:~indicator-applet-developers/indicator-sound/trunk_3

[Marco Trevisan (Treviño)]

The indicator-sound indicator has a great usability issue imho: in fact when using the scroll event over it to change the volume level, no feedback is given (just the not-precise-at-all icon change).

Now I know that tooltips can't be show, but showing a notification via notify-osd like when using the volume keys imho is a good way to give precise informations about the current volume level, keeping coherency with the system.

Here how it works (video): http://go.3v1n0.net/hjRrGu

There's just one thing to improve, currently it "reccomends" the installation of notify-osd-icons, if this is has not been done, the "off" volume icon is not shown (I guess the "mute" one should be shown instead), I've tried to make it works anyway, but it doesn't. Look at commit 189 (http://bazaar.launchpad.net/~3v1n0/indicator-sound/notify-osd-on-scroll/revision/189) for more informations.

[TheBeest (thebeest)]

Looks great!

[Tom Wright (twright-tdw)]

Great idea; this should greatly improve the usability of the sound indicator.

[Marco Trevisan (Treviño) (3v1n0)]

I was thinking... Maybe you'd prefer to get an option to disable this feature on request, with something like a gconf key...

[Conor Curran (cjcurran)]

Trevino, nice work.
I'd like to merge this after alpha 2 ( this week ) if that is okay. Some pressing bugs to see to first. A gsettings entry to switch on or off this would definitely help. I merged a big refactor yesterday which means alot your work below needs to be restructured. Not a biggie, we can look at this next week.

Conor

review: Needs Resubmitting

[Matthew Paul Thomas (mpt)]

I like this design-wise. I have corrected the Notify OSD specification to match. <https://wiki.ubuntu.com/NotifyOSD?action=diff&rev2=205&rev1=203> Thanks for your work, Treviño.

review: Approve (design)

[Marco Trevisan (Treviño) (3v1n0)]

> Trevino, nice work.
> I'd like to merge this after alpha 2 ( this week ) if that is okay. Some
> pressing bugs to see to first. A gsettings entry to switch on or off this
> would definitely help.

Ok, I'll see to do that, however I'd keep that setting ON by default, isn't it?

> I merged a big refactor yesterday which means alot your
> work below needs to be restructured. Not a biggie, we can look at this next
> week.

Yes, I saw that, I also had already started to merge my changes.

[Conor Curran (cjcurran)]

On by the default it fine.

>Yes, I saw that, I also had already started to merge my changes.
great.

Thanks for the nice patch.

Conor

[Marco Trevisan (Treviño) (3v1n0)]

> On by the default it fine.

Done, the patch is coming to launchpad, feel free to merge it when you want! :)

[Marco Trevisan (Treviño) (3v1n0)]

> > On by the default it fine.
>
> Done, the patch is coming to launchpad, feel free to merge it when you want!

Ah, remember that packagers should add "notify-osd" and "notify-osd-icons" in the Recommends field...

[Conor Curran (cjcurran)]

Works nicely Trevino. Thank you !
I might in the morning move the notify code out of the indicator-sound and into the state-manager. Will merge by lunchtime.
thanks again :)
c

review: Approve

=== modified file 'configure.ac'
--- configure.ac	2011-01-27 03:09:45 +0000
+++ configure.ac	2011-01-27 17:35:48 +0000
@@ -35,11 +35,13 @@
 INDICATOR_DISPLAY_OBJECTS=0.1.11
 DBUSMENUGLIB_REQUIRED_VERSION=0.3.9
 GIO_2_0_REQUIRED_VERSION=2.25.13
+LIBNOTIFY_REQUIRED_VERSION=0.5.0
 
 PKG_CHECK_MODULES(APPLET, gtk+-2.0 >= $GTK_REQUIRED_VERSION
                           indicator >= $INDICATOR_REQUIRED_VERSION
                           dbusmenu-gtk-0.4 >= $DBUSMENUGTK_REQUIRED_VERSION
-                          libido-0.1 >= $INDICATOR_DISPLAY_OBJECTS)
+                          libido-0.1 >= $INDICATOR_DISPLAY_OBJECTS
+                          libnotify >= $LIBNOTIFY_REQUIRED_VERSION)
                           
 AC_SUBST(APPLET_CFLAGS)
 AC_SUBST(APPLET_LIBS)
 
=== modified file 'data/com.canonical.indicators.sound.gschema.xml'
--- data/com.canonical.indicators.sound.gschema.xml	2010-12-16 12:36:43 +0000
+++ data/com.canonical.indicators.sound.gschema.xml	2011-01-27 17:35:48 +0000
@@ -25,5 +25,14 @@
         On start up volume should not be muted.
       </description>
     </key>
+    <key name="show-notify-osd-on-scroll" type="b">
+      <default>true</default>
+      <summary>Initial setting for showing notify-osd notification on scroll volume-change</summary>
+      <description>
+        When using the mouse scroll-wheel over the indicator-sound icon, the volume changes.
+        Enabling this setting, every scroll volume-change a notify-osd bubble with the
+        updated volume value will be shown (if supported by your notification daemon).
+      </description>
+    </key>
   </schema>
 </schemalist>
 
=== modified file 'src/indicator-sound.c'
--- src/indicator-sound.c	2011-01-26 22:40:38 +0000
+++ src/indicator-sound.c	2011-01-27 17:35:48 +0000
@@ -27,6 +27,8 @@
 
 #include <gio/gio.h>
 
+#include <libnotify/notify.h>
+
 #include "indicator-sound.h"
 #include "transport-widget.h"
 #include "metadata-widget.h"
@@ -47,6 +49,8 @@
   GList* transport_widgets_list;
   GDBusProxy *dbus_proxy; 
   SoundStateManager* state_manager;
+  GSettings *settings_manager;
+  NotifyNotification* notification;
 };
 
 #define INDICATOR_SOUND_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), INDICATOR_SOUND_TYPE, IndicatorSoundPrivate))
@@ -70,6 +74,11 @@
                                     gint delta,
                                     IndicatorScrollDirection direction);
 
+//Notification
+static void indicator_sound_notification_init (IndicatorSound *self);
+static void indicator_sound_notification_show (IndicatorSound *self,
+                                               SoundState state, double value);
+
 //key/moust event handlers
 static gboolean key_press_cb(GtkWidget* widget, GdkEventKey* event, gpointer data);
 static gboolean key_release_cb(GtkWidget* widget, GdkEventKey* event, gpointer data);
@@ -130,7 +139,11 @@
   GList* t_list = NULL;
   priv->transport_widgets_list = t_list;
   priv->state_manager = g_object_new (SOUND_TYPE_STATE_MANAGER, NULL);
-  
+  priv->notification = NULL;
+
+  priv->settings_manager = g_settings_new("com.canonical.indicators.sound");
+  indicator_sound_notification_init (self);
+
   g_signal_connect ( G_OBJECT(self->service),
                      INDICATOR_SERVICE_MANAGER_SIGNAL_CONNECTION_CHANGE,
                      G_CALLBACK(connection_changed), self );
@@ -149,6 +162,12 @@
 
   g_list_free ( priv->transport_widgets_list );
 
+  g_object_unref(priv->settings_manager);
+
+  if (priv->notification) {
+    notify_uninit();
+  }
+
   G_OBJECT_CLASS (indicator_sound_parent_class)->dispose (object);
   return;
 }
@@ -198,6 +217,33 @@
 }
 
 static void
+indicator_sound_notification_init (IndicatorSound *self)
+{
+  IndicatorSoundPrivate* priv = INDICATOR_SOUND_GET_PRIVATE(self);
+
+  if (!notify_init(PACKAGE_NAME))
+    return;
+
+  GList* caps = notify_get_server_caps();
+  gboolean has_notify_osd = FALSE;
+
+  if (caps) {
+    if (g_list_find_custom(caps, "x-canonical-private-synchronous",
+                           (GCompareFunc) g_strcmp0)) {
+      has_notify_osd = TRUE;
+    }
+    g_list_foreach(caps, (GFunc) g_free, NULL);
+    g_list_free(caps);
+  }
+
+  if (has_notify_osd) {
+    priv->notification = notify_notification_new(PACKAGE_NAME, NULL, NULL, NULL);
+    notify_notification_set_hint_string(priv->notification,
+                                        "x-canonical-private-synchronous", "");
+  }
+}
+
+static void
 connection_changed (IndicatorServiceManager * sm,
                     gboolean connected,
                     gpointer user_data)
@@ -388,7 +434,11 @@
                                   newitem,
                                   menu_volume_item,
                                   parent);
-  return TRUE;  
+
+  if (priv->notification)
+    notify_notification_attach_to_widget(priv->notification, volume_widget);
+
+  return TRUE;
 }
 
 /*******************************************************************/
@@ -549,6 +599,34 @@
   return digested;
 }
 
+static void
+indicator_sound_notification_show(IndicatorSound *self, SoundState state, double value)
+{
+  IndicatorSoundPrivate* priv = INDICATOR_SOUND_GET_PRIVATE(self);
+
+  if (priv->notification == NULL)
+    return;
+
+  char *icon;
+  const int notify_value = CLAMP((int)value, -1, 101);
+
+  if (state == ZERO_LEVEL) {
+    // Not available for all the themes
+    icon = "audio-volume-off";
+  } else if (state == LOW_LEVEL) {
+    icon = "audio-volume-low";
+  } else if (state == MEDIUM_LEVEL) {
+    icon = "audio-volume-medium";
+  } else if (state == HIGH_LEVEL) {
+    icon = "audio-volume-high";
+  } else {
+    icon = "audio-volume-muted";
+  }
+
+  notify_notification_update(priv->notification, PACKAGE_NAME, NULL, icon);
+  notify_notification_set_hint_int32(priv->notification, "value", notify_value);
+  notify_notification_show(priv->notification, NULL);
+}
 
 static void
 indicator_sound_scroll (IndicatorObject *io, gint delta, 
@@ -575,5 +653,8 @@
     value -= adj->step_increment;
   }
   //g_debug("indicator-sound-scroll - update slider with value %f", value);
-  volume_widget_update(VOLUME_WIDGET(priv->volume_widget), value);  
+  volume_widget_update(VOLUME_WIDGET(priv->volume_widget), value);
+
+  if (g_settings_get_boolean(priv->settings_manager, "show-notify-osd-on-scroll"))
+    indicator_sound_notification_show(INDICATOR_SOUND (io), current_state, value);
 }
 
=== modified file 'src/indicator-sound.h'
--- src/indicator-sound.h	2011-01-24 22:26:21 +0000
+++ src/indicator-sound.h	2011-01-27 17:35:48 +0000
@@ -23,6 +23,8 @@
 You should have received a copy of the GNU General Public License along
 with this program.  If not, see <http://www.gnu.org/licenses/>.
 */
+#include "config.h"
+
 #include <libindicator/indicator.h>
 #include <libindicator/indicator-object.h>
 #include <libindicator/indicator-service-manager.h>