Merge lp:~cjcurran/indicator-sound/blacklist-dbus-access into lp:~indicator-applet-developers/indicator-sound/trunk_3

[Conor Curran]

Fixes the 'bug' attached. Now addition or removal of players from the menu can be achieved through dbus.
Any change to the gsettings is also automatically reflected in the menu.

[Ted Gould (ted)]

+ g_variant_get (the_black_list, "as", &iter);

I don't think you can do this to the iterator like that. You need to do a g_variant_iter_init();

I'm pretty sure you need to reinitialize the iterator before using it in a second loop.

You can allocate the builder on the stack, which would make the code slightly faster. Something like:

  GVariantBuilder builder;
  g_variant_builder_init(&builder, G_VARIANT_TYPE_ARRAY);

When you find that it's already in the black list you still need to clean up the builder. (note: even if it's on the stack it needs an _end()

review: Needs Fixing

[Mikkel Kamstrup Erlandsen (kamstrup)]

 review needsfixing

> === modified file 'src/sound-service-dbus.c'
...
> +static gboolean sound_service_dbus_blacklist_player (SoundServiceDbus* self,
> +                                                     gchar* player_name,
> +                                                     gboolean blacklist)
> +{
> +  gboolean result = FALSE;
> +  GSettings* our_settings = NULL;
> +  our_settings  = g_settings_new ("com.canonical.indicators.sound");
> +  GVariant* the_black_list = g_settings_get_value (our_settings,
> +                                                   "blacklisted-media-players");
> +  GVariantIter iter;
> +  gchar *str;
> +  // Firstly prep new array which will be set on the key.
> +  GVariantBuilder builder;
> +
> +  g_variant_iter_init (&iter, the_black_list);
> +  g_variant_builder_init(&builder, G_VARIANT_TYPE_STRING_ARRAY);
> +
> +  while (g_variant_iter_loop (&iter, "s", &str)){
> +    g_variant_builder_add (&builder, "s", str);
> +  }
> +  g_variant_iter_init (&iter, the_black_list);
> +
> +  if (blacklist == TRUE){
> +    while (g_variant_iter_loop (&iter, "s", &str)){
> +      g_print ("first pass to check if %s is present\n", str);
> +      if (g_strcmp0 (player_name, str) == 0){
> +        // Return if its already there
> +        g_debug ("we have this already blacklisted, no need to do anything");
> +        g_variant_builder_end (&builder);
> +        g_object_unref (our_settings);
> +        return result;

Here you'll leak the result from g_variant_builder_end() and
the_black_list. It's probably simplest to use g_variant_iter_clear()
instead of _end() (an then unref the_black_list of course).

> +      }
> +    }
> +    // Otherwise blacklist it !
> +    g_debug ("about to blacklist %s", player_name);
> +    g_variant_builder_add (&builder, "s", player_name);
> +  }
> +  else{
> +    gboolean present = FALSE;
> +    g_variant_iter_init (&iter, the_black_list);
> +    g_debug ("attempting to UN-blacklist %s", player_name);
> +
> +    while (g_variant_iter_loop (&iter, "s", &str)){
> +      if (g_strcmp0 (player_name, str) == 0){
> +        present = TRUE;
> +      }
> +    }
> +    // It was not there anyway, return false
> +    if (present == FALSE){
> +      g_debug ("it was not blacklisted ?, no need to do anything");
> +      g_variant_builder_end (&builder);
> +      g_object_unref (our_settings);
> +      return result;

Same as return statement above.

> +    }
> +
> +    // Otherwise free the builder and reconstruct ensuring no duplicates.
> +    g_variant_builder_end (&builder);

_clear() as before

> +    g_variant_builder_init (&builder, G_VARIANT_TYPE_STRING_ARRAY);
> +
> +    g_variant_iter_init (&iter, the_black_list);
> +
> +    while (g_variant_iter_loop (&iter, "s", &str)){
> +      if (g_strcmp0 (player_name, str) != 0){
> +        g_variant_builder_add (&builder, "s", str);
> +      }
> +    }
> +  }
> +  GVariant* value = g_variant_builder_end (&builder);
> +  g_variant_ref (value);
> +  result = g_settings_set_value (our_settings,
> +                                 "blacklisted-media-players",
> +                                 value);
> +
> +  g_variant_unref (value);

No need to ref/unref v...

review: Needs Fixing

[Mikkel Kamstrup Erlandsen (kamstrup)]

Just a clarification. Conor's original code is perfectly valid:

GVariantIter *iter;
g_variant_get (val, "as", &iter);
// do stuff
g_variant_iter_free(iter);

although it does seem like there was missing a call to free the iter? Please see http://library.gnome.org/devel/glib/unstable/gvariant-format-strings.html#gvariant-format-strings-arrays

And for cleaning up a stack allocated builder when you find you don't need it, use g_variant_builder_clear() not _end() as the latter call will return a ref to a newly allocated variant that you'd also have to free.

[Conor Curran (cjcurran)]

> There is generally something about this
> sound_service_dbus_blacklist_player() method that nags me - it seems
> overly complex in it's logical structure for doing what is really just
> updating a set of strings. You might wanna consider if there is a
> simpler way to do this.

Yes it's too long-winded. But it does need to handle the four cases. I plan to do more refactoring once outstanding issues are resolved. Hopefully get around to this refactor this week.

[Mikkel Kamstrup Erlandsen (kamstrup)]

 review needsfixing

> === modified file 'src/sound-service-dbus.c'
...
> +static gboolean sound_service_dbus_blacklist_player (SoundServiceDbus* self,
> +                                                     gchar* player_name,
> +                                                     gboolean blacklist)
> +{
> +  gboolean result = FALSE;
> +  GSettings* our_settings = NULL;
> +  our_settings  = g_settings_new ("com.canonical.indicators.sound");
> +  GVariant* the_black_list = g_settings_get_value (our_settings,
> +                                                   "blacklisted-media-players");
> +  GVariantIter iter;
> +  gchar *str;
> +  // Firstly prep new array which will be set on the key.
> +  GVariantBuilder builder;
> +
> +  g_variant_iter_init (&iter, the_black_list);
> +  g_variant_builder_init(&builder, G_VARIANT_TYPE_STRING_ARRAY);
> +
> +  while (g_variant_iter_loop (&iter, "s", &str)){
> +    g_variant_builder_add (&builder, "s", str);
> +  }
> +  g_variant_iter_init (&iter, the_black_list);
> +
> +  if (blacklist == TRUE){
> +    while (g_variant_iter_loop (&iter, "s", &str)){
> +      g_print ("first pass to check if %s is present\n", str);
> +      if (g_strcmp0 (player_name, str) == 0){
> +        // Return if its already there
> +        g_debug ("we have this already blacklisted, no need to do anything");
> +        g_variant_builder_clear (&builder);
> +        g_object_unref (our_settings);
> +        g_object_unref (the_black_list);
> +        return result;
> +      }
> +    }
> +    // Otherwise blacklist it !
> +    g_debug ("about to blacklist %s", player_name);
> +    g_variant_builder_add (&builder, "s", player_name);
> +  }
> +  else{
> +    gboolean present = FALSE;
> +    g_variant_iter_init (&iter, the_black_list);
> +    g_debug ("attempting to UN-blacklist %s", player_name);
> +
> +    while (g_variant_iter_loop (&iter, "s", &str)){
> +      if (g_strcmp0 (player_name, str) == 0){
> +        present = TRUE;
> +      }
> +    }
> +    // It was not there anyway, return false
> +    if (present == FALSE){
> +      g_debug ("it was not blacklisted ?, no need to do anything");
> +      g_variant_builder_clear (&builder);
> +      g_object_unref (our_settings);
> +      g_object_unref (the_black_list);
> +      return result;
> +    }
> +
> +    // Otherwise free the builder and reconstruct ensuring no duplicates.
> +    g_variant_builder_clear (&builder);
> +    g_variant_builder_init (&builder, G_VARIANT_TYPE_STRING_ARRAY);
> +
> +    g_variant_iter_init (&iter, the_black_list);
> +
> +    while (g_variant_iter_loop (&iter, "s", &str)){
> +      if (g_strcmp0 (player_name, str) != 0){
> +        g_variant_builder_add (&builder, "s", str);
> +      }
> +    }
> +  }
> +  GVariant* value = g_variant_builder_end (&builder);
> +  result = g_settings_set_value (our_settings,
> +                                 "blacklisted-media-players",
> +                                 value);
> +
> +  g_object_unref (our_settings);
> +  g_object_unref (the_black_list);
> +
> +  return result;
> +}

There are a bunch of g_object_unref()s here that should be
g_variant_unref() instead.

review: Needs Fixing

=== modified file 'src/mpris2-watcher.vala'
--- src/mpris2-watcher.vala	2011-01-09 15:05:33 +0000
+++ src/mpris2-watcher.vala	2011-02-01 09:54:15 +0000
@@ -50,7 +50,7 @@
 
   // At startup check to see if there are clients up that we are interested in
   // More relevant for development and daemon's like mpd. 
-  private async void check_for_active_clients()
+  public async void check_for_active_clients()
   {
     string[] interfaces;
     try{

=== modified file 'src/music-player-bridge.vala'
--- src/music-player-bridge.vala	2011-01-27 23:08:08 +0000
+++ src/music-player-bridge.vala	2011-02-01 09:54:15 +0000
@@ -41,6 +41,17 @@
   private void on_blacklist_update ( string[] blacklist )
   {
     debug("some blacklist update");
+
+    foreach(var s in blacklist){
+      string key = this.determine_key (s);
+      if (this.registered_clients.has_key (key)){
+        debug ("Apparently %s is now blacklisted - remove thy self", key);
+        this.registered_clients[key].remove_from_menu();
+        this.registered_clients.unset (key);
+      }
+    }
+    // double check present players to ensure dynamic removal/addition 
+    this.watcher.check_for_active_clients.begin();
   }
 
   private void try_to_add_inactive_familiar_clients()

=== modified file 'src/player-controller.vala'
--- src/player-controller.vala	2011-01-27 23:08:08 +0000
+++ src/player-controller.vala	2011-02-01 09:54:15 +0000
@@ -119,10 +119,14 @@
     this.determine_state();
   }
   
-  public void vanish()
+  public void remove_from_menu()
   {
-    foreach(Dbusmenu.Menuitem item in this.custom_items){
-      root_menu.child_delete(item);
+    foreach(PlayerItem item in this.custom_items){
+      this.root_menu.child_delete(item);
+    }
+    if (this.use_playlists == true){
+      PlaylistsMenuitem playlists_menuitem = this.custom_items[widget_order.PLAYLISTS] as PlaylistsMenuitem;
+      this.root_menu.child_delete (playlists_menuitem.root_item);
     }
   }
 
@@ -180,7 +184,7 @@
     
     foreach(PlayerItem item in this.custom_items){
       if (this.custom_items.index_of(item) != 4) {
-        root_menu.child_add_position(item, this.menu_offset + this.custom_items.index_of(item));      
+        root_menu.child_add_position(item, this.menu_offset + this.custom_items.index_of(item));
       }
       else{
         PlaylistsMenuitem playlists_menuitem = item as PlaylistsMenuitem;
@@ -188,7 +192,7 @@
       }
     }
   }
-
+   
   private void determine_state()
   {
     if(this.mpris_bridge.connected() == true){

=== modified file 'src/playlists-menu-item.vala'
--- src/playlists-menu-item.vala	2010-12-21 14:53:56 +0000
+++ src/playlists-menu-item.vala	2011-02-01 09:54:15 +0000
@@ -50,7 +50,7 @@
       this.root_item.child_append( menuitem );
     }
   }
-
+  
   private bool already_observed (PlaylistDetails new_detail)
   {
     foreach ( PlaylistDetails detail in this.current_playlists.values ){

=== modified file 'src/settings-manager.vala'
--- src/settings-manager.vala	2011-01-10 12:57:35 +0000
+++ src/settings-manager.vala	2011-02-01 09:54:15 +0000
@@ -52,7 +52,7 @@
     this.settings.reset("interested-media-players");
   }
   
-  public void add_interested(string app_desktop_name)
+  public void add_interested (string app_desktop_name)
   {
     var already_interested = this.settings.get_strv ("interested-media-players");
     foreach (var s in already_interested){
@@ -67,5 +67,24 @@
   private void on_blacklist_event()
   {
     this.blacklist_updates(this.settings.get_strv ("blacklisted-media-players"));        
-  }  
+  }
+
+  // Convenient debug method inorder to provide visability over 
+  // the contents of both interested and blacklisted containers in its gsettings
+  private void reveal_contents()
+  {
+    var already_interested = this.settings.get_strv ("interested-media-players");
+    foreach (var s in already_interested)
+    {
+      debug ("client %s is in interested array", s);      
+    }
+    var blacklisted = this.settings.get_strv ("blacklisted-media-players");
+    foreach (var s in blacklisted)
+    {
+      debug ("client %s is in blacklisted array", s);      
+    }
+
+    debug ("interested array size = %i", already_interested.length);
+    debug ("blacklisted array size = %i", blacklisted.length);
+  }
 }

=== modified file 'src/sound-service-dbus.c'
--- src/sound-service-dbus.c	2011-01-27 22:10:48 +0000
+++ src/sound-service-dbus.c	2011-02-01 09:54:15 +0000
@@ -85,6 +85,10 @@
                                                 gboolean availability,
                                                 gboolean mute,
                                                 gdouble volume);
+static gboolean sound_service_dbus_blacklist_player (SoundServiceDbus* self,
+                                                     gchar* player_name,
+                                                     gboolean blacklist); 
+
 
 G_DEFINE_TYPE (SoundServiceDbus, sound_service_dbus, G_TYPE_OBJECT);
 
@@ -400,12 +404,102 @@
     g_debug("Get state -  %i", priv->current_sound_state );
     retval =  g_variant_new ( "(i)", priv->current_sound_state);    
   }   
+  else if (g_strcmp0(method, "BlacklistMediaPlayer") == 0) {    
+    gboolean blacklist;
+    gchar* player_name;
+    g_variant_get (params, "(sb)", &player_name, &blacklist);
+                   
+    g_debug ("BlacklistMediaPlayer - bool %i", blacklist); 
+    g_debug ("BlacklistMediaPlayer - name %s", player_name); 
+    gboolean result = sound_service_dbus_blacklist_player (service,                                                           
+                                                           player_name,
+                                                           blacklist);
+    retval =  g_variant_new ("(b)", result);
+  }     
   else {
     g_warning("Calling method '%s' on the sound service but it's unknown", method); 
   }
   g_dbus_method_invocation_return_value (invocation, retval);
 }
 
