Merge lp:~3v1n0/unity/hud-invalid-icon-fix into lp:unity

[Marco Trevisan (Treviño)]

HudController: use the top-most valid window in the stack to fetch the icon, if an invalid window is focused

HudController: use the top-most valid window in the stack to fetch the icon, if an invalid window is focused

When the dash or any other unity window is focused, the HUD isn't able to get the icon from the currently focused window (this manifests with bug #932371), so we need to fallback to the top-most valid window in the stack.

Getting the list of windows by stack order from BAMF, we can easily get the top-most application and then retrieve its icon. Thanks to this switching from HUD to Dash and the other way around, works as expected.

In the case that no valid window is focused, we fallback to the Ubuntu icon.

This includes AP tests.

[Thomi Richards (thomir-deactivatedaccount)]

Hi,

Great branch - a few small issues with the AP test though:

99 + self.keybinding("dash/reveal", 0.1)
100 + sleep(1)

Please use self.dash.ensure_visible() for this - it's safer, since it doesn't require you to sleep() after calling it.

105 + if self.hud.is_locked_launcher:
106 + hud_launcher_icon = self.get_hud_launcher_icon()
107 + self.assertThat(hud_launcher_icon.icon_name, Equals(calc.icon))
108 + else:
109 + hud_embedded_icon = self.hud.get_embedded_icon()
110 + self.assertThat(hud_embedded_icon.icon_name, Equals(calc.icon))

This seems to be something we're going to be doing a lot, so please just add a property to the Hud class that gets you the correct icon in all situations. THen these 6 lines become one:

self.assertThat(self.hud.icon.name, Equals(calc.icon))

... or something similar.

114 + self.start_app("Calculator")
115 + calctools = self.get_app_instances("Calculator")
116 + self.assertThat(len(calctools), GreaterThan(0))
117 + calc = calctools[0]
118 + self.assertTrue(calc.is_active)

This is ugly - and it's my fault. Please could you patch self.start_app(...) so it returns the BamfApplication instance? It shouldn't be hard to do, since we launch the application in the Bamf Emulator. THen you can replace all that code with this:

calc = self.start_app("Calculator")

Cheers,

review: Needs Fixing

[John Lea (johnlea)]

Thx, yes the bfb should be used.

review: Approve (design)

[Marco Trevisan (Treviño) (3v1n0)]

> Please use self.dash.ensure_visible() for this - it's safer, since it doesn't
> require you to sleep() after calling it.

Done.

> 105 + if self.hud.is_locked_launcher:
> 106 + hud_launcher_icon = self.get_hud_launcher_icon()
> 107 + self.assertThat(hud_launcher_icon.icon_name,
> Equals(calc.icon))
> 108 + else:
> 109 + hud_embedded_icon = self.hud.get_embedded_icon()
> 110 + self.assertThat(hud_embedded_icon.icon_name,
> Equals(calc.icon))
>
> This seems to be something we're going to be doing a lot, so please just add a
> property to the Hud class that gets you the correct icon in all situations.
> THen these 6 lines become one:
>
> self.assertThat(self.hud.icon.name, Equals(calc.icon))

Done.

> 114 + self.start_app("Calculator")
> 115 + calctools = self.get_app_instances("Calculator")
> 116 + self.assertThat(len(calctools), GreaterThan(0))
> 117 + calc = calctools[0]
> 118 + self.assertTrue(calc.is_active)
>
>
> This is ugly - and it's my fault. Please could you patch self.start_app(...)
> so it returns the BamfApplication instance? It shouldn't be hard to do, since
> we launch the application in the Bamf Emulator. THen you can replace all that
> code with this:
>
> calc = self.start_app("Calculator")

Done.

[Thomi Richards (thomir-deactivatedaccount)]

Awesome tests, thanks!

review: Approve

=== modified file 'plugins/unityshell/src/HudController.cpp'
--- plugins/unityshell/src/HudController.cpp	2012-04-02 00:16:21 +0000
+++ plugins/unityshell/src/HudController.cpp	2012-04-03 17:02:21 +0000
@@ -274,11 +274,51 @@
   view_->ShowEmbeddedIcon(!IsLockedToLauncher(monitor_index_));
   view_->AboutToShow();
 
