Merge lp:~3v1n0/unity/expo-terminate-fix into lp:unity

[Marco Trevisan (Treviño)]

ExpoLauncherIcon: Correctly toggle the expo on activation

PluginAdapter: change the internals of MultiActionList to use a map to reference actions by name
WindowManager: add TerminateExpo function to hide the expo plugin

The expo is not always terminated by calling again PluginAdapter::InitiateExpo().
Made some refactoring to allow to define a PluginAdapter::TerminateExpo() function that calls the expo action to close the view.

Added new tests.

[Brandon Schaefer (brandontschaefer)]

Looks good to me. Test pass, and works manually.

review: Approve

=== modified file 'launcher/ExpoLauncherIcon.cpp'
--- launcher/ExpoLauncherIcon.cpp	2012-09-11 10:59:53 +0000
+++ launcher/ExpoLauncherIcon.cpp	2012-10-01 19:11:21 +0000
@@ -41,7 +41,17 @@
 void ExpoLauncherIcon::ActivateLauncherIcon(ActionArg arg)
 {
   SimpleLauncherIcon::ActivateLauncherIcon(arg);
-  WindowManager::Default()->InitiateExpo();
+
+  auto wm = WindowManager::Default();
+
+  if (!wm->IsExpoActive())
+  {
+    wm->InitiateExpo();
+  }
+  else
+  {
+    wm->TerminateExpo();
+  }
 }
 
 void ExpoLauncherIcon::Stick(bool save)

