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.

[Christopher Townsend (townsend)]

LGTM. Nice tests:)

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3575
http://jenkins.qa.ubuntu.com/job/unity-ci/431/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-saucy-amd64-ci/320/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-saucy-armhf-ci/318
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-saucy-i386-ci/319

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/unity-ci/431/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'launcher/ApplicationLauncherIcon.cpp'
--- launcher/ApplicationLauncherIcon.cpp	2013-10-09 02:31:52 +0000
+++ launcher/ApplicationLauncherIcon.cpp	2013-10-10 13:14:39 +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:14:39 +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:14:39 +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:14:39 +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:14:39 +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