-  // we first want to grab the currently active window, luckly we can just ask the jason interface(bamf)
-  BamfMatcher* matcher = bamf_matcher_get_default();
-  glib::Object<BamfView> bamf_app((BamfView*)(bamf_matcher_get_active_application(matcher)), glib::AddRef());
-  glib::String view_icon(bamf_view_get_icon(bamf_app));
-  focused_app_icon_ = view_icon.Str();
+  // We first want to grab the currently active window
+  glib::Object<BamfMatcher> matcher(bamf_matcher_get_default());
+  BamfWindow* active_win = bamf_matcher_get_active_window(matcher);
+
+  Window active_xid = bamf_window_get_xid(active_win);
+  std::vector<Window> const& unity_xids = nux::XInputWindow::NativeHandleList();
+
+  // If the active window is an unity window, we must get the top-most valid window
+  if (std::find(unity_xids.begin(), unity_xids.end(), active_xid) != unity_xids.end())
+  {
+    // Windows list stack for all the monitors
+    GList *windows = bamf_matcher_get_window_stack_for_monitor(matcher, -1);
+
+    for (GList *l = windows; l; l = l->next)
+    {
+      if (!BAMF_IS_WINDOW(l->data))
+        continue;
+
+      auto win = static_cast<BamfWindow*>(l->data);
+      auto view = static_cast<BamfView*>(l->data);
+      Window xid = bamf_window_get_xid(win);
+
+      if (bamf_view_user_visible(view) && bamf_window_get_window_type(win) != BAMF_WINDOW_DOCK &&
+          std::find(unity_xids.begin(), unity_xids.end(), xid) == unity_xids.end())
+      {
+        active_win = win;
+        active_xid = xid;
+      }
+    }
+
+    g_list_free(windows);
+  }
+
+  BamfApplication* active_app = bamf_matcher_get_application_for_window(matcher, active_win);
+
+  if (BAMF_IS_VIEW(active_app))
+  {
+    auto active_view = reinterpret_cast<BamfView*>(active_app);
+    glib::String view_icon(bamf_view_get_icon(active_view));
+    focused_app_icon_ = view_icon.Str();
+  }
+  else
+  {
+    focused_app_icon_ = focused_app_icon_ = PKGDATADIR "/launcher_bfb.png";
+  }
 
   LOG_DEBUG(logger) << "Taking application icon: " << focused_app_icon_;
   ubus.SendMessage(UBUS_HUD_ICON_CHANGED, g_variant_new_string(focused_app_icon_.c_str())); 

=== modified file 'plugins/unityshell/src/HudIcon.cpp'
--- plugins/unityshell/src/HudIcon.cpp	2012-04-01 00:00:45 +0000
+++ plugins/unityshell/src/HudIcon.cpp	2012-04-03 17:02:21 +0000
@@ -80,9 +80,8 @@
 
 std::string Icon::GetName() const
 {
-  return "EmbeddedIcon";
-}
-
-}
-}
-
+  return "HudEmbeddedIcon";
+}
+
+}
+}

=== modified file 'plugins/unityshell/src/HudLauncherIcon.cpp'
--- plugins/unityshell/src/HudLauncherIcon.cpp	2012-03-28 18:03:24 +0000
+++ plugins/unityshell/src/HudLauncherIcon.cpp	2012-04-03 17:02:21 +0000
@@ -58,10 +58,13 @@
     if (data_string)
       hud_icon_name = data_string;
     LOG_DEBUG(logger) << "Hud icon change: " << hud_icon_name;
-    if (!hud_icon_name.empty()
-        && hud_icon_name != icon_name())
+    if (hud_icon_name != icon_name)
     {
-      icon_name = hud_icon_name;
+      if (hud_icon_name.empty())
+        icon_name = PKGDATADIR"/launcher_bfb.png";
+      else
+        icon_name = hud_icon_name;
+
       EmitNeedsRedraw();
     }
   });
@@ -91,7 +94,7 @@
   g_variant_get(data, UBUS_OVERLAY_FORMAT_STRING,
                 &overlay_identity, &can_maximise, &overlay_monitor);
 
