Merge lp:~3v1n0/unity/device-launcher-section-c++ into lp:unity

[Marco Trevisan (Treviño)]

DeviceLauncherSection: updated to use glib::SignalManager and glib::Object

This allow to be more safe; it fixes also some crashes caused by the fact that DeviceLauncherIcon didn't own the GVolume* object. Now it uses glib::Object completely, so we can automagically ref/unref the volume when the icon is created/removed.

[Andrea Azzarone (azzar1)]

LGTM.

review: Approve

=== modified file 'plugins/unityshell/src/DeviceLauncherIcon.cpp'
--- plugins/unityshell/src/DeviceLauncherIcon.cpp	2012-04-03 13:44:01 +0000
+++ plugins/unityshell/src/DeviceLauncherIcon.cpp	2012-04-24 19:07:27 +0000
@@ -43,7 +43,7 @@
 
 }
 
-DeviceLauncherIcon::DeviceLauncherIcon(GVolume* volume)
+DeviceLauncherIcon::DeviceLauncherIcon(glib::Object<GVolume> const& volume)
   : SimpleLauncherIcon()
   , volume_(volume)
   , device_file_(g_volume_get_identifier(volume_, G_VOLUME_IDENTIFIER_KIND_UNIX_DEVICE))

=== modified file 'plugins/unityshell/src/DeviceLauncherIcon.h'
--- plugins/unityshell/src/DeviceLauncherIcon.h	2012-04-03 13:44:01 +0000
+++ plugins/unityshell/src/DeviceLauncherIcon.h	2012-04-24 19:07:27 +0000
@@ -36,7 +36,7 @@
 {
 
 public:
-  DeviceLauncherIcon(GVolume* volume);
+  DeviceLauncherIcon(glib::Object<GVolume> const& volume);
 
   void UpdateVisibility(int visibility = -1);
   void OnRemoved();
@@ -67,7 +67,7 @@
   void ShowNotification(std::string const&, unsigned, GdkPixbuf*, std::string const&);
 
 private:
-  GVolume* volume_;
+  glib::Object<GVolume> volume_;
   glib::String device_file_;
   std::string name_;
   glib::Object<GduDevice> gdu_device_;

=== modified file 'plugins/unityshell/src/DeviceLauncherSection.cpp'
--- plugins/unityshell/src/DeviceLauncherSection.cpp	2012-03-14 06:24:18 +0000
+++ plugins/unityshell/src/DeviceLauncherSection.cpp	2012-04-24 19:07:27 +0000
@@ -25,128 +25,101 @@
 
 DeviceLauncherSection::DeviceLauncherSection()
   : monitor_(g_volume_monitor_get())
-{  
-  on_volume_added_handler_id_ = g_signal_connect(monitor_,
-                                                 "volume-added",
-                                                 G_CALLBACK(&DeviceLauncherSection::OnVolumeAdded),
-                                                 this);
-
-  on_volume_removed_handler_id_ = g_signal_connect(monitor_,
-                                                   "volume-removed",
-                                                   G_CALLBACK(&DeviceLauncherSection::OnVolumeRemoved),
-                                                   this);
-    
-  on_mount_added_handler_id_ = g_signal_connect(monitor_,
-                                                "mount-added",
-                                                G_CALLBACK(&DeviceLauncherSection::OnMountAdded),
-                                                this);
-                                                
-  on_mount_pre_unmount_handler_id_ = g_signal_connect(monitor_,
-                                                      "mount-pre-unmount",
-                                                      G_CALLBACK(&DeviceLauncherSection::OnMountPreUnmount),
-                                                      this);
-
-  on_device_populate_entry_id_ = g_idle_add((GSourceFunc)&DeviceLauncherSection::PopulateEntries, this);
+{
+  typedef glib::Signal<void, GVolumeMonitor*, GVolume*> VolumeSignal;
+  sig_manager_.Add(new VolumeSignal(monitor_, "volume-added", sigc::mem_fun(this, &DeviceLauncherSection::OnVolumeAdded)));
+  sig_manager_.Add(new VolumeSignal(monitor_, "volume-removed", sigc::mem_fun(this, &DeviceLauncherSection::OnVolumeRemoved)));
+
+  typedef glib::Signal<void, GVolumeMonitor*, GMount*> MountSignal;
+  sig_manager_.Add(new MountSignal(monitor_, "mount-added", sigc::mem_fun(this, &DeviceLauncherSection::OnMountAdded)));
+  sig_manager_.Add(new MountSignal(monitor_, "mount-pre-unmount", sigc::mem_fun(this, &DeviceLauncherSection::OnMountPreUnmount)));
+
+  on_device_populate_entry_id_ = g_idle_add([] (gpointer data) {
+    auto self = static_cast<DeviceLauncherSection*>(data);
+    self->PopulateEntries();
+    self->on_device_populate_entry_id_ = 0;
+    return FALSE;
+    }, this);
 }
 
 DeviceLauncherSection::~DeviceLauncherSection()
 {
-  if (on_volume_added_handler_id_)
-    g_signal_handler_disconnect((gpointer) monitor_,
-                                on_volume_added_handler_id_);
-
-  if (on_volume_removed_handler_id_)
-    g_signal_handler_disconnect((gpointer) monitor_,
-                                on_volume_removed_handler_id_);
-
-  if (on_mount_added_handler_id_)
-    g_signal_handler_disconnect((gpointer) monitor_,
-                                on_mount_added_handler_id_);
-
-  if (on_mount_pre_unmount_handler_id_)
-    g_signal_handler_disconnect((gpointer) monitor_,
-                                on_mount_pre_unmount_handler_id_);
-
   if (on_device_populate_entry_id_)
     g_source_remove(on_device_populate_entry_id_);
 }
 
-bool DeviceLauncherSection::PopulateEntries(DeviceLauncherSection* self)
+void DeviceLauncherSection::PopulateEntries()
 {
-  GList* volumes = g_volume_monitor_get_volumes(self->monitor_);
+  GList* volumes = g_volume_monitor_get_volumes(monitor_);
 
   for (GList* v = volumes; v; v = v->next)
   {
-    glib::Object<GVolume> volume((GVolume* )v->data);
+    if (!G_IS_VOLUME(v->data))
+      continue;
+
+    // This will unref the volume, since the list entries needs that.
+    // We'll keep a reference in the icon.
+    glib::Object<GVolume> volume(G_VOLUME(v->data));
     DeviceLauncherIcon* icon = new DeviceLauncherIcon(volume);
 
-    self->map_[volume] = icon;
-    self->IconAdded.emit(AbstractLauncherIcon::Ptr(icon));
+    map_[volume] = icon;
+    IconAdded.emit(AbstractLauncherIcon::Ptr(icon));
   }
 
   g_list_free(volumes);
-  
-  self->on_device_populate_entry_id_ = 0;
-  
-  return false;
 }
 
 /* Uses a std::map to track all the volume icons shown and not shown.
  * Keep in mind: when "volume-removed" is recevied we should erase
  * the pair (GVolume - DeviceLauncherIcon) from the std::map to avoid leaks
  */
-void DeviceLauncherSection::OnVolumeAdded(GVolumeMonitor* monitor,
-                                          GVolume* volume,
-                                          DeviceLauncherSection* self)
+void DeviceLauncherSection::OnVolumeAdded(GVolumeMonitor* monitor, GVolume* volume)
 {
-  DeviceLauncherIcon* icon = new DeviceLauncherIcon(volume);
-  
-  self->map_[volume] = icon;
-  self->IconAdded.emit(AbstractLauncherIcon::Ptr(icon));
+  // This just wraps the volume in a glib::Object, global ref_count is only
+  // temporary changed.
+  glib::Object<GVolume> gvolume(volume, glib::AddRef());
+  DeviceLauncherIcon* icon = new DeviceLauncherIcon(gvolume);
+
+  map_[gvolume] = icon;
+  IconAdded.emit(AbstractLauncherIcon::Ptr(icon));
 }
 
-void DeviceLauncherSection::OnVolumeRemoved(GVolumeMonitor* monitor,
-                                            GVolume* volume,
-                                            DeviceLauncherSection* self)
+void DeviceLauncherSection::OnVolumeRemoved(GVolumeMonitor* monitor, GVolume* volume)
 {
+  auto volume_it = map_.find(volume);
+
   // It should not happen! Let me do the check anyway.
-  if (self->map_.find(volume) != self->map_.end())
-  {	
-    self->map_[volume]->OnRemoved();
-    self->map_.erase(volume);
+  if (volume_it != map_.end())
+  {
+    volume_it->second->OnRemoved();
+    map_.erase(volume_it);
   }
 }
 
 /* Keep in mind: we could have a GMount without a related GVolume
  * so check everything to avoid unwanted behaviors.
  */
-void DeviceLauncherSection::OnMountAdded(GVolumeMonitor* monitor,
-                                         GMount* mount,
-                                         DeviceLauncherSection* self)
+void DeviceLauncherSection::OnMountAdded(GVolumeMonitor* monitor, GMount* mount)
 {
-  std::map<GVolume* , DeviceLauncherIcon* >::iterator it;
   glib::Object<GVolume> volume(g_mount_get_volume(mount));
 
-  it = self->map_.find(volume);
+  auto volume_it = map_.find(volume);
 
-  if (it != self->map_.end())
-    it->second->UpdateVisibility(1);
+  if (volume_it != map_.end())
+    volume_it->second->UpdateVisibility(1);
 }
 
 /* We don't use "mount-removed" signal since it is received after "volume-removed"
  * signal. You should read also the comment above.
  */
-void DeviceLauncherSection::OnMountPreUnmount(GVolumeMonitor* monitor,
-                                              GMount* mount,
-                                              DeviceLauncherSection* self)
+void DeviceLauncherSection::OnMountPreUnmount(GVolumeMonitor* monitor, GMount* mount)
 {
-  std::map<GVolume* , DeviceLauncherIcon* >::iterator it;
   glib::Object<GVolume> volume(g_mount_get_volume(mount));
 
-  it = self->map_.find(volume);
+  auto volume_it = map_.find(volume);
 
-  if (it != self->map_.end())
-    it->second->UpdateVisibility(0);
+  if (volume_it != map_.end())
+    volume_it->second->UpdateVisibility(0);
 }
 
 } // namespace launcher

=== modified file 'plugins/unityshell/src/DeviceLauncherSection.h'
--- plugins/unityshell/src/DeviceLauncherSection.h	2012-03-14 06:24:18 +0000
+++ plugins/unityshell/src/DeviceLauncherSection.h	2012-04-24 19:07:27 +0000
@@ -25,6 +25,7 @@
 #include <sigc++/sigc++.h>
 #include <sigc++/signal.h>
 #include <UnityCore/GLibWrapper.h>
+#include <UnityCore/GLibSignal.h>
 
 #include "DeviceLauncherIcon.h"
 
@@ -45,31 +46,17 @@
   sigc::signal<void, AbstractLauncherIcon::Ptr> IconAdded;
 
 private:
-  static bool PopulateEntries(DeviceLauncherSection* self);
-  
-  static void OnVolumeAdded(GVolumeMonitor* monitor,
-                            GVolume* volume,
-                            DeviceLauncherSection* self);
-
-  static void OnVolumeRemoved(GVolumeMonitor* monitor,
-                              GVolume* volume,
-                              DeviceLauncherSection* self);
-
-  static void OnMountAdded(GVolumeMonitor* monitor,
-                           GMount* mount,
-                           DeviceLauncherSection* self);
-
-  static void OnMountPreUnmount(GVolumeMonitor* monitor,
-                                GMount* mount,
-                                DeviceLauncherSection* self);
+  void PopulateEntries();
+
+  void OnVolumeAdded(GVolumeMonitor* monitor, GVolume* volume);
+  void OnVolumeRemoved(GVolumeMonitor* monitor, GVolume* volume);
+  void OnMountAdded(GVolumeMonitor* monitor, GMount* mount);
+  void OnMountPreUnmount(GVolumeMonitor* monitor, GMount* mount);
 
 private:
+  glib::SignalManager sig_manager_;
   glib::Object<GVolumeMonitor> monitor_;
   std::map<GVolume*, DeviceLauncherIcon*> map_;
-  gulong on_volume_added_handler_id_;
-  gulong on_volume_removed_handler_id_;
-  gulong on_mount_added_handler_id_;
-  gulong on_mount_pre_unmount_handler_id_;
   gulong on_device_populate_entry_id_;
 };