Merge lp:~3v1n0/unity/icon-activation-overlay-focus-fix into lp:unity

[Marco Trevisan (Treviño)]

SimpleLauncherIcon: we need to restore the focus when closing the Overlay for activation

Always make the Dash/Hud restore the input focus when hidden, also don't
save it when showing it, giving the focus back to the default window
is just enough.

This fixes a focus issue we had when clicking on a launcher icon when
an overlay was opened.

Added both unit and AP tests.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3558
http://jenkins.qa.ubuntu.com/job/unity-ci/417/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-saucy-amd64-ci/306
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-saucy-armhf-ci/304
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-saucy-i386-ci/305

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-ci/417/rebuild

review: Approve (continuous-integration)

[Brandon Schaefer (brandontschaefer)]

LGTM

review: Approve

=== modified file 'dash/DashController.cpp'
--- dash/DashController.cpp	2013-09-19 00:03:28 +0000
+++ dash/DashController.cpp	2013-10-03 13:43:44 +0000
@@ -105,8 +105,7 @@
     }
   });
 
-  auto spread_cb = sigc::bind(sigc::mem_fun(this, &Controller::HideDash), true);
-  WindowManager::Default().initiate_spread.connect(spread_cb);
+  WindowManager::Default().initiate_spread.connect(sigc::mem_fun(this, &Controller::HideDash));
 
   dbus_server_.AddObjects(dbus::INTROSPECTION, dbus::PATH);
   dbus_server_.GetObjects().front()->SetMethodsCallsHandler([this] (std::string const& method, GVariant*) {
@@ -174,7 +173,7 @@
     // hide if something else is coming up
     if (overlay_identity.Str() != "dash")
     {
-      HideDash(true);
+      HideDash();
     }
   });
 
@@ -276,16 +275,7 @@
 
 void Controller::OnExternalHideDash(GVariant* variant)
 {
-  EnsureDash();
-
-  if (variant)
-  {
-    HideDash(g_variant_get_boolean(variant));
-  }
-  else
-  {
-    HideDash();
-  }
+  HideDash();
 }
 
 void Controller::ShowDash()
@@ -340,14 +330,14 @@
   nux::GetWindowCompositor().SetKeyFocusArea(view_->default_focus());
 }
 
-void Controller::QuicklyHideDash(bool restore)
+void Controller::QuicklyHideDash()
 {
-  HideDash(restore);
+  HideDash();
   timeline_animator_.Stop();
   window_->ShowWindow(false);
 }
 
