Merge lp:~3v1n0/unity/dual-icon-race-fix into lp:unity

[Marco Trevisan (Treviño)]

ApplicationLauncherIcon: don't unset the app if the icon has been already removed

In this case the app is unset when removed, doing it twice causes the app->seen flag to
be reset and this breaks the assumtions of the LauncherController, making it to recreate
a new app for the same BamfApplication.

We had an issue caused by the fact that we were setting the app "seen" flag
as false, both when an icon was removed and afterwards (after some seconds
of delay) when destroyed.

Doing this was wrong especially in the case where we were destroying an icon
that was already removed and recently re-used, because we were messing with
the "seen" flag of an application that was now owned by another icon.

Some cleanup, moving stuff to LauncherIcon and a bunch of new unit tests.

=== modified file 'UnityCore/GLibSource.cpp'
--- UnityCore/GLibSource.cpp	2012-09-04 10:22:15 +0000
+++ UnityCore/GLibSource.cpp	2013-10-10 13:12:25 +0000
@@ -189,10 +189,7 @@
 
 SourceManager::~SourceManager()
 {
-  for (auto it = sources_.begin(); it != sources_.end();)
-  {
-    RemoveItem(it++);
-  }
+  RemoveAll();
 }
 
 bool SourceManager::Add(Source* source, std::string const& nick)
@@ -358,6 +355,12 @@
   return false;
 }
 
+void SourceManager::RemoveAll()
+{
+  for (auto it = sources_.begin(); it != sources_.end();)
+    RemoveItem(it++);
+}
+
 void SourceManager::RemoveItem(SourcesMap::iterator it)
 {
   auto source = it->second;

=== modified file 'UnityCore/GLibSource.h'
--- UnityCore/GLibSource.h	2012-09-04 10:22:15 +0000
+++ UnityCore/GLibSource.h	2013-10-10 13:12:25 +0000
@@ -217,6 +217,7 @@
 
   bool Remove(std::string const& nick);
   bool Remove(unsigned int id);
+  void RemoveAll();
 
   Source::Ptr GetSource(std::string const& nick) const;
   Source::Ptr GetSource(unsigned int id) const;

=== modified file 'launcher/ApplicationLauncherIcon.cpp'
--- launcher/ApplicationLauncherIcon.cpp	2013-10-09 02:31:52 +0000
+++ launcher/ApplicationLauncherIcon.cpp	2013-10-10 13:12:25 +0000
@@ -39,8 +39,6 @@
 #include <glib/gi18n-lib.h>
 #include <gio/gdesktopappinfo.h>
 
-#include <libbamf/bamf-tab.h>
-
 namespace unity
 {
 namespace launcher
@@ -97,11 +95,7 @@
 
 ApplicationLauncherIcon::~ApplicationLauncherIcon()
 {
-  if (app_)
-  {
-    app_->sticky = false;
-    app_->seen = false;
-  }
+  UnsetApplication();
 }
 
 ApplicationPtr ApplicationLauncherIcon::GetApplication() const
@@ -120,12 +114,8 @@
     return;
   }
 
-  if (app_)
-  {
-    app_->sticky = false;
-    app_->seen = false;
-    signals_conn_.Clear();
-  }
+  bool was_sticky = IsSticky();
+  UnsetApplication();
 
   app_ = app;
   app_->seen = true;
@@ -140,10 +130,26 @@
   app_->desktop_file.changed.emit(app_->desktop_file());
 
   // Make sure we set the LauncherIcon stick bit too...
-  if (app_->sticky())
+  if (app_->sticky() || was_sticky)
     Stick(false); // don't emit the signal
 }
 
+void ApplicationLauncherIcon::UnsetApplication()
+{
+  if (!app_ || removed())
+    return;
+
+  /* Removing the unity-seen flag to the wrapped bamf application, on remove
+   * request we make sure that if the application is re-opened while the
+   * removal process is still ongoing, the application will be shown on the
+   * launcher. Disconnecting from signals we make sure that this icon won't be
+   * updated or will change visibility (no duplicated icon). */
+
+  signals_conn_.Clear();
+  app_->sticky = false;
+  app_->seen = false;
+}
+
 void ApplicationLauncherIcon::SetupApplicationSignalsConnections()
 {
   // Lambda functions should be fine here because when the application the icon
@@ -242,16 +248,7 @@
 void ApplicationLauncherIcon::Remove()
 {
   LogUnityEvent(ApplicationEventType::LEAVE);
-  /* Removing the unity-seen flag to the wrapped bamf application, on remove
-   * request we make sure that if the application is re-opened while the
-   * removal process is still ongoing, the application will be shown on the
-   * launcher. Disconnecting from signals we make sure that this icon won't be
-   * reused (no duplicated icon). */
-  app_->seen = false;
-  app_->sticky = false;
-  // Disconnect all our callbacks.
-  notify_callbacks(); // This is from sigc++::trackable
-  signals_conn_.Clear();
+  UnsetApplication();
   SimpleLauncherIcon::Remove();
 }
 

=== modified file 'launcher/ApplicationLauncherIcon.h'
--- launcher/ApplicationLauncherIcon.h	2013-10-07 18:35:17 +0000
+++ launcher/ApplicationLauncherIcon.h	2013-10-10 13:12:25 +0000
@@ -113,6 +113,7 @@
     ON_ALL_MONITORS = (1 << 3),
   };
 