=== modified file 'plugins/unityshell/src/unityshell.cpp'
--- plugins/unityshell/src/unityshell.cpp	2012-10-01 14:43:57 +0000
+++ plugins/unityshell/src/unityshell.cpp	2012-10-01 19:11:21 +0000
@@ -2242,17 +2242,24 @@
 
   if (p)
   {
-    MultiActionList expoActions(0);
+    MultiActionList expoActions;
 
-    foreach(CompOption & option, p->vTable->getOptions())
+    for (CompOption& option : p->vTable->getOptions())
     {
-      if (option.name() == "expo_key" ||
-          option.name() == "expo_button" ||
-          option.name() == "expo_edge")
-      {
-        CompAction* action = &option.value().action();
-        expoActions.AddNewAction(action, false);
-        break;
+      std::string const& option_name = option.name();
+
+      if (!expoActions.HasPrimary() &&
+          (option_name == "expo_key" ||
+           option_name == "expo_button" ||
+           option_name == "expo_edge"))
+      {
+        CompAction* action = &option.value().action();
+        expoActions.AddNewAction(option_name, action, true);
+      }
+      else if (option_name == "exit_button")
+      {
+        CompAction* action = &option.value().action();
+        expoActions.AddNewAction(option_name, action, false);
       }
     }
 
@@ -2263,29 +2270,31 @@
 
   if (p)
   {
-    MultiActionList scaleActions(0);
+    MultiActionList scaleActions;
 
-    foreach(CompOption & option, p->vTable->getOptions())
+    for (CompOption& option : p->vTable->getOptions())
     {
-      if (option.name() == "initiate_all_key" ||
-          option.name() == "initiate_all_edge" ||
-          option.name() == "initiate_key" ||
-          option.name() == "initiate_button" ||
-          option.name() == "initiate_edge" ||
-          option.name() == "initiate_group_key" ||
-          option.name() == "initiate_group_button" ||
-          option.name() == "initiate_group_edge" ||
-          option.name() == "initiate_output_key" ||
-          option.name() == "initiate_output_button" ||
-          option.name() == "initiate_output_edge")
+      std::string const& option_name = option.name();
+
+      if (option_name == "initiate_all_key" ||
+          option_name == "initiate_all_edge" ||
+          option_name == "initiate_key" ||
+          option_name == "initiate_button" ||
+          option_name == "initiate_edge" ||
+          option_name == "initiate_group_key" ||
+          option_name == "initiate_group_button" ||
+          option_name == "initiate_group_edge" ||
+          option_name == "initiate_output_key" ||
+          option_name == "initiate_output_button" ||
+          option_name == "initiate_output_edge")
       {
         CompAction* action = &option.value().action();
-        scaleActions.AddNewAction(action, false);
+        scaleActions.AddNewAction(option_name, action, false);
       }
-      else if (option.name() == "initiate_all_button")
+      else if (option_name == "initiate_all_button")
       {
         CompAction* action = &option.value().action();
-        scaleActions.AddNewAction(action, true);
+        scaleActions.AddNewAction(option_name, action, true);
       }
     }
 

=== modified file 'tests/autopilot/unity/tests/launcher/test_icon_behavior.py'
--- tests/autopilot/unity/tests/launcher/test_icon_behavior.py	2012-09-25 22:54:22 +0000
+++ tests/autopilot/unity/tests/launcher/test_icon_behavior.py	2012-10-01 19:11:21 +0000
@@ -15,7 +15,7 @@
 from testtools.matchers import Equals, NotEquals
 from time import sleep
 
-from unity.emulators.icons import BamfLauncherIcon
+from unity.emulators.icons import BamfLauncherIcon, ExpoLauncherIcon
 from unity.emulators.launcher import IconDragType
 from unity.tests.launcher import LauncherTestCase, _make_scenarios
 
@@ -29,6 +29,25 @@
         super(LauncherIconsTests, self).setUp()
         self.set_unity_option('launcher_hide_mode', 0)
 
+    def ensure_expo_launcher_icon(self):
+        EXPO_URI = "'unity://expo-icon'"
+        old_fav = self.call_gsettings_cmd('get', 'com.canonical.Unity.Launcher', 'favorites')
+
+        if not EXPO_URI in old_fav:
+            if old_fav[:-2] == "[]":
+                new_fav = "["+EXPO_URI+"]"
+            else:
+                new_fav = old_fav[:-1]+", "+EXPO_URI+"]"
+
+            self.addCleanup(self.call_gsettings_cmd, 'set', 'com.canonical.Unity.Launcher', 'favorites', old_fav)
+            self.call_gsettings_cmd('set', 'com.canonical.Unity.Launcher', 'favorites', new_fav)
+
+        icon = self.launcher.model.get_children_by_type(ExpoLauncherIcon)[0]
+        self.assertThat(icon, NotEquals(None))
+        self.assertThat(icon.visible, Eventually(Equals(True)))
+
+        return icon
+
     def test_bfb_tooltip_disappear_when_dash_is_opened(self):
          """Tests that the bfb tooltip disappear when the dash is opened."""
          bfb = self.launcher.model.get_bfb_icon()
@@ -155,7 +174,11 @@
         and show the quicklist.
 
         """
+        if self.workspace.num_workspaces <= 1:
+            self.skipTest("This test requires enabled workspaces.")
+
         self.keybinding("expo/start")
+        self.assertThat(self.window_manager.expo_active, Eventually(Equals(True)))
         self.addCleanup(self.keybinding, "expo/cancel")
 
         bfb = self.launcher.model.get_bfb_icon()
@@ -163,13 +186,32 @@
         self.mouse.click(button=3)
 
         self.assertThat(self.launcher_instance.quicklist_open, Eventually(Equals(True)))
-
-        monitor = self.screen_geo.get_primary_monitor()
-        self.panel = self.panels.get_panel_for_monitor(monitor)
-
-        # When workspace switcher is opened the panel title is "Ubuntu Desktop" so we check
-        # to make sure that workspace switcher end.
-        self.assertThat(self.panels.get_active_panel().title, Eventually(NotEquals("Ubuntu Desktop")))
+        self.assertThat(self.window_manager.expo_active, Eventually(Equals(False)))
+
+    def test_expo_launcher_icon_initiates_expo(self):
+        """Clicking on the expo launcher icon must start the expo."""
+        if self.workspace.num_workspaces <= 1:
+            self.skipTest("This test requires enabled workspaces.")
+
+        expo = self.ensure_expo_launcher_icon()
+        self.addCleanup(self.keybinding, "expo/cancel")
+        self.launcher_instance.click_launcher_icon(expo)
+
+        self.assertThat(self.window_manager.expo_active, Eventually(Equals(True)))
+
+    def test_expo_launcher_icon_terminates_expo(self):
+        """Clicking on the expo launcher icon when expo is active must close it."""
+        if self.workspace.num_workspaces <= 1:
+            self.skipTest("This test requires enabled workspaces.")
+
+        self.keybinding("expo/start")
+        self.assertThat(self.window_manager.expo_active, Eventually(Equals(True)))
+        self.addCleanup(self.keybinding, "expo/cancel")
+
+        expo = self.ensure_expo_launcher_icon()
+        self.launcher_instance.click_launcher_icon(expo)
+
+        self.assertThat(self.window_manager.expo_active, Eventually(Equals(False)))
 
 
 class LauncherDragIconsBehavior(LauncherTestCase):

=== modified file 'unity-shared/PluginAdapter.h'
--- unity-shared/PluginAdapter.h	2012-09-24 09:27:57 +0000
+++ unity-shared/PluginAdapter.h	2012-10-01 19:11:21 +0000
@@ -40,20 +40,23 @@
 class MultiActionList
 {
 public:
-
-  MultiActionList(int n) :
-    m_ActionList(n),
-    _primary_action(NULL) {};
-
-  void InitiateAll(CompOption::Vector& extraArgs, int state);
-  void TerminateAll(CompOption::Vector& extraArgs);
-
-  void AddNewAction(CompAction*, bool primary);
-  void RemoveAction(CompAction*);
+  MultiActionList() :
+    primary_action_(nullptr)
+  {}
+
+  void Initiate(std::string const& name, CompOption::Vector const& extra_args = CompOption::Vector(), int state = 0) const;
+  void InitiateAll(CompOption::Vector const& extra_args = CompOption::Vector(), int state = 0) const;
+  void TerminateAll(CompOption::Vector const& extra_args = CompOption::Vector()) const;
+
+  void AddNewAction(std::string const& name, CompAction*, bool primary);
+  void RemoveAction(std::string const& name);
+  bool HasPrimary() const;
+
 private:
+  CompAction* GetAction(std::string const& name) const;
 
-  std::list <CompAction*> m_ActionList;
-  CompAction*              _primary_action;
+  CompAction* primary_action_;
+  std::map<std::string, CompAction*> actions_;
 };
 
 
@@ -96,6 +99,7 @@
   bool IsScaleActiveForGroup() const;
 
   void InitiateExpo();
+  void TerminateExpo();
   bool IsExpoActive() const;
 
   bool IsWallActive() const;
@@ -174,7 +178,7 @@
   void AddProperties(GVariantBuilder* builder);
 
 private:
-  std::string MatchStringForXids(std::vector<Window> *windows);
+  std::string MatchStringForXids(std::vector<Window> const& windows);
   void InitiateScale(std::string const& match, int state = 0);
 
   bool CheckWindowIntersection(nux::Geometry const& region, CompWindow* window) const;

=== modified file 'unity-shared/PluginAdapterCompiz.cpp'
--- unity-shared/PluginAdapterCompiz.cpp	2012-09-24 10:31:04 +0000
+++ unity-shared/PluginAdapterCompiz.cpp	2012-10-01 19:11:21 +0000
@@ -62,8 +62,6 @@
 
 PluginAdapter::PluginAdapter(CompScreen* screen) :
   m_Screen(screen),
-  m_ExpoActionList(0),
-  m_ScaleActionList(0),
   _in_show_desktop (false),
   _last_focused_window(nullptr)
 {
@@ -223,70 +221,103 @@
 }
 
 void
-MultiActionList::AddNewAction(CompAction* a, bool primary)
+MultiActionList::AddNewAction(std::string const& name, CompAction* a, bool primary)
 {
-  if (std::find(m_ActionList.begin(), m_ActionList.end(), a)  == m_ActionList.end())
-    m_ActionList.push_back(a);
+  actions_[name] = a;
 
   if (primary)
-    _primary_action = a;
-}
-
-void
-MultiActionList::RemoveAction(CompAction* a)
-{
-  m_ActionList.remove(a);
-}
-
-void
-MultiActionList::InitiateAll(CompOption::Vector& extraArgs, int state)
-{
-  CompOption::Vector argument;
-  if (m_ActionList.empty())
-    return;
-
-  argument.resize(1);
+    primary_action_ = a;
+}
+
+void MultiActionList::RemoveAction(std::string const& name)
+{
+  actions_.erase(name);
+}
+
+CompAction* MultiActionList::GetAction(std::string const& name) const
+{
+  auto it = actions_.find(name);
+
+  if (it == actions_.end())
+    return nullptr;
+
+  return it->second;
+}
+
+bool MultiActionList::HasPrimary() const
+{
+  return bool(primary_action_);
+}
+
+void MultiActionList::Initiate(std::string const& name, CompOption::Vector const& extra_args, int state) const
+{
+  if (name.empty())
+    return;
+
+  CompAction* action = GetAction(name);
+
+  if (!action)
+    return;
+
+  CompOption::Vector argument(1);
   argument[0].setName("root", CompOption::TypeInt);
   argument[0].value().set((int) screen->root());
-  foreach(CompOption & arg, extraArgs)
-  {
+
+  for (CompOption const& arg : extra_args)
     argument.push_back(arg);
+
+  /* Initiate the selected action with the arguments */
+  action->initiate()(action, state, argument);
+}
+
+void MultiActionList::InitiateAll(CompOption::Vector const& extra_args, int state) const
+{
+  if (actions_.empty())
+    return;
+
+  std::string action_name;
+
+  if (primary_action_)
+  {
+    for (auto const& it : actions_)
+    {
+      if (it.second == primary_action_)
+      {
+        action_name = it.first;
+        break;
+      }
+    }
   }
-
-  CompAction* a;
-
-  if (_primary_action)
-    a = _primary_action;
   else
-    a = m_ActionList.front();
+  {
+    action_name = actions_.begin()->first;
+  }
 
-  /* Initiate the first available action with the arguments */
-  a->initiate()(a, state, argument);
+  Initiate(action_name, extra_args, state);
 }
 
-void
-MultiActionList::TerminateAll(CompOption::Vector& extraArgs)
+void MultiActionList::TerminateAll(CompOption::Vector const& extra_args) const
 {
-  CompOption::Vector argument;
-  CompOption::Value  value;
-  if (m_ActionList.empty())
+  if (actions_.empty())
     return;
 
-  argument.resize(1);
+  CompOption::Vector argument(1);
   argument[0].setName("root", CompOption::TypeInt);
   argument[0].value().set((int) screen->root());
 
-  foreach(CompOption & a, extraArgs)
-  argument.push_back(a);
+  for (CompOption const& a : extra_args)
+    argument.push_back(a);
 
-  if (_primary_action)
+  if (primary_action_)
   {
-    _primary_action->terminate()(_primary_action, 0, argument);
+    primary_action_->terminate()(primary_action_, 0, argument);
     return;
   }
 
-  foreach(CompAction * action, m_ActionList)
+  for (auto const& it : actions_)
   {
+    CompAction* action = it.second;
+
     if (action->state() & (CompAction::StateTermKey |
                            CompAction::StateTermButton |
                            CompAction::StateTermEdge |
@@ -331,31 +362,24 @@
 }
 
 std::string
-PluginAdapter::MatchStringForXids(std::vector<Window> *windows)
+PluginAdapter::MatchStringForXids(std::vector<Window> const& windows)
 {
-  std::ostringstream sout;
-
-  sout << "any & (";
-
-  std::vector<Window>::iterator it;
-  for (it = windows->begin(); it != windows->end(); ++it)
-  {
-    sout << "| xid=" << static_cast<int>(*it) << " ";
-  }
-  sout << ")";
-
-  return sout.str();
+  std::string out_string = "any & (";
+
+  for (auto const& xid : windows)
+    out_string += "| xid=" + std::to_string(xid) + " ";
+
+  out_string += ")";
+
+  return out_string;
 }
 
 void
 PluginAdapter::InitiateScale(std::string const& match, int state)
 {
-  CompOption::Vector argument;
-  CompMatch      m(match);
-
-  argument.resize(1);
+  CompOption::Vector argument(1);
   argument[0].setName("match", CompOption::TypeMatch);
-  argument[0].value().set(m);
+  argument[0].value().set(CompMatch(match));
 
   m_ScaleActionList.InitiateAll(argument, state);
 }
@@ -363,8 +387,7 @@
 void
 PluginAdapter::TerminateScale()
 {
-  CompOption::Vector argument(0);
-  m_ScaleActionList.TerminateAll(argument);
+  m_ScaleActionList.TerminateAll();
 }
 
 bool
@@ -391,12 +414,14 @@
   return m_Screen->grabExist("wall");
 }
 
-void
-PluginAdapter::InitiateExpo()
+void PluginAdapter::InitiateExpo()
 {
-  CompOption::Vector argument(0);
+  m_ExpoActionList.InitiateAll();
+}
 
-  m_ExpoActionList.InitiateAll(argument, 0);
+void PluginAdapter::TerminateExpo()
+{
+  m_ExpoActionList.Initiate("exit_button");
 }
 
 // WindowManager implementation
@@ -819,7 +844,7 @@
   std::size_t num_windows = windows.size();
   if (num_windows > 1 || (force && num_windows))
   {
-    std::string match = MatchStringForXids(&windows);
+    std::string const& match = MatchStringForXids(windows);
     InitiateScale(match, state);
     _spread_windows_state = true;
     return true;
@@ -1189,9 +1214,7 @@
   if (!_grab_show_action || !window)
     return;
 
-  CompOption::Vector argument;
-
-  argument.resize(3);
+  CompOption::Vector argument(3);
   argument[0].setName("root", CompOption::TypeInt);
   argument[0].value().set((int) screen->root());
   argument[1].setName("window", CompOption::TypeInt);
@@ -1209,9 +1232,7 @@
   if (!_grab_hide_action || !window)
     return;
 
-  CompOption::Vector argument;
-
-  argument.resize(2);
+  CompOption::Vector argument(2);
   argument[0].setName("root", CompOption::TypeInt);
   argument[0].value().set((int) screen->root());
   argument[1].setName("window", CompOption::TypeInt);
@@ -1227,9 +1248,7 @@
   if (!_grab_toggle_action || !window)
     return;
 
-  CompOption::Vector argument;
-
-  argument.resize(2);
+  CompOption::Vector argument(2);
   argument[0].setName("root", CompOption::TypeInt);
   argument[0].value().set((int) screen->root());
   argument[1].setName("window", CompOption::TypeInt);

=== modified file 'unity-shared/PluginAdapterStandalone.cpp'
--- unity-shared/PluginAdapterStandalone.cpp	2012-09-10 22:44:29 +0000
+++ unity-shared/PluginAdapterStandalone.cpp	2012-10-01 19:11:21 +0000
@@ -52,8 +52,6 @@
 
 PluginAdapter::PluginAdapter(CompScreen* screen)
   : m_Screen(screen)
-  , m_ExpoActionList(0)
-  , m_ScaleActionList(0)
   , _expo_state(false)
   , _in_show_desktop(false)
   , _last_focused_window(nullptr)
@@ -106,24 +104,37 @@
 }
 
 void
-MultiActionList::AddNewAction(CompAction* a, bool primary)
-{
-}
-
-void
-MultiActionList::RemoveAction(CompAction* a)
-{
-}
-
-void
-MultiActionList::InitiateAll(CompOption::Vector& extraArgs, int state)
-{
-}
-
-void
-MultiActionList::TerminateAll(CompOption::Vector& extraArgs)
-{
-}
+MultiActionList::AddNewAction(std::string const& n, CompAction* a, bool primary)
+{
+}
+
+void
+MultiActionList::RemoveAction(std::string const& n)
+{
+}
+
+void
+MultiActionList::InitiateAll(CompOption::Vector const& extraArgs, int state) const
+{
+}
+
+void
+MultiActionList::TerminateAll(CompOption::Vector const& extraArgs) const
+{
+}
+
+bool
+MultiActionList::HasPrimary() const
+{
+  return false;
+}
+
+CompAction*
+MultiActionList::GetAction(std::string const& name) const
+{
+  return nullptr;
+}
+
 
 unsigned long long
 PluginAdapter::GetWindowActiveNumber (guint32 xid) const
@@ -142,7 +153,7 @@
 }
 
 std::string
-PluginAdapter::MatchStringForXids(std::vector<Window> *windows)
+PluginAdapter::MatchStringForXids(std::vector<Window> const& windows)
 {
   return "";
 }
@@ -178,7 +189,13 @@
 void
 PluginAdapter::InitiateExpo()
 {
-  _expo_state = !_expo_state;
+  _expo_state = true;
+}
+
+void
+PluginAdapter::TerminateExpo()
+{
+  _expo_state = false;
 }
 
 bool

=== modified file 'unity-shared/WindowManager.cpp'
--- unity-shared/WindowManager.cpp	2012-09-10 22:44:29 +0000
+++ unity-shared/WindowManager.cpp	2012-10-01 19:11:21 +0000
@@ -214,6 +214,11 @@
     g_debug("%s", G_STRFUNC);
   }
 
+  void TerminateExpo()
+  {
+    g_debug("%s", G_STRFUNC);
+  }
+
   bool IsExpoActive() const
   {
     g_debug("%s", G_STRFUNC);

=== modified file 'unity-shared/WindowManager.h'
--- unity-shared/WindowManager.h	2012-09-10 22:44:29 +0000
+++ unity-shared/WindowManager.h	2012-10-01 19:11:21 +0000
@@ -79,6 +79,7 @@
   virtual bool IsScaleActiveForGroup() const = 0;
 
   virtual void InitiateExpo() = 0;
+  virtual void TerminateExpo() = 0;
   virtual bool IsExpoActive() const = 0;
 
   virtual bool IsWallActive() const = 0;