-  // If the hud is open, we show the HUD button iff we have a locked launcher
+  // If the hud is open, we show the HUD button if we have a locked launcher
   if (overlay_identity.Str() == "hud" &&
       launcher_hide_mode_ == LAUNCHER_HIDE_NEVER)
   {

=== modified file 'tests/autopilot/autopilot/emulators/unity/hud.py'
--- tests/autopilot/autopilot/emulators/unity/hud.py	2012-04-02 20:18:58 +0000
+++ tests/autopilot/autopilot/emulators/unity/hud.py	2012-04-03 17:02:21 +0000
@@ -9,7 +9,7 @@
 
 from autopilot.emulators.unity import UnityIntrospectionObject
 from autopilot.emulators.unity.dash import SearchBar
-from autopilot.emulators.unity.icons import EmbeddedIcon
+from autopilot.emulators.unity.icons import HudEmbeddedIcon, HudLauncherIcon
 from autopilot.keybindings import KeybindingsHelper
 
 
@@ -42,9 +42,22 @@
         if (not view):
           return None
 
-        icons = view.get_children_by_type(EmbeddedIcon)
+        icons = view.get_children_by_type(HudEmbeddedIcon)
         return icons[0] if icons else None
 
+    def get_launcher_icon(self):
+        """Returns the HUD launcher icon"""
+        icons = HudLauncherIcon.get_all_instances()
+        assert(len(icons) == 1)
+        return icons[0]
+
+    @property
+    def icon(self):
+        if self.is_locked_launcher:
+            return self.get_launcher_icon()
+        else:
+            return self.get_embedded_icon()
+
     @property
     def view(self):
         """Returns the HudView."""

=== modified file 'tests/autopilot/autopilot/emulators/unity/icons.py'
--- tests/autopilot/autopilot/emulators/unity/icons.py	2012-03-31 10:38:26 +0000
+++ tests/autopilot/autopilot/emulators/unity/icons.py	2012-04-03 17:02:21 +0000
@@ -69,10 +69,12 @@
 class DesktopLauncherIcon(SimpleLauncherIcon):
     """Represents an icon that may appear in the switcher."""
 
+
 class SoftwareCenterLauncherIcon(BamfLauncherIcon):
     """Represents a launcher icon of a Software Center app."""
 
-class EmbeddedIcon(SimpleLauncherIcon):
+
+class HudEmbeddedIcon(SimpleLauncherIcon):
     """Proxy object for the hud embedded icon child of the view."""
 
     @property

=== modified file 'tests/autopilot/autopilot/keybindings.py'
--- tests/autopilot/autopilot/keybindings.py	2012-04-02 01:46:10 +0000
+++ tests/autopilot/autopilot/keybindings.py	2012-04-03 17:02:21 +0000
@@ -90,6 +90,7 @@
     "workspace/move_up": ("wall", "up_key"),
     "workspace/move_down": ("wall", "down_key"),
     # Window management
+    "window/show_desktop" : ("core", "show_desktop_key"),
     "window/minimize": ("core", "minimize_window_key"),
     # expo plugin:
     "expo/start": ("expo", "expo_key"),

=== modified file 'tests/autopilot/autopilot/tests/__init__.py'
--- tests/autopilot/autopilot/tests/__init__.py	2012-03-31 10:38:26 +0000
+++ tests/autopilot/autopilot/tests/__init__.py	2012-04-03 17:02:21 +0000
@@ -242,6 +242,9 @@
 
         If files is specified, start the application with the specified files.
         If locale is specified, the locale will be set when the application is launched.
+
+        The method returns the BamfApplication instance.
+
         """
         if locale:
             os.putenv("LC_ALL", locale)
@@ -252,7 +255,10 @@
 
         app = self.KNOWN_APPS[app_name]
         self.bamf.launch_application(app['desktop-file'], files)
+        apps = self.bamf.get_running_applications_by_desktop_file(app['desktop-file'])
         self.addCleanup(call, ["killall", app['process-name']])
+        self.assertThat(len(apps), Equals(1))
+        return apps[0]
 
     def close_all_app(self, app_name):
         """Close all instances of the app_name."""

=== modified file 'tests/autopilot/autopilot/tests/test_hud.py'
--- tests/autopilot/autopilot/tests/test_hud.py	2012-04-02 20:18:58 +0000
+++ tests/autopilot/autopilot/tests/test_hud.py	2012-04-03 17:02:21 +0000
@@ -13,7 +13,7 @@
 from autopilot.emulators.X11 import ScreenGeometry
 from autopilot.emulators.unity.icons import HudLauncherIcon
 from autopilot.tests import AutopilotTestCase, multiply_scenarios
-from os import remove
+from os import remove, path
 
 
 def _make_monitor_scenarios():
@@ -33,17 +33,11 @@
 
     def setUp(self):
         super(HudTestsBase, self).setUp()
-        sleep(0.25)
 
     def tearDown(self):
         self.hud.ensure_hidden()
         super(HudTestsBase, self).tearDown()
 
-    def get_hud_launcher_icon(self):
-        icons = HudLauncherIcon.get_all_instances()
-        self.assertEqual(1, len(icons))
-        return icons[0]
-
     def get_num_active_launcher_icons(self):
         num_active = 0
         for icon in self.launcher.model.get_launcher_icons():
@@ -124,8 +118,8 @@
 
     def test_reveal_hud_with_no_apps(self):
         """Hud must show even with no visible applications."""
-        self.keyboard.press_and_release("Ctrl+Alt+d")
-        self.addCleanup(self.keyboard.press_and_release, "Ctrl+Alt+d")
+        self.keybinding("window/show_desktop")
+        self.addCleanup(self.keybinding, "window/show_desktop")
         sleep(1)
 
         self.hud.toggle_reveal()
@@ -140,10 +134,7 @@
         """Ensures that once the hud is dismissed, the same application
         that was focused before hud invocation is refocused
         """
-        self.start_app("Calculator")
-        calc = self.get_app_instances("Calculator")
-        self.assertThat(len(calc), Equals(1))
-        calc = calc[0]
+        calc = self.start_app("Calculator")
 
         # first ensure that the application has started and is focused
         self.assertEqual(calc.is_active, True)
@@ -316,7 +307,7 @@
     def test_hud_launcher_icon_hides_bfb(self):
         """Tests that the BFB icon is hidden when the HUD launcher icon is shown"""
 
-        hud_icon = self.get_hud_launcher_icon()
+        hud_icon = self.hud.get_launcher_icon()
         bfb_icon = self.launcher.model.get_bfb_icon()
 
         self.assertTrue(bfb_icon.is_visible_on_monitor(self.hud_monitor))
@@ -400,7 +391,7 @@
         self.hud.visible
         sleep(.5)
 
-        hud_launcher_icon = self.get_hud_launcher_icon()
+        hud_launcher_icon = self.hud.get_launcher_icon()
         hud_embedded_icon = self.hud.get_embedded_icon()
 
         if self.hud.is_locked_launcher:
@@ -414,20 +405,48 @@
             self.assertFalse(hud_launcher_icon.active)
             self.assertThat(hud_embedded_icon, NotEquals(None))
 
-    def test_hud_icon_show_the_focused_application_emblem(self):
+    def test_hud_icon_shows_the_focused_application_emblem(self):
         """Tests that the correct HUD icon is shown"""
-        self.start_app("Calculator")
-        calctools = self.get_app_instances("Calculator")
-        self.assertThat(len(calctools), GreaterThan(0))
-        calc = calctools[0]
-        self.assertTrue(calc.is_active)
-
-        self.reveal_hud()
-        sleep(.5)
-
-        if self.hud.is_locked_launcher:
-            hud_launcher_icon = self.get_hud_launcher_icon()
-            self.assertThat(hud_launcher_icon.icon_name, Equals(calc.icon))
-        else:
-            hud_embedded_icon = self.hud.get_embedded_icon()
-            self.assertThat(hud_embedded_icon.icon_name, Equals(calc.icon))
+        self.close_all_app("Calculator")
+        calc = self.start_app("Calculator")
+        self.assertTrue(calc.is_active)
+
+        self.reveal_hud()
+        sleep(.5)
+
+        self.assertThat(self.hud.icon.icon_name, Equals(calc.icon))
+
+    def test_hud_icon_shows_the_ubuntu_emblem_on_empty_desktop(self):
+        self.keybinding("window/show_desktop")
+        self.addCleanup(self.keybinding, "window/show_desktop")
+        sleep(1)
+
+        self.reveal_hud()
+        sleep(.5)
+
+        self.assertThat(path.basename(self.hud.icon.icon_name), Equals("launcher_bfb.png"))
+
+    def test_switch_dash_hud_does_not_break_the_focused_application_emblem(self):
+        """Tests that the correct HUD icon is shown when switching from Dash to HUD"""
+        self.close_all_app("Calculator")
+        calc = self.start_app("Calculator")
+        self.assertTrue(calc.is_active)
+
+        self.dash.ensure_visible()
+        self.reveal_hud()
+        sleep(.5)
+
+        self.assertThat(self.hud.icon.icon_name, Equals(calc.icon))
+
+    def test_switch_hud_dash_does_not_break_the_focused_application_emblem(self):
+        """Tests that the correct HUD icon is shown when switching from HUD to Dash and back"""
+        self.close_all_app("Calculator")
+        calc = self.start_app("Calculator")
+        self.assertTrue(calc.is_active)
+
+        self.reveal_hud()
+        self.dash.ensure_visible()
+        self.reveal_hud()
+        sleep(.5)
+
+        self.assertThat(self.hud.icon.icon_name, Equals(calc.icon))

=== modified file 'tests/autopilot/autopilot/tests/test_showdesktop.py'
--- tests/autopilot/autopilot/tests/test_showdesktop.py	2012-03-27 21:42:52 +0000
+++ tests/autopilot/autopilot/tests/test_showdesktop.py	2012-04-03 17:02:21 +0000
@@ -33,8 +33,8 @@
         self.launch_test_apps()
 
         # show desktop, verify all windows are hidden:
-        self.keyboard.press_and_release('Control+Alt+d')
-        self.addCleanup(self.keyboard.press_and_release, keys='Control+Alt+d')
+        self.keybinding("window/show_desktop")
+        self.addCleanup(self.keybinding, "window/show_desktop")
         sleep(3)
         open_wins = self.bamf.get_open_windows()
         self.assertGreaterEqual(len(open_wins), 2)
@@ -47,7 +47,8 @@
         self.launch_test_apps()
 
         # show desktop, verify all windows are hidden:
-        self.keyboard.press_and_release('Control+Alt+d')
+        self.keybinding("window/show_desktop")
+        self.addCleanup(self.keybinding, "window/show_desktop")
         sleep(3)
         open_wins = self.bamf.get_open_windows()
         self.assertGreaterEqual(len(open_wins), 2)
@@ -56,7 +57,8 @@
             self.assertTrue(win.is_hidden, "Window '%s' is not hidden after show desktop activated." % (win.title))
 
         # un-show desktop, verify all windows are shown:
-        self.keyboard.press_and_release('Control+Alt+d')
+        self.keybinding("window/show_desktop")
+        self.addCleanup(self.keybinding, "window/show_desktop")
         sleep(3)
         for win in self.bamf.get_open_windows():
             self.assertTrue(win.is_valid)
@@ -67,7 +69,9 @@
         self.launch_test_apps()
 
         # show desktop, verify all windows are hidden:
-        self.keyboard.press_and_release('Control+Alt+d')
+        self.keybinding("window/show_desktop")
+        self.addCleanup(self.keybinding, "window/show_desktop")
+
         sleep(3)
         open_wins = self.bamf.get_open_windows()
         self.assertGreaterEqual(len(open_wins), 2)
@@ -91,7 +95,8 @@
                     self.assertTrue(win.is_hidden, "Window '%s' should still be hidden." % (win.title))
 
         # hide desktop - now all windows should be visible:
-        self.keyboard.press_and_release('Control+Alt+d')
+        self.keybinding("window/show_desktop")
+        self.addCleanup(self.keybinding, "window/show_desktop")
         sleep(3)
         for win in self.bamf.get_open_windows():
             if win.is_valid:
@@ -102,21 +107,20 @@
         self.launch_test_apps()
 
         # show desktop, verify all windows are hidden:
-        switcher = Switcher()
-        switcher.initiate()
+        self.switcher.initiate()
         sleep(0.5)
         found = False
-        for i in range(switcher.get_model_size()):
-            current_icon = switcher.current_icon
+        for i in range(self.switcher.get_model_size()):
+            current_icon = self.switcher.current_icon
             self.assertIsNotNone(current_icon)
             if isinstance(current_icon, DesktopLauncherIcon):
                 found = True
                 break
-            switcher.previous_icon()
-            sleep(0.5)
+            self.switcher.previous_icon()
+            sleep(0.25)
         self.assertTrue(found, "Could not find 'Show Desktop' entry in switcher.")
-        switcher.stop()
-        self.addCleanup(self.keyboard.press_and_release, keys='Control+Alt+d')
+        self.addCleanup(self.keybinding, "window/show_desktop")
+        self.switcher.stop()
 
         sleep(3)
         open_wins = self.bamf.get_open_windows()

=== modified file 'tests/autopilot/autopilot/tests/test_switcher.py'
--- tests/autopilot/autopilot/tests/test_switcher.py	2012-04-02 20:18:58 +0000
+++ tests/autopilot/autopilot/tests/test_switcher.py	2012-04-03 17:02:21 +0000
@@ -290,29 +290,24 @@
 class SwitcherWorkspaceTests(AutopilotTestCase):
     """Test Switcher behavior with respect to multiple workspaces."""
 
-    def get_bamf_application(self, name):
-        apps = self.get_app_instances(name)
-        self.assertThat(len(apps), Equals(1))
-        return apps[0]
-
     def test_switcher_shows_current_workspace_only(self):
         """Switcher must show apps from the current workspace only."""
         self.close_all_app('Calculator')
         self.close_all_app('Character Map')
 
         self.workspace.switch_to(1)
-        self.start_app("Calculator")
+        calc = self.start_app("Calculator")
         sleep(1)
         self.workspace.switch_to(2)
-        self.start_app("Character Map")
+        char_map = self.start_app("Character Map")
         sleep(1)
 
         self.switcher.initiate()
         sleep(1)
         icon_names = [i.tooltip_text for i in self.switcher.get_switcher_icons()]
         self.switcher.terminate()
-        self.assertThat(icon_names, Contains(self.get_bamf_application("Character Map").name))
-        self.assertThat(icon_names, Not(Contains(self.get_bamf_application("Calculator").name)))
+        self.assertThat(icon_names, Contains(char_map.name))
+        self.assertThat(icon_names, Not(Contains(calc.name)))
 
     def test_switcher_all_mode_shows_all_apps(self):
         """Test switcher 'show_all' mode shows apps from all workspaces."""
@@ -320,18 +315,18 @@
         self.close_all_app('Character Map')
 
         self.workspace.switch_to(1)
-        self.start_app("Calculator")
+        calc = self.start_app("Calculator")
         sleep(1)
         self.workspace.switch_to(2)
-        self.start_app("Character Map")
+        char_map = self.start_app("Character Map")
         sleep(1)
 
         self.switcher.initiate_all_mode()
         sleep(1)
         icon_names = [i.tooltip_text for i in self.switcher.get_switcher_icons()]
         self.switcher.terminate()
-        self.assertThat(icon_names, Contains(self.get_bamf_application("Calculator").name))
-        self.assertThat(icon_names, Contains(self.get_bamf_application("Character Map").name))
+        self.assertThat(icon_names, Contains(calc.name))
+        self.assertThat(icon_names, Contains(char_map.name))
 
     def test_switcher_can_switch_to_minimised_window(self):
         """Switcher must be able to switch to a minimised window when there's
@@ -346,7 +341,7 @@
         self.start_app("Mahjongg")
 
         self.workspace.switch_to(3)
-        self.start_app("Mahjongg")
+        mahjongg = self.start_app("Mahjongg")
         sleep(1)
         self.keybinding("window/minimize")
         sleep(1)
@@ -356,14 +351,14 @@
 
         self.switcher.initiate()
         sleep(1)
-        while self.switcher.current_icon.tooltip_text != self.get_bamf_application("Mahjongg").name:
+        while self.switcher.current_icon.tooltip_text != mahjongg.name:
             self.switcher.next_icon()
             sleep(1)
         self.switcher.stop()
         sleep(1)
 
         #get mahjongg windows - there should be two:
-        wins = self.get_bamf_application("Mahjongg").get_windows()
+        wins = mahjongg.get_windows()
         self.assertThat(len(wins), Equals(2))
         # Ideally we should be able to find the instance that is on the
         # current workspace and ask that one if it is hidden.