-
+static gboolean sound_service_dbus_blacklist_player (SoundServiceDbus* self,
+                                                     gchar* player_name,
+                                                     gboolean blacklist) 
+{
+  gboolean result = FALSE;
+  GSettings* our_settings = NULL;
+  our_settings  = g_settings_new ("com.canonical.indicators.sound");
+  GVariant* the_black_list = g_settings_get_value (our_settings,
+                                                   "blacklisted-media-players");
+  GVariantIter iter;
+  gchar *str;
+  // Firstly prep new array which will be set on the key.
+  GVariantBuilder builder;
+  
+  g_variant_iter_init (&iter, the_black_list);
+  g_variant_builder_init(&builder, G_VARIANT_TYPE_STRING_ARRAY);  
+
+  while (g_variant_iter_loop (&iter, "s", &str)){
+    g_variant_builder_add (&builder, "s", str);
+  }
+  g_variant_iter_init (&iter, the_black_list);
+
+  if (blacklist == TRUE){
+    while (g_variant_iter_loop (&iter, "s", &str)){
+      g_print ("first pass to check if %s is present\n", str);
+      if (g_strcmp0 (player_name, str) == 0){
+        // Return if its already there
+        g_debug ("we have this already blacklisted, no need to do anything");
+        g_variant_builder_clear (&builder);
+        g_object_unref (our_settings);
+        g_object_unref (the_black_list);
+        return result;
+      }
+    }
+    // Otherwise blacklist it !
+    g_debug ("about to blacklist %s", player_name);
+    g_variant_builder_add (&builder, "s", player_name);
+  }
+  else{
+    gboolean present = FALSE;
+    g_variant_iter_init (&iter, the_black_list);
+    g_debug ("attempting to UN-blacklist %s", player_name);
+        
+    while (g_variant_iter_loop (&iter, "s", &str)){
+      if (g_strcmp0 (player_name, str) == 0){      
+        present = TRUE;
+      }
+    }
+    // It was not there anyway, return false
+    if (present == FALSE){
+      g_debug ("it was not blacklisted ?, no need to do anything");
+      g_variant_builder_clear (&builder);
+      g_object_unref (our_settings);
+      g_object_unref (the_black_list);
+      return result;
+    }
+    
+    // Otherwise free the builder and reconstruct ensuring no duplicates.
+    g_variant_builder_clear (&builder);  
+    g_variant_builder_init (&builder, G_VARIANT_TYPE_STRING_ARRAY);  
+
+    g_variant_iter_init (&iter, the_black_list);
+    
+    while (g_variant_iter_loop (&iter, "s", &str)){
+      if (g_strcmp0 (player_name, str) != 0){            
+        g_variant_builder_add (&builder, "s", str);
+      }
+    }
+  }
+  GVariant* value = g_variant_builder_end (&builder);
+  result = g_settings_set_value (our_settings,
+                                 "blacklisted-media-players",
+                                 value);
+
+  g_object_unref (our_settings);
+  g_object_unref (the_black_list);
+  
+  return result;
+}
 
 

=== modified file 'src/sound-service.xml'
--- src/sound-service.xml	2011-01-27 22:10:48 +0000
+++ src/sound-service.xml	2011-02-01 09:54:15 +0000
@@ -2,12 +2,13 @@
 <node name="/com/canonical/indicators/sound">
   <interface name="com.canonical.indicators.sound">
     <method name = "BlacklistMediaPlayer">
-        <annotation name="org.freedesktop.DBus.GLib.CSymbol" value="sound_service_dbus_blacklist_media_player"/>
+        <annotation name="org.freedesktop.DBus.GLib.Async" value="true"/>
         <arg type='s' name='player_desktop_name' direction="in"/>
         <arg type='b' name='blacklist' direction="in"/>
+        <arg type='b' name='result' direction="out"/>
     </method>
     <method name = "GetSoundState">
-        <annotation name="org.freedesktop.DBus.GLib.CSymbol" value="sound_service_dbus_get_sink_state"/>
+        <annotation name="org.freedesktop.DBus.GLib.Async" value="true"/>
         <arg type='i' name='current_state' direction="out"/>
     </method>
     <signal name="SoundStateUpdate">