+  void UnsetApplication();
   void SetupApplicationSignalsConnections();
   void EnsureWindowState();
   void EnsureMenuItemsWindowsReady();

=== modified file 'launcher/LauncherIcon.cpp'
--- launcher/LauncherIcon.cpp	2013-10-07 23:30:27 +0000
+++ launcher/LauncherIcon.cpp	2013-10-10 13:12:25 +0000
@@ -645,7 +645,9 @@
   _source_manager.AddTimeout(500, [this] {
     if (!std::equal(_center.begin(), _center.end(), _last_stable.begin()))
     {
-      OnCenterStabilized(_center);
+      if (!removed())
+        OnCenterStabilized(_center);
+
       _last_stable = _center;
     }
 
@@ -747,8 +749,17 @@
   if (_quicklist && _quicklist->IsVisible())
       _quicklist->Hide();
 
+  if (_tooltip && _tooltip->IsVisible())
+    _tooltip->Hide();
+
   SetQuirk(Quirk::VISIBLE, false);
   EmitRemove();
+
+  // Disconnect all the callbacks that may interact with the icon data
+  _source_manager.RemoveAll();
+  sigc::trackable::notify_callbacks();
+
+  removed = true;
 }
 
 void

=== modified file 'launcher/SoftwareCenterLauncherIcon.cpp'
--- launcher/SoftwareCenterLauncherIcon.cpp	2013-10-09 02:32:30 +0000
+++ launcher/SoftwareCenterLauncherIcon.cpp	2013-10-10 13:12:25 +0000
@@ -220,7 +220,6 @@
     // exchange the temp Application with the real one
     auto& app_manager = ApplicationManager::Default();
     auto const& new_app = app_manager.GetApplicationForDesktopFile(new_desktop_path);
-    if (new_app) new_app->sticky = IsSticky();
     SetApplication(new_app);
 
     if (new_app)

=== modified file 'tests/test_application_launcher_icon.cpp'
--- tests/test_application_launcher_icon.cpp	2013-10-03 13:34:26 +0000
+++ tests/test_application_launcher_icon.cpp	2013-10-10 13:12:25 +0000
@@ -70,6 +70,8 @@
   using ApplicationLauncherIcon::IsFileManager;
   using ApplicationLauncherIcon::LogUnityEvent;
   using ApplicationLauncherIcon::Remove;
+  using ApplicationLauncherIcon::SetApplication;
+  using ApplicationLauncherIcon::GetApplication;
 };
 
 MATCHER_P(AreArgsEqual, a, "")
@@ -149,6 +151,21 @@
     return HasMenuItemWithLabel(icon->Menus(), label);
   }
 
+  void VerifySignalsDisconnection(MockApplication::Ptr const& app)
+  {
+    EXPECT_TRUE(app->closed.empty());
+    EXPECT_TRUE(app->window_opened.empty());
+    EXPECT_TRUE(app->window_moved.empty());
+    EXPECT_TRUE(app->window_closed.empty());
+    EXPECT_TRUE(app->visible.changed.empty());
+    EXPECT_TRUE(app->active.changed.empty());
+    EXPECT_TRUE(app->running.changed.empty());
+    EXPECT_TRUE(app->urgent.changed.empty());
+    EXPECT_TRUE(app->desktop_file.changed.empty());
+    EXPECT_TRUE(app->title.changed.empty());
+    EXPECT_TRUE(app->icon.changed.empty());
+  }
+
   StandaloneWindowManager* WM;
   MockApplication::Ptr usc_app;
   MockApplication::Ptr empty_app;
@@ -166,7 +183,7 @@
     EXPECT_FALSE(app->closed.empty());
   }
 
-  EXPECT_TRUE(app->closed.empty());
+  VerifySignalsDisconnection(app);
 }
 
 TEST_F(TestApplicationLauncherIcon, Position)
@@ -1129,4 +1146,180 @@
   EXPECT_TRUE(closes_overlay);
 }
 
