Merge lp:~3v1n0/unity/switcher-better-active-app-check into lp:unity

[Marco Trevisan (Treviño)]

ApplicationLauncherIcon: add a custom implementation of GetQuirk for Quirk::ACTIVE

We need to make sure that an application is set as active also if the WindowManager active window
is contained in the application windows list.

Due to the dbus nature of the bamfdaemon, it can happen that when quickly switching the application focus through Alt+Tab the unity active application is updated with some delay. This can lead to a wrong Alt+Tab behavior (see screencast on attached bug).

So, we need to ensure that an application is considered active when both BAMF has marked it as active and the active Window pointed by the WindowManager is a child of the application.

Made some minor cleanup of SwitcherModel, added new tests.

[Brandon Schaefer (brandontschaefer)]

LGTM.

review: Approve

=== modified file 'launcher/ApplicationLauncherIcon.cpp'
--- launcher/ApplicationLauncherIcon.cpp	2013-01-22 01:28:32 +0000
+++ launcher/ApplicationLauncherIcon.cpp	2013-02-05 02:25:24 +0000
@@ -173,6 +173,31 @@
   }
 }
 
+bool ApplicationLauncherIcon::GetQuirk(AbstractLauncherIcon::Quirk quirk) const
+{
+  if (quirk == Quirk::ACTIVE)
+  {
+    if (!SimpleLauncherIcon::GetQuirk(Quirk::ACTIVE))
+      return false;
+
+    if (app_->type() == "webapp")
+      return true;
+
+    // Sometimes BAMF is not fast enough to update the active application
+    // while quickly switching between apps, so we double check that the
+    // real active window is part of the selection (see bug #1111620)
+    Window active = WindowManager::Default().GetActiveWindow();
+
+    for (auto& window : app_->GetWindows())
+      if (window->window_id() == active)
+        return true;
+
+    return false;
+  }
+
+  return SimpleLauncherIcon::GetQuirk(quirk);
+}
+
 void ApplicationLauncherIcon::Remove()
 {
   /* Removing the unity-seen flag to the wrapped bamf application, on remove

=== modified file 'launcher/ApplicationLauncherIcon.h'
--- launcher/ApplicationLauncherIcon.h	2013-01-22 01:28:32 +0000
+++ launcher/ApplicationLauncherIcon.h	2013-02-05 02:25:24 +0000
@@ -51,6 +51,8 @@
   bool IsRunning() const;
   bool IsUrgent() const;
 
+  virtual bool GetQuirk(Quirk quirk) const;
+
   virtual void Quit();
   virtual void AboutToRemove();
 

=== modified file 'launcher/SwitcherController.cpp'
--- launcher/SwitcherController.cpp	2013-02-01 03:21:51 +0000
+++ launcher/SwitcherController.cpp	2013-02-05 02:25:24 +0000
@@ -547,17 +547,16 @@
   if (model_)
   {
     application = model_->Selection();
+
     if (application)
     {
       if (model_->detail_selection)
       {
         window = model_->DetailSelectionWindow();
       }
-      else if (application->GetQuirk(AbstractLauncherIcon::Quirk::ACTIVE))
+      else if (model_->SelectionIsActive())
       {
-        auto const& xids = model_->DetailXids();
-        if (!xids.empty())
-          window = xids.front();
+        window = model_->DetailXids().front();
       }
     }
   }

=== modified file 'launcher/SwitcherModel.cpp'
--- launcher/SwitcherModel.cpp	2013-02-01 02:56:13 +0000
+++ launcher/SwitcherModel.cpp	2013-02-05 02:25:24 +0000
@@ -49,7 +49,7 @@
 
 SwitcherModel::~SwitcherModel()
 {
-  for (auto application : applications_)
+  for (auto const& application : applications_)
   {
     RemoveChild(application.GetPointer());
   }
@@ -71,69 +71,64 @@
   .add("last-selection-index", LastSelectionIndex());
 }
 
-SwitcherModel::iterator
-SwitcherModel::begin()
+SwitcherModel::iterator SwitcherModel::begin()
 {
   return applications_.begin();
 }
 
-SwitcherModel::iterator
-SwitcherModel::end()
+SwitcherModel::iterator SwitcherModel::end()
 {
   return applications_.end();
 }
 
-SwitcherModel::reverse_iterator
-SwitcherModel::rbegin()
+SwitcherModel::reverse_iterator SwitcherModel::rbegin()
 {
   return applications_.rbegin();
 }
 
-SwitcherModel::reverse_iterator
-SwitcherModel::rend()
+SwitcherModel::reverse_iterator SwitcherModel::rend()
 {
   return applications_.rend();
 }
 
-AbstractLauncherIcon::Ptr
-SwitcherModel::at(unsigned int index)
+AbstractLauncherIcon::Ptr SwitcherModel::at(unsigned int index) const
 {
   if ((int) index >= Size ())
     return AbstractLauncherIcon::Ptr();
   return applications_[index];
 }
 
-int
-SwitcherModel::Size()
+int SwitcherModel::Size() const
 {
   return applications_.size();
 }
 
-AbstractLauncherIcon::Ptr
-SwitcherModel::Selection()
+AbstractLauncherIcon::Ptr SwitcherModel::Selection() const
 {
   return applications_.at(index_);
 }
 
-int
-SwitcherModel::SelectionIndex()
+int SwitcherModel::SelectionIndex() const
 {
   return index_;
 }
 
-AbstractLauncherIcon::Ptr
-SwitcherModel::LastSelection()
+bool SwitcherModel::SelectionIsActive() const
+{
+  return Selection()->GetQuirk(AbstractLauncherIcon::Quirk::ACTIVE);
+}
+
+AbstractLauncherIcon::Ptr SwitcherModel::LastSelection() const
 {
   return applications_.at(last_index_);
 }
 
-int SwitcherModel::LastSelectionIndex()
+int SwitcherModel::LastSelectionIndex() const
 {
   return last_index_;
 }
 
-
-std::vector<Window> SwitcherModel::DetailXids()
+std::vector<Window> SwitcherModel::DetailXids() const
 {
   WindowManager& wm = WindowManager::Default();
   std::vector<Window> results;
@@ -159,7 +154,7 @@
   return results;
 }
 
-Window SwitcherModel::DetailSelectionWindow()
+Window SwitcherModel::DetailSelectionWindow() const
 {
   auto windows = DetailXids();
   if (!detail_selection || windows.empty())
@@ -209,7 +204,7 @@
     detail_selection_index = 0;
 }
 
-void SwitcherModel::PrevDetail ()
+void SwitcherModel::PrevDetail()
 {
   if (!detail_selection())
     return;
@@ -242,8 +237,7 @@
   }
 }
 
-void
-SwitcherModel::Select(unsigned int index)
+void SwitcherModel::Select(unsigned int index)
 {
   unsigned int target = CLAMP(index, 0, applications_.size() - 1);
 

=== modified file 'launcher/SwitcherModel.h'
--- launcher/SwitcherModel.h	2013-01-15 22:46:58 +0000
+++ launcher/SwitcherModel.h	2013-02-05 02:25:24 +0000
@@ -70,18 +70,19 @@
   reverse_iterator rbegin();
   reverse_iterator rend();
 
-  launcher::AbstractLauncherIcon::Ptr at(unsigned int index);
-
-  int Size();
-
-  launcher::AbstractLauncherIcon::Ptr Selection();
-  int SelectionIndex();
-
-  launcher::AbstractLauncherIcon::Ptr LastSelection();
-  int LastSelectionIndex();
-
-  std::vector<Window> DetailXids ();
-  Window DetailSelectionWindow ();
+  launcher::AbstractLauncherIcon::Ptr at(unsigned int index) const;
+
+  int Size() const;
+
+  launcher::AbstractLauncherIcon::Ptr Selection() const;
+  int SelectionIndex() const;
+  bool SelectionIsActive() const;
+
+  launcher::AbstractLauncherIcon::Ptr LastSelection() const;
+  int LastSelectionIndex() const;
+
+  std::vector<Window> DetailXids() const;
+  Window DetailSelectionWindow() const;
 
   void Next();
   void Prev();

=== modified file 'tests/mock-application.h'
--- tests/mock-application.h	2012-12-19 22:34:22 +0000
+++ tests/mock-application.h	2013-02-05 02:25:24 +0000
@@ -25,9 +25,45 @@
 
 namespace testmocks
 {
-class MockApplication : public unity::Application
-{
-public:
+struct MockApplicationWindow : unity::ApplicationWindow
+{
+  MockApplicationWindow(Window xid)
+    : xid_(xid)
+    , monitor_(0)
+    , type_("window")
+    , visible_(true)
+    , active_(false)
+    , urgent_(false)
+  {
+    visible.SetGetterFunction([this] { return visible_; });
+    active.SetGetterFunction([this] { return active_; });
+    urgent.SetGetterFunction([this] { return urgent_; });
+  }
+
+  Window xid_;
+  int monitor_;
+  std::string title_;
+  std::string icon_;
+  std::string type_;
+
+  bool visible_;
+  bool active_;
+  bool urgent_;
+
+  virtual std::string title() const { return title_; }
+  virtual std::string icon() const { return icon_; }
+  virtual std::string type() const { return type_; }
+
+  virtual Window window_id() const { return xid_; }
+  virtual int monitor() const { return monitor_; }
+
+  virtual unity::ApplicationPtr application() const { return unity::ApplicationPtr(); }
+  virtual bool Focus() const { return false; }
+  virtual void Quit() const {}
+};
+
+struct MockApplication : unity::Application
+{
   MockApplication(std::string const& desktop_file,
                   std::string const& icon = "",
                   std::string const& title = "")
@@ -51,6 +87,7 @@
       urgent.SetGetterFunction(sigc::mem_fun(this, &MockApplication::GetUrgent));
     }
 
+  unity::WindowList window_list_;
   std::string desktop_file_;
   std::string icon_;
   std::string title_;
@@ -67,7 +104,7 @@
   virtual std::string type() const { return "mock"; }
   virtual std::string repr() const { return "MockApplication"; }
 
-  virtual unity::WindowList GetWindows() const { return unity::WindowList(); }
+  virtual unity::WindowList GetWindows() const { return window_list_; }
   virtual bool OwnsWindow(Window window_id) const { return false; }
 
   virtual std::vector<std::string> GetSupportedMimeTypes() const { return {}; }
@@ -98,6 +135,14 @@
     return false;
   }
 
+  void SetActiveState(bool state)
+  {
+    if (active_ == state)
+      return;
+    active_ = state;
+    active.changed.emit(state);
+  }
+
   bool GetVisible() const { return visible_; }
   bool GetActive() const { return active_; }
   bool GetRunning() const { return running_; }

=== modified file 'tests/test_application_launcher_icon.cpp'
--- tests/test_application_launcher_icon.cpp	2012-11-23 04:05:35 +0000
+++ tests/test_application_launcher_icon.cpp	2013-02-05 02:25:24 +0000
@@ -28,7 +28,7 @@
 #include "ApplicationLauncherIcon.h"
 #include "FavoriteStore.h"
 #include "mock-application.h"
-
+#include "StandaloneWindowManager.h"
 
 using namespace unity;
 using namespace testmocks;
@@ -45,6 +45,7 @@
 public:
   virtual void SetUp()
   {
+    WM = dynamic_cast<StandaloneWindowManager*>(&WindowManager::Default());
     usc_app.reset(new MockApplication(USC_DESKTOP, "softwarecenter"));
     usc_icon = new launcher::ApplicationLauncherIcon(usc_app);
     ASSERT_EQ(usc_icon->DesktopFile(), USC_DESKTOP);
@@ -58,6 +59,7 @@
     ASSERT_TRUE(mock_icon->DesktopFile().empty());
   }
 
+  StandaloneWindowManager* WM;
   std::shared_ptr<MockApplication> usc_app;
   std::shared_ptr<MockApplication> empty_app;
   std::shared_ptr<MockApplication> mock_app;
@@ -165,4 +167,17 @@
   EXPECT_EQ(mock_icon->icon_name(), "icon-name");
 }
 
+TEST_F(TestApplicationLauncherIcon, ActiveQuirkWMCrossCheck)
+{
+  auto win = std::make_shared<MockApplicationWindow>(g_random_int());
+  mock_app->window_list_ = { win };
+  ASSERT_FALSE(mock_icon->IsActive());
+
+  mock_app->SetActiveState(true);
+  ASSERT_FALSE(mock_icon->IsActive());
+
+  WM->AddStandaloneWindow(std::make_shared<StandaloneWindow>(win->window_id()));
+  EXPECT_TRUE(mock_icon->IsActive());
+}
+
 }

=== modified file 'tests/test_switcher_model.cpp'
--- tests/test_switcher_model.cpp	2012-12-18 00:11:20 +0000
+++ tests/test_switcher_model.cpp	2013-02-05 02:25:24 +0000
@@ -147,4 +147,15 @@
   EXPECT_EQ(sorted, unsorted);
 }
 
+TEST_F(TestSwitcherModel, SelectionIsActive)
+{
+  SwitcherModel model(icons_);
+
+  model.Selection()->SetQuirk(AbstractLauncherIcon::Quirk::ACTIVE, false);
+  EXPECT_FALSE(model.SelectionIsActive());
+
+  model.Selection()->SetQuirk(AbstractLauncherIcon::Quirk::ACTIVE, true);
+  EXPECT_TRUE(model.SelectionIsActive());
+}
+
 }

=== modified file 'unity-shared/BamfApplicationManager.cpp'
--- unity-shared/BamfApplicationManager.cpp	2013-01-09 19:41:24 +0000
+++ unity-shared/BamfApplicationManager.cpp	2013-02-05 02:25:24 +0000
@@ -68,7 +68,6 @@
 bool View::GetActive() const
 {
   return bamf_view_is_active(bamf_view_);
-
 }
 
 bool View::GetRunning() const