Merge lp:~3v1n0/unity/unset-bamficon-on-removal into lp:unity

[Marco Trevisan (Treviño)]

BamfLauncherIcon: Unset the BamfApplication when removing the icon.

Doing this manually, we prevent that a duplicated application can be added between the removal of the BamfLauncherIcon and the effective destruction (that is controlled by a timeout in LauncherModel).

In some rare cases, when a BamfLauncherIcon is set as removed and before of being actually destroyed by the LauncherModel (that uses a timeout), it could happen that two BamfLauncherIcon's that wrap the same BamfApplication could be created. And this causes a duplicated Launcher Icon.

Unsetting the signals on removal and the BamfApplication, would avoid this.
We already have tests for duplicated icons checks. This case can't be easily reproduced.

Updated also the glib::SignalManager so that it's possible to disconnect from all signals of a given object (tests included).

[Tim Penhey (thumper)]

Looks good.

review: Approve

=== modified file 'UnityCore/GLibSignal.cpp'
--- UnityCore/GLibSignal.cpp	2012-04-16 12:12:05 +0000
+++ UnityCore/GLibSignal.cpp	2012-05-23 19:37:18 +0000
@@ -71,12 +71,10 @@
 // (it allows you to pass in a GObject without casting up).
 void SignalManager::Disconnect(void* object, std::string const& signal_name)
 {
-  for (ConnectionVector::iterator it = connections_.begin();
-       it != connections_.end();
-       ++it)
+  for (auto it = connections_.begin(); it != connections_.end(); ++it)
   {
-    if ((*it)->object() == object
-        && (*it)->name() == signal_name)
+    if ((*it)->object() == object &&
+        (signal_name.empty() || (*it)->name() == signal_name))
     {
       (*it)->Disconnect();
       connections_.erase(it, it);

=== modified file 'UnityCore/GLibSignal.h'
--- UnityCore/GLibSignal.h	2011-07-21 14:59:25 +0000
+++ UnityCore/GLibSignal.h	2012-05-23 19:37:18 +0000
@@ -295,7 +295,7 @@
 
   SignalManager();
   void Add(SignalBase* signal);
-  void Disconnect(void* object, std::string const& signal_name);
+  void Disconnect(void* object, std::string const& signal_name = "");
 
 private:
   ConnectionVector connections_;

=== modified file 'launcher/BamfLauncherIcon.cpp'
--- launcher/BamfLauncherIcon.cpp	2012-05-15 18:52:54 +0000
+++ launcher/BamfLauncherIcon.cpp	2012-05-23 19:37:18 +0000
@@ -191,9 +191,12 @@
   /* Removing the unity-seen flag to the wrapped bamf application, on remove
    * request we make sure that if the bamf application is re-opened while
    * the removal process is still ongoing, the application will be shown
-   * on the launcher. */
+   * on the launcher. Disconnecting from signals and nullifying the _bamf_app
+   * we make sure that this icon won't be reused (no duplicated icon). */
+  _gsignals.Disconnect(_bamf_app);
   g_object_set_qdata(G_OBJECT(_bamf_app.RawPtr()),
                      g_quark_from_static_string("unity-seen"), nullptr);
+  _bamf_app = nullptr;
 
   SimpleLauncherIcon::Remove();
 }
@@ -749,8 +752,8 @@
   const gchar** nicks = indicator_desktop_shortcuts_get_nicks(_desktop_shortcuts);
 
   int index = 0;
-  while (nicks[index]) {
-
+  while (nicks[index])
+  {
     // Build a dbusmenu item for each nick that is the desktop
     // file that is built from it's name and includes a callback
     // to the desktop shortcuts object to execute the nick

=== modified file 'launcher/LauncherController.cpp'
--- launcher/LauncherController.cpp	2012-05-17 19:34:56 +0000
+++ launcher/LauncherController.cpp	2012-05-23 19:37:18 +0000
@@ -130,6 +130,8 @@
 
   void OnWindowFocusChanged (guint32 xid);
 
+  void OnViewOpened(BamfMatcher* matcher, BamfView* view);
+
   void ReceiveMouseDownOutsideArea(int x, int y, unsigned long button_flags, unsigned long key_flags);
 
   void ReceiveLauncherKeyPress(unsigned long eventType,
@@ -138,17 +140,14 @@
                                const char* character,
                                unsigned short keyCount);
 
-  /* statics */
-
-  static void OnViewOpened(BamfMatcher* matcher, BamfView* view, gpointer data);
-
   Controller* parent_;
   glib::Object<BamfMatcher> matcher_;
+  glib::Signal<void, BamfMatcher*, BamfView*> view_opened_signal_;
   LauncherModel::Ptr     model_;
   nux::ObjectPtr<Launcher> launcher_;
   nux::ObjectPtr<Launcher> keyboard_launcher_;
   int                    sort_priority_;
-  DeviceLauncherSection* device_section_;
+  DeviceLauncherSection  device_section_;
   LauncherEntryRemoteModel remote_model_;
   AbstractLauncherIcon::Ptr expo_icon_;
   AbstractLauncherIcon::Ptr desktop_icon_;
@@ -157,7 +156,6 @@
   Display*               display_;
 
   guint                  bamf_timer_handler_id_;
-  guint                  on_view_opened_id_;
   guint                  launcher_key_press_handler_id_;
   guint                  launcher_label_show_handler_id_;
   guint                  launcher_hide_handler_id_;
@@ -210,9 +208,7 @@
   EnsureLaunchers(primary, monitors);
 
   launcher_ = launchers[0];
-
-  device_section_ = new DeviceLauncherSection();
-  device_section_->IconAdded.connect(sigc::mem_fun(this, &Impl::OnIconAdded));
+  device_section_.IconAdded.connect(sigc::mem_fun(this, &Impl::OnIconAdded));
 
   num_workspaces_ = WindowManager::Default()->WorkspaceCount();
   if (num_workspaces_ > 1)
@@ -289,11 +285,6 @@
 
   if (bamf_timer_handler_id_ != 0)
     g_source_remove(bamf_timer_handler_id_);
-
-  if (matcher_ != nullptr && on_view_opened_id_ != 0)
-    g_signal_handler_disconnect((gpointer) matcher_, on_view_opened_id_);
-
-  delete device_section_;
 }
 
 void Controller::Impl::EnsureLaunchers(int primary, std::vector<nux::Geometry> const& monitors)
@@ -724,15 +715,12 @@
 }
 
 /* static private */
-void Controller::Impl::OnViewOpened(BamfMatcher* matcher, BamfView* view, gpointer data)
+void Controller::Impl::OnViewOpened(BamfMatcher* matcher, BamfView* view)
 {
-  Impl* self = static_cast<Impl*>(data);
-  BamfApplication* app;
-
   if (!BAMF_IS_APPLICATION(view))
     return;
 
-  app = BAMF_APPLICATION(view);
+  BamfApplication* app = BAMF_APPLICATION(view);
 
   if (bamf_view_is_sticky(view) ||
       g_object_get_qdata(G_OBJECT(app), g_quark_from_static_string("unity-seen")))
@@ -741,10 +729,10 @@
   }
 
   AbstractLauncherIcon::Ptr icon(new BamfLauncherIcon(app));
-  icon->visibility_changed.connect(sigc::mem_fun(self, &Impl::SortAndUpdate));
-  icon->SetSortPriority(self->sort_priority_++);
-  self->RegisterIcon(icon);
-  self->SortAndUpdate();
+  icon->visibility_changed.connect(sigc::mem_fun(this, &Impl::SortAndUpdate));
+  icon->SetSortPriority(sort_priority_++);
+  RegisterIcon(icon);
+  SortAndUpdate();
 }
 
 AbstractLauncherIcon::Ptr Controller::Impl::CreateFavorite(const char* file_path)
@@ -821,7 +809,7 @@
   }
 
   apps = bamf_matcher_get_applications(matcher_);