+TEST_F(TestApplicationLauncherIcon, RemovedPropertyOnRemove)
+{
+  ASSERT_FALSE(mock_icon->removed);
+  mock_icon->Remove();
+  EXPECT_TRUE(mock_icon->removed);
+}
+
+TEST_F(TestApplicationLauncherIcon, RemoveDisconnectsSignals)
+{
+  ASSERT_FALSE(mock_app->closed.empty());
+  mock_icon->Remove();
+  VerifySignalsDisconnection(mock_app);
+}
+
+TEST_F(TestApplicationLauncherIcon, RemoveUnsetsAppParameters)
+{
+  usc_icon->Stick();
+  ASSERT_TRUE(usc_app->seen);
+  ASSERT_TRUE(usc_app->sticky);
+
+  usc_icon->Remove();
+  EXPECT_FALSE(usc_app->seen);
+  EXPECT_FALSE(usc_app->sticky);
+}
+
+TEST_F(TestApplicationLauncherIcon, DestructionUnsetsAppParameters)
+{
+  usc_icon->Stick();
+  ASSERT_TRUE(usc_app->seen);
+  ASSERT_TRUE(usc_app->sticky);
+
+  usc_icon = nullptr;
+  EXPECT_FALSE(usc_app->seen);
+  EXPECT_FALSE(usc_app->sticky);
+}
+
+TEST_F(TestApplicationLauncherIcon, DestructionDontUnsetsAppSeenIfRemoved)
+{
+  ASSERT_TRUE(mock_app->seen);
+
+  mock_icon->Remove();
+  ASSERT_FALSE(mock_app->seen);
+
+  mock_app->seen = true;
+  mock_icon = nullptr;
+
+  EXPECT_TRUE(mock_app->seen);
+}
+
+TEST_F(TestApplicationLauncherIcon, DestructionDontUnsetsAppSeenIfReplaced)
+{
+  ASSERT_TRUE(mock_app->seen);
+
+  mock_icon->Remove();
+  ASSERT_FALSE(mock_app->seen);
+
+  MockApplicationLauncherIcon::Ptr new_icon(new NiceMock<MockApplicationLauncherIcon>(mock_app));
+  mock_icon = nullptr;
+
+  EXPECT_TRUE(mock_app->seen);
+}
+
+TEST_F(TestApplicationLauncherIcon, SetApplicationEqual)
+{
+  ASSERT_TRUE(usc_app->seen);
+  usc_icon->SetApplication(usc_app);
+  EXPECT_EQ(usc_app, usc_icon->GetApplication());
+  EXPECT_TRUE(usc_app->seen);
+  EXPECT_FALSE(usc_icon->removed);
+}
+
+TEST_F(TestApplicationLauncherIcon, SetApplicationNull)
+{
+  usc_icon->Stick();
+  ASSERT_TRUE(usc_app->seen);
+  ASSERT_TRUE(usc_app->sticky);
+
+  usc_icon->SetApplication(nullptr);
+
+  EXPECT_EQ(usc_app, usc_icon->GetApplication());
+  EXPECT_FALSE(usc_app->seen);
+  EXPECT_FALSE(usc_app->sticky);
+  EXPECT_TRUE(usc_icon->removed);
+  VerifySignalsDisconnection(usc_app);
+}
+
+TEST_F(TestApplicationLauncherIcon, SetApplicationNewUpdatesApp)
+{
+  usc_icon->Stick();
+  ASSERT_TRUE(usc_app->seen);
+  ASSERT_TRUE(usc_app->sticky);
+  ASSERT_TRUE(usc_icon->IsSticky());
+
+  auto new_app = std::make_shared<MockApplication::Nice>(UM_DESKTOP);
+  ASSERT_FALSE(new_app->seen);
+  ASSERT_FALSE(new_app->sticky);
+
+  usc_icon->SetApplication(new_app);
+
+  EXPECT_EQ(new_app, usc_icon->GetApplication());
+  EXPECT_FALSE(usc_app->seen);
+  EXPECT_FALSE(usc_app->sticky);
+  EXPECT_FALSE(usc_icon->removed);
+  VerifySignalsDisconnection(usc_app);
+
+  EXPECT_TRUE(new_app->seen);
+  EXPECT_TRUE(new_app->sticky);
+  EXPECT_TRUE(usc_icon->IsSticky());
+}
+
+TEST_F(TestApplicationLauncherIcon, SetApplicationNewUpdatesFlags)
+{
+  auto new_app = std::make_shared<MockApplication::Nice>(UM_DESKTOP, "um_icon", "um_title");
+  new_app->active_ = true;
+  new_app->visible_ = true;
+  new_app->running_ = true;
+
+  // This is needed to really make the new_app active
+  auto win = std::make_shared<MockApplicationWindow::Nice>(g_random_int());
+  new_app->windows_ = { win };
+  WM->AddStandaloneWindow(std::make_shared<StandaloneWindow>(win->window_id()));
+
+  ASSERT_NE(usc_app->title(), new_app->title());
+  ASSERT_NE(usc_app->icon(), new_app->icon());
+  ASSERT_NE(usc_app->visible(), new_app->visible());
+  ASSERT_NE(usc_app->active(), new_app->active());
+  ASSERT_NE(usc_app->running(), new_app->running());
+  ASSERT_NE(usc_app->desktop_file(), new_app->desktop_file());
+
+  usc_icon->SetApplication(new_app);
+
+  EXPECT_EQ(new_app->title(), usc_icon->tooltip_text());
+  EXPECT_EQ(new_app->icon(), usc_icon->icon_name());
+  EXPECT_EQ(new_app->visible(), usc_icon->IsVisible());
+  EXPECT_EQ(new_app->active(), usc_icon->IsActive());
+  EXPECT_EQ(new_app->running(), usc_icon->IsRunning());
+  EXPECT_EQ(new_app->desktop_file(), usc_icon->DesktopFile());
+}
+
+TEST_F(TestApplicationLauncherIcon, SetApplicationEmitsChangedSignalsInOrder)
+{
+  auto new_app = std::make_shared<MockApplication::Nice>(UM_DESKTOP, "um_icon", "um_title");
+  new_app->active_ = true;
+  new_app->visible_ = true;
+  new_app->running_ = true;
+
+  struct SignalsHandler
+  {
+    MOCK_METHOD1(TitleUpdated, void(std::string const&));
+    MOCK_METHOD1(IconUpdated, void(std::string const&));
+    MOCK_METHOD1(DesktopUpdated, void(std::string const&));
+    MOCK_METHOD1(ActiveUpdated, void(bool));
+    MOCK_METHOD1(VisibleUpdated, void(bool));
+    MOCK_METHOD1(RunningUpdated, void(bool));
+  };
+
+  SignalsHandler handler;
+  new_app->title.changed.connect(sigc::mem_fun(handler, &SignalsHandler::TitleUpdated));
+  new_app->icon.changed.connect(sigc::mem_fun(handler, &SignalsHandler::IconUpdated));
+  new_app->visible.changed.connect(sigc::mem_fun(handler, &SignalsHandler::VisibleUpdated));
+  new_app->active.changed.connect(sigc::mem_fun(handler, &SignalsHandler::ActiveUpdated));
+  new_app->running.changed.connect(sigc::mem_fun(handler, &SignalsHandler::RunningUpdated));
+  new_app->desktop_file.changed.connect(sigc::mem_fun(handler, &SignalsHandler::DesktopUpdated));
+
+  // The desktop file must be updated as last item!
+  ExpectationSet unordered_calls;
+  unordered_calls += EXPECT_CALL(handler, TitleUpdated(new_app->title()));
+  unordered_calls += EXPECT_CALL(handler, IconUpdated(new_app->icon()));
+  unordered_calls += EXPECT_CALL(handler, VisibleUpdated(new_app->visible()));
+  unordered_calls += EXPECT_CALL(handler, ActiveUpdated(new_app->active()));
+  unordered_calls += EXPECT_CALL(handler, RunningUpdated(new_app->running()));
+  EXPECT_CALL(handler, DesktopUpdated(new_app->desktop_file())).After(unordered_calls);
+
+  usc_icon->SetApplication(new_app);
+}
+
 } // anonymous namespace