-void Controller::HideDash(bool restore)
+void Controller::HideDash()
 {
   if (!visible_)
    return;
@@ -362,10 +352,8 @@
   window_->EnableInputWindow(false, dash::window_title, true, false);
   visible_ = false;
 
-  nux::GetWindowCompositor().SetKeyFocusArea(NULL,nux::KEY_NAV_NONE);
-
-  if (restore)
-    WindowManager::Default().RestoreInputFocus();
+  nux::GetWindowCompositor().SetKeyFocusArea(NULL, nux::KEY_NAV_NONE);
+  WindowManager::Default().RestoreInputFocus();
 
   StartShowHideTimeline();
 

=== modified file 'dash/DashController.h'
--- dash/DashController.h	2013-09-16 17:40:56 +0000
+++ dash/DashController.h	2013-10-03 13:43:44 +0000
@@ -59,8 +59,8 @@
 
   sigc::signal<void> on_realize;
 
-  void HideDash(bool restore_focus = true);
-  void QuicklyHideDash(bool restore_focus = true);
+  void HideDash();
+  void QuicklyHideDash();
   void ShowDash();
 
   void ReFocusKeyInput();

=== modified file 'dash/DashView.cpp'
--- dash/DashView.cpp	2013-09-24 20:18:42 +0000
+++ dash/DashView.cpp	2013-10-03 13:43:44 +0000
@@ -1401,7 +1401,7 @@
   {
     if (preview_displaying_)
       ClosePreview();
-    else if (search_bar_->search_string != "")
+    else if (!search_bar_->search_string().empty())
       search_bar_->search_string = "";
     else
       ubus_manager_.SendMessage(UBUS_OVERLAY_CLOSE_REQUEST);

=== modified file 'hud/HudController.cpp'
--- hud/HudController.cpp	2013-09-13 01:36:20 +0000
+++ hud/HudController.cpp	2013-10-03 13:43:44 +0000
@@ -65,7 +65,7 @@
   // As a default. the create_window_ function should just create a base window.
   if (create_window_ == nullptr)
   {
-    create_window_ = [&]() {
+    create_window_ = [this]() {
       return new ResizingBaseWindow("Hud",
                                     [this](nux::Geometry const& geo) {
                                       if (view_)
@@ -76,14 +76,14 @@
   }
 
   SetupWindow();
-  UScreen::GetDefault()->changed.connect([&] (int, std::vector<nux::Geometry>&) { Relayout(true); });
+  UScreen::GetDefault()->changed.connect([this] (int, std::vector<nux::Geometry>&) { Relayout(true); });
 
   ubus.RegisterInterest(UBUS_HUD_CLOSE_REQUEST, sigc::mem_fun(this, &Controller::OnExternalHideHud));
 
   //!!FIXME!! - just hijacks the dash close request so we get some more requests than normal,
   ubus.RegisterInterest(UBUS_OVERLAY_CLOSE_REQUEST, sigc::mem_fun(this, &Controller::OnExternalHideHud));
 
-  ubus.RegisterInterest(UBUS_OVERLAY_SHOWN, [&] (GVariant *data) {
+  ubus.RegisterInterest(UBUS_OVERLAY_SHOWN, [this] (GVariant *data) {
     unity::glib::String overlay_identity;
     gboolean can_maximise = FALSE;
     gint32 overlay_monitor = 0;
@@ -92,15 +92,15 @@
 
     if (overlay_identity.Str() != "hud")
     {
-      HideHud(true);
+      HideHud();
     }
   });
 
-  launcher_width.changed.connect([&] (int new_width) { Relayout(); });
+  launcher_width.changed.connect([this] (int) { Relayout(); });
 
   WindowManager& wm = WindowManager::Default();
   wm.screen_ungrabbed.connect(sigc::mem_fun(this, &Controller::OnScreenUngrabbed));
-  wm.initiate_spread.connect(sigc::bind(sigc::mem_fun(this, &Controller::HideHud), true));
+  wm.initiate_spread.connect(sigc::mem_fun(this, &Controller::HideHud));
 
   hud_service_.queries_updated.connect(sigc::mem_fun(this, &Controller::OnQueriesFinished));
   timeline_animator_.updated.connect(sigc::mem_fun(this, &Controller::OnViewShowHideFrame));
@@ -289,22 +289,13 @@
 void Controller::OnExternalHideHud(GVariant* variant)
 {
   LOG_DEBUG(logger) << "External Hiding the hud";
-  EnsureHud();
-
-  if (variant)
-  {
-    HideHud(g_variant_get_boolean(variant));
-  }
-  else
-  {
-    HideHud();
-  }
+  HideHud();
 }
 
 void Controller::ShowHideHud()
 {
   EnsureHud();
-  visible_ ? HideHud(true) : ShowHud();
+  visible_ ? HideHud() : ShowHud();
 }
 
 void Controller::ReFocusKeyInput()
@@ -364,8 +355,6 @@
 
   LOG_DEBUG(logger) << "Taking application icon: " << focused_app_icon_;
   SetIcon(focused_app_icon_);
-
-  WindowManager::Default().SaveInputFocus();
   FocusWindow();
 
   view_->ResetToDefault();
@@ -402,10 +391,10 @@
   window_->QueueDraw();
 }
 
-void Controller::HideHud(bool restore)
+void Controller::HideHud()
 {
   LOG_DEBUG (logger) << "hiding the hud";
-  if (visible_ == false)
+  if (!visible_)
     return;
 
   need_show_ = false;
@@ -415,14 +404,9 @@
   window_->EnableInputWindow(false, "Hud", true, false);
   visible_ = false;
 
-  nux::GetWindowCompositor().SetKeyFocusArea(NULL,nux::KEY_NAV_NONE);
-
+  WindowManager::Default().RestoreInputFocus();
+  nux::GetWindowCompositor().SetKeyFocusArea(NULL, nux::KEY_NAV_NONE);
   StartShowHideTimeline();
-
-  restore = true;
-  if (restore)
-    WindowManager::Default().RestoreInputFocus();
-
   hud_service_.CloseQuery();
 
   //unhide the launcher

=== modified file 'hud/HudController.h'
--- hud/HudController.h	2012-12-13 03:17:19 +0000
+++ hud/HudController.h	2013-10-03 13:43:44 +0000
@@ -60,7 +60,7 @@
 
   void ShowHideHud();
   void ShowHud();
-  void HideHud(bool restore_focus = true);
+  void HideHud();
   void ReFocusKeyInput();
   bool IsVisible();
 

=== modified file 'launcher/SimpleLauncherIcon.cpp'
--- launcher/SimpleLauncherIcon.cpp	2013-07-29 16:35:54 +0000
+++ launcher/SimpleLauncherIcon.cpp	2013-10-03 13:43:44 +0000
@@ -49,7 +49,7 @@
 
 void SimpleLauncherIcon::ActivateLauncherIcon(ActionArg arg)
 {
-  UBusManager::SendMessage(UBUS_OVERLAY_CLOSE_REQUEST, g_variant_new_boolean(FALSE));
+  UBusManager::SendMessage(UBUS_OVERLAY_CLOSE_REQUEST);
 }
 
 nux::BaseTexture* SimpleLauncherIcon::GetTextureForSize(int size)

=== modified file 'plugins/unityshell/src/unityshell.cpp'
--- plugins/unityshell/src/unityshell.cpp	2013-10-01 02:17:09 +0000
+++ plugins/unityshell/src/unityshell.cpp	2013-10-03 13:43:44 +0000
@@ -1514,7 +1514,7 @@
           }
           else
           {
-            dash_controller_->HideDash(false);
+            dash_controller_->HideDash();
           }
         }
       }
@@ -1529,7 +1529,7 @@
 
         if (!hud_geo.IsInside(pt) && !DoesPointIntersectUnityGeos(pt) && !on_top_geo.IsInside(pt))
         {
-          hud_controller_->HideHud(false);
+          hud_controller_->HideHud();
         }
       }
       else if (switcher_controller_->Visible())

=== modified file 'tests/autopilot/unity/tests/launcher/test_icon_behavior.py'
--- tests/autopilot/unity/tests/launcher/test_icon_behavior.py	2013-05-02 15:51:41 +0000
+++ tests/autopilot/unity/tests/launcher/test_icon_behavior.py	2013-10-03 13:43:44 +0000
@@ -90,11 +90,10 @@
         """Shift+Clicking MUST open a new instance of an already-running application."""
         app = self.process_manager.start_app("Calculator")
         icon = self.unity.launcher.model.get_icon(desktop_id=app.desktop_file)
-        launcher_instance = self.unity.launcher.get_launcher_for_monitor(0)
 
         self.keyboard.press("Shift")
         self.addCleanup(self.keyboard.release, "Shift")
-        launcher_instance.click_launcher_icon(icon)
+        self.launcher_instance.click_launcher_icon(icon)
 
         self.assertNumberWinsIsEventually(app, 2)
 
@@ -326,6 +325,37 @@
         self.launcher_instance.click_launcher_icon(icon)
         self.assertThat(lambda: window.x_win.get_wm_state()['state'], Eventually(Equals(Xutil.NormalState)))
 
+    def test_icon_focuses_application_window_when_dash_is_open(self):
+        """Clicking on an icon when dash is shown must focus it."""
+        win = self.process_manager.start_app_window("Calculator")
+        self.assertThat(lambda: win.is_focused, Eventually(Equals(True)))
+
+        self.addCleanup(self.unity.dash.ensure_hidden)
+        self.unity.dash.ensure_visible()
+        self.assertThat(lambda: win.is_focused, Eventually(Equals(False)))
+
+        icon = self.unity.launcher.model.get_icon(desktop_id=win.application.desktop_file)
+        self.launcher_instance.click_launcher_icon(icon)
+
+        self.assertThat(self.unity.dash.visible, Eventually(Equals(False)))
+        self.assertThat(lambda: win.is_focused, Eventually(Equals(True)))
+
+    def test_icon_focuses_application_window_when_hud_is_open(self):
+        """Clicking on an icon when hud is shown must focus it."""
+        win = self.process_manager.start_app_window("Calculator")
+        self.assertThat(lambda: win.is_focused, Eventually(Equals(True)))
+
+        self.addCleanup(self.unity.hud.ensure_hidden)
+        self.unity.hud.ensure_visible()
+        self.assertThat(lambda: win.is_focused, Eventually(Equals(False)))
+
+        icon = self.unity.launcher.model.get_icon(desktop_id=win.application.desktop_file)
+        self.launcher_instance.click_launcher_icon(icon)
+
+        self.assertThat(self.unity.hud.visible, Eventually(Equals(False)))
+        self.assertThat(lambda: win.is_focused, Eventually(Equals(True)))
+
+
 class LauncherDragIconsBehavior(LauncherTestCase):
     """Tests dragging icons around the Launcher."""
 

=== modified file 'tests/test_application_launcher_icon.cpp'
--- tests/test_application_launcher_icon.cpp	2013-09-11 10:45:10 +0000
+++ tests/test_application_launcher_icon.cpp	2013-10-03 13:43:44 +0000
@@ -27,6 +27,8 @@
 #include "ApplicationLauncherIcon.h"
 #include "FavoriteStore.h"
 #include "StandaloneWindowManager.h"
+#include "UBusWrapper.h"
+#include "UBusMessages.h"
 #include "ZeitgeistUtils.h"
 #include "mock-application.h"
 #include "test_utils.h"
@@ -63,6 +65,7 @@
   MOCK_CONST_METHOD0(GetRemoteMenus, glib::Object<DbusmenuMenuitem>());
 
   bool LauncherIconIsSticky() const { return LauncherIcon::IsSticky(); }
+  void LocalActivate(ActionArg a) { ApplicationLauncherIcon::ActivateLauncherIcon(a); }
 
   using ApplicationLauncherIcon::IsFileManager;
   using ApplicationLauncherIcon::LogUnityEvent;
@@ -1109,4 +1112,21 @@
   usc_icon->Remove();
 }
 
+TEST_F(TestApplicationLauncherIcon, ClosesOverlayOnActivation)
+{
+  bool closes_overlay = false;
+  UBusManager manager;
+  manager.RegisterInterest(UBUS_OVERLAY_CLOSE_REQUEST, [&closes_overlay] (GVariant* v) {
+    ASSERT_THAT(v, IsNull());
+    closes_overlay = true;
+  });
+
+  // XXX: we should abstract the application activation into application::Manager
+  ON_CALL(*mock_icon, ActivateLauncherIcon(_)).WillByDefault(Invoke([this] (ActionArg a) { mock_icon->LocalActivate(a); }));
+  mock_icon->Activate(ActionArg());
+  Utils::WaitUntil(closes_overlay);
+
+  EXPECT_TRUE(closes_overlay);
 }
+
+} // anonymous namespace