-  on_view_opened_id_ = g_signal_connect(matcher_, "view-opened", (GCallback) &Impl::OnViewOpened, this);
+  view_opened_signal_.Connect(matcher_, "view-opened", sigc::mem_fun(this, &Impl::OnViewOpened));
 
   for (l = apps; l; l = l->next)
   {

=== modified file 'tests/test_glib_signals.cpp'
--- tests/test_glib_signals.cpp	2011-07-21 14:59:25 +0000
+++ tests/test_glib_signals.cpp	2012-05-23 19:37:18 +0000
@@ -364,4 +364,24 @@
   EXPECT_FALSE(signal0_received_);
 }
 
+TEST_F(TestGLibSignals, TestManagerObjectDisconnection)
+{
+  SignalManager manager;
+
+  manager.Add(new Signal<void, TestSignals*>(test_signals_,
+                                             "signal0",
+                                             sigc::mem_fun(this, &TestGLibSignals::Signal0Callback)));
+
+  manager.Add(new Signal<void, TestSignals*, const char*>(test_signals_,
+                                                          "signal1",
+                                                          sigc::mem_fun(this, &TestGLibSignals::Signal1Callback)));
+
+  manager.Disconnect(test_signals_);
+
+  g_signal_emit_by_name(test_signals_, "signal0");
+  EXPECT_FALSE(signal0_received_);
+  g_signal_emit_by_name(test_signals_, "signal1", "test");
+  EXPECT_FALSE(signal1_received_);
+}
+
 }