=== modified file 'tests/test_glib_source.cpp'
--- tests/test_glib_source.cpp	2013-07-11 00:52:33 +0000
+++ tests/test_glib_source.cpp	2013-10-10 13:12:25 +0000
@@ -705,6 +705,23 @@
   EXPECT_FALSE(idle->IsRunning());
 }
 
+TEST(TestGLibSourceManager, DisconnectsOnRemoveAll)
+{
+  Source::Ptr timeout(new Timeout(1, &OnSourceCallbackContinue));
+  Source::Ptr idle(new Idle(&OnSourceCallbackContinue));
+
+  MockSourceManager manager;
+  manager.Add(timeout);
+  manager.Add(idle, "test-idle");
+  ASSERT_TRUE(timeout->IsRunning());
+  ASSERT_TRUE(idle->IsRunning());
+
+  manager.RemoveAll();
+  ASSERT_TRUE(manager.GetSources().empty());
+  EXPECT_FALSE(timeout->IsRunning());
+  EXPECT_FALSE(idle->IsRunning());
+}
+
 TEST(TestGLibSourceManager, RemoveSourceOnCallback)
 {
   SourceManager manager;
@@ -723,9 +740,9 @@
 
   Utils::WaitForTimeoutMSec(100);
 
-  ASSERT_EQ(idle->IsRunning(), false);
-  EXPECT_EQ(local_callback_called, true);
-  EXPECT_EQ(local_callback_call_count, 1);
+  ASSERT_FALSE(idle->IsRunning());
+  EXPECT_TRUE(local_callback_called);
+  EXPECT_EQ(1, local_callback_call_count);
 }
 
 }