Merge lp:~3v1n0/unity/missing-launcher-icon-fix-5.0 into lp:unity/5.0

[Marco Trevisan (Treviño)]

Merged branches lp:~3v1n0/unity/launcher-double-icons-tests and lp:~3v1n0/unity/quick-app-reopen-icon-fix

Merged branches lp:~3v1n0/unity/launcher-double-icons-tests and lp:~3v1n0/unity/quick-app-reopen-icon-fix

[Unity Merger (unity-merger)]

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-unity/737/console reported an error when processing this lp:~3v1n0/unity/missing-launcher-icon-fix-5.0 branch.
Not merging it.

[Gord Allott (gordallott)]

Surely this means that it now takes every application a second before its removed from the launcher? i don't think that is something we want

review: Needs Information

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

No, the icon is immediately removed (because this is controlled by the "running-changed" signal), but the icon itself is not removed by the model too early. We wait some time.
See the the description of the MR on lp:~3v1n0/unity/quick-app-reopen-icon-fix

=== modified file 'plugins/unityshell/src/BamfLauncherIcon.cpp'
--- plugins/unityshell/src/BamfLauncherIcon.cpp	2012-04-26 09:19:21 +0000
+++ plugins/unityshell/src/BamfLauncherIcon.cpp	2012-05-21 15:34:22 +0000
@@ -54,6 +54,8 @@
   , _fill_supported_types_id(0)
   , _window_moved_id(0)
 {
+  g_object_set_qdata(G_OBJECT(app), g_quark_from_static_string("unity-seen"),
+                     GUINT_TO_POINTER(1));
   auto bamf_view = glib::object_cast<BamfView>(_bamf_app);
 
   glib::String icon(bamf_view_get_icon(bamf_view));
@@ -99,10 +101,17 @@
   sig = new glib::Signal<void, BamfView*, gboolean>(bamf_view, "running-changed",
                           [&] (BamfView*, gboolean running) {
                             SetQuirk(QUIRK_RUNNING, running);
+
                             if (running)
                             {
                               EnsureWindowState();
                               UpdateIconGeometries(GetCenters());
+
+                              if (_remove_timeout_id)
+                              {
+                                g_source_remove(_remove_timeout_id);
+                                _remove_timeout_id = 0;
+                              }
                             }
                           });
   _gsignals.Add(sig);
@@ -117,7 +126,23 @@
   sig = new glib::Signal<void, BamfView*>(bamf_view, "closed",
                           [&] (BamfView*) {
                             if (!IsSticky())
-                              Remove();
+                            {
+                              /* Use a timeout to remove the icon, this avoids
+                               * that we remove an application that is going
+                               * to be reopened soon. So applications that
+                               * have a splash screen won't be removed from
+                               * the launcher while the splash is closed and
+                               * a new window is opened. */
+                              if (_remove_timeout_id)
+                                g_source_remove(_remove_timeout_id);
+
+                              _remove_timeout_id = g_timeout_add_seconds(1, [] (gpointer data) -> gboolean {
+                                auto self = static_cast<BamfLauncherIcon*>(data);
+                                self->Remove();
+                                self->_remove_timeout_id = 0;
+                                return false;
+                              }, this);
+                            }
                           });
   _gsignals.Add(sig);
 
@@ -143,8 +168,10 @@
 BamfLauncherIcon::~BamfLauncherIcon()
 {
   g_object_set_qdata(G_OBJECT(_bamf_app.RawPtr()),
-                     g_quark_from_static_string("unity-seen"),
-                     GUINT_TO_POINTER(0));
+                     g_quark_from_static_string("unity-seen"), nullptr);
+
+  if (_remove_timeout_id != 0)
+    g_source_remove(_remove_timeout_id);
 
   if (_fill_supported_types_id != 0)
     g_source_remove(_fill_supported_types_id);
@@ -159,6 +186,18 @@
     g_source_remove(_dnd_hover_timer);
 }
 
+void BamfLauncherIcon::Remove()
+{
+  /* Removing the unity-seen flag to the wrapped bamf application, on remove
+   * request we make sure that if the bamf application is re-opened while
+   * the removal process is still ongoing, the application will be shown
+   * on the launcher. */
+  g_object_set_qdata(G_OBJECT(_bamf_app.RawPtr()),
+                     g_quark_from_static_string("unity-seen"), nullptr);
+
+  SimpleLauncherIcon::Remove();
+}
+
 bool BamfLauncherIcon::IsSticky() const
 {
   return bamf_view_is_sticky(BAMF_VIEW(_bamf_app.RawPtr()));

=== modified file 'plugins/unityshell/src/BamfLauncherIcon.h'
--- plugins/unityshell/src/BamfLauncherIcon.h	2012-04-26 00:35:37 +0000
+++ plugins/unityshell/src/BamfLauncherIcon.h	2012-05-21 15:34:22 +0000
@@ -66,6 +66,7 @@
   std::string NameForWindow(Window window);
 
 protected:
+  void Remove();
   void UpdateIconGeometries(std::vector<nux::Point3> center);
   void OnCenterStabilized(std::vector<nux::Point3> center);
   void AddProperties(GVariantBuilder* builder);
@@ -124,6 +125,7 @@
   guint _dnd_hover_timer;
 
   bool _supported_types_filled;
+  guint _remove_timeout_id;
   guint _fill_supported_types_id;
   guint _window_moved_id;
   guint _quicklist_activated_id;

=== modified file 'plugins/unityshell/src/LauncherController.cpp'
--- plugins/unityshell/src/LauncherController.cpp	2012-04-26 00:53:36 +0000
+++ plugins/unityshell/src/LauncherController.cpp	2012-05-21 15:34:22 +0000
@@ -740,7 +740,6 @@
     return;
   }
 
-  g_object_set_qdata(G_OBJECT(app), g_quark_from_static_string("unity-seen"), GUINT_TO_POINTER(1));
   AbstractLauncherIcon::Ptr icon(new BamfLauncherIcon(app));
   icon->visibility_changed.connect(sigc::mem_fun(self, &Impl::SortAndUpdate));
   icon->SetSortPriority(self->sort_priority_++);
@@ -764,7 +763,6 @@
   }
 
   bamf_view_set_sticky(BAMF_VIEW(app), true);
-  g_object_set_qdata(G_OBJECT(app), g_quark_from_static_string("unity-seen"), GUINT_TO_POINTER(1));
   AbstractLauncherIcon::Ptr icon (new BamfLauncherIcon(app));
   icon->SetSortPriority(sort_priority_++);
   result = icon;
@@ -790,7 +788,6 @@
   }
 
   bamf_view_set_sticky(BAMF_VIEW(app), true);
-  g_object_set_qdata(G_OBJECT(app), g_quark_from_static_string("unity-seen"), GUINT_TO_POINTER(1));
   result = new SoftwareCenterLauncherIcon(app, aptdaemon_trans_id, icon_path);
   result->SetSortPriority(sort_priority_++);
 
@@ -833,7 +830,6 @@
     if (g_object_get_qdata(G_OBJECT(app), g_quark_from_static_string("unity-seen")))
       continue;
 
-    g_object_set_qdata(G_OBJECT(app), g_quark_from_static_string("unity-seen"), GUINT_TO_POINTER(1));
     AbstractLauncherIcon::Ptr icon(new BamfLauncherIcon(app));
     icon->SetSortPriority(sort_priority_++);
     RegisterIcon(icon);

=== modified file 'tests/autopilot/autopilot/emulators/bamf.py'
--- tests/autopilot/autopilot/emulators/bamf.py	2012-04-26 02:53:18 +0000
+++ tests/autopilot/autopilot/emulators/bamf.py	2012-05-21 15:34:22 +0000
@@ -75,6 +75,14 @@
         """
         return [a for a in self.get_running_applications() if a.desktop_file == desktop_file]
 
+    def get_application_by_xid(self, xid):
+        """Return the application that has a child with the requested xid or None."""
+
+        app_path = self.matcher_interface.ApplicationForXid(xid)
+        if len(app_path):
+            return BamfApplication(app_path)
+        return None
+
     def get_open_windows(self, user_visible_only=True):
         """Get a list of currently open windows.
 
@@ -85,17 +93,16 @@
 
         """
 
-        # Get the stacking order from the root window.
-        root_win = _X_DISPLAY.screen().root
-        prop = root_win.get_full_property(
-            _X_DISPLAY.get_atom('_NET_CLIENT_LIST_STACKING'), X.AnyPropertyType)
-        stack = prop.value.tolist()
-
-        windows = [BamfWindow(w) for w in self.matcher_interface.WindowPaths()]
+        windows = [BamfWindow(w) for w in self.matcher_interface.WindowStackForMonitor(-1)]
         if user_visible_only:
             windows = filter(_filter_user_visible, windows)
         # Now sort on stacking order.
-        return sorted(windows, key=lambda w: stack.index(w.x_id), reverse=True)
+        return reversed(windows)
+
+    def get_window_by_xid(self, xid):
+        """Get the BamfWindow that matches the provided 'xid'."""
+        windows = [BamfWindow(w) for w in self.matcher_interface.WindowPaths() if BamfWindow(w).x_id == xid]
+        return windows[0] if windows else None
 
     def wait_until_application_is_running(self, desktop_file, timeout):
         """Wait until a given application is running.

=== modified file 'tests/autopilot/autopilot/emulators/unity/icons.py'
--- tests/autopilot/autopilot/emulators/unity/icons.py	2012-04-24 13:14:52 +0000
+++ tests/autopilot/autopilot/emulators/unity/icons.py	2012-05-21 15:34:22 +0000
@@ -20,7 +20,7 @@
 
     @property
     def center_position(self):
-        """Get the center point of an icon, returns a tuple with (x, y, z)"""
+        """Get the center point of an icon, returns a tuple with (x, y, z)."""
         return (self.center_x, self.center_y, self.center_z)
 
     def get_quicklist(self):
@@ -34,12 +34,17 @@
         return matches[0] if matches else None
 
     def is_on_monitor(self, monitor):
-        """Returns True if the icon is available in the defined monitor"""
+        """Returns True if the icon is available in the defined monitor."""
         if monitor >= 0 and monitor < len(self.monitors_visibility):
             return self.monitors_visibility[monitor]
 
         return False
 
+    def controls_window(self, xid):
+        """Returns true if the icon controls the specified xid."""
+
+        return self.xids.contains(xid)
+
 
 class BFBLauncherIcon(SimpleLauncherIcon):
     """Represents the BFB button in the launcher."""

=== modified file 'tests/autopilot/autopilot/emulators/unity/launcher.py'
--- tests/autopilot/autopilot/emulators/unity/launcher.py	2012-04-26 05:11:45 +0000
+++ tests/autopilot/autopilot/emulators/unity/launcher.py	2012-05-21 15:34:22 +0000
@@ -338,6 +338,14 @@
 
         return None
 
+    def get_icon_by_window_xid(self, xid):
+        """Gets a launcher icon that controls the specified window xid."""
+        icons = [i for i in self.get_children_by_type(SimpleLauncherIcon) if i.xids.contains(xid)]
+        if (len(icons)):
+            return icons[0]
+
+        return None
+
     def get_icons_by_filter(self, **kwargs):
         """Get a list of icons that satisfy the given filters.
 

=== modified file 'tests/autopilot/autopilot/tests/__init__.py'
--- tests/autopilot/autopilot/tests/__init__.py	2012-04-26 04:02:39 +0000
+++ tests/autopilot/autopilot/tests/__init__.py	2012-05-21 15:34:22 +0000
@@ -277,8 +277,9 @@
     def close_all_app(self, app_name):
         """Close all instances of the app_name."""
         app = self.KNOWN_APPS[app_name]
-        self.addCleanup(call, "kill `pidof %s`" % (app['process-name']), shell=True)
-        super(LoggedTestCase, self).tearDown()
+        pids = check_output(["pidof", app['process-name']]).split()
+        if len(pids):
+            call(["kill"] + pids)
 
     def get_app_instances(self, app_name):
         """Get BamfApplication instances for app_name."""

=== modified file 'tests/autopilot/autopilot/tests/test_launcher.py'
--- tests/autopilot/autopilot/tests/test_launcher.py	2012-04-26 06:44:00 +0000
+++ tests/autopilot/autopilot/tests/test_launcher.py	2012-05-21 15:34:22 +0000
@@ -417,7 +417,6 @@
         sleep(1)
         [mah_win2] = [w for w in mahj.get_windows() if w.x_id != mah_win1.x_id]
         self.assertTrue(mah_win2.is_focused)
-
         self.assertVisibleWindowStack([mah_win2, calc_win, mah_win1])
 
         mahj_icon = self.launcher.model.get_icon_by_desktop_id(mahj.desktop_file)
@@ -446,6 +445,20 @@
         self.assertTrue(mah_win2.is_hidden)
         self.assertVisibleWindowStack([mah_win1, calc_win])
 
+    def test_icon_shows_on_quick_application_reopen(self):
+        """Icons should stay on launcher when an application is quickly closed/reopened."""
+        calc = self.start_app("Calculator")
+        desktop_file = calc.desktop_file
+        calc_icon = self.launcher.model.get_icon_by_desktop_id(desktop_file)
+        self.assertThat(calc_icon.visible, Eventually(Equals(True)))
+
+        self.close_all_app("Calculator")
+        calc = self.start_app("Calculator")
+        sleep(2)
+
+        calc_icon = self.launcher.model.get_icon_by_desktop_id(desktop_file)
+        self.assertThat(calc_icon, NotEquals(None))
+        self.assertThat(calc_icon.visible, Eventually(Equals(True)))
 
 class LauncherRevealTests(LauncherTestCase):
     """Test the launcher reveal behavior when in autohide mode."""
@@ -579,6 +592,15 @@
         self.start_app("Calculator")
         self.start_app("System Settings")
 
+    def start_desktopless_test_apps(self):
+        """Start test applications with no .desktop file associated."""
+        test_apps = ["xclock"]
+
+        for app in test_apps:
+            os.spawnlp(os.P_NOWAIT, app, app)
+            self.addCleanup(call, ["killall", app])
+            self.wait_for_process_started(app)
+
     def get_test_apps(self):
         """Return a tuple of test application instances.
 
@@ -588,35 +610,75 @@
         """
         [calc] = self.get_app_instances("Calculator")
         [sys_settings] = self.get_app_instances("System Settings")
-        return (calc, sys_settings)
+        return [calc, sys_settings]
+
+    def get_desktopless_test_apps(self):
+        """Return a tuple of test application with no .desktop files instances."""
+        [xclock_win] = [w for w in self.bamf.get_open_windows() if w.name == "xclock"]
+        return [xclock_win.application]
 
     def assertOnlyOneLauncherIcon(self, **kwargs):
         """Asserts that there is only one launcher icon with the given filter."""
         icons = self.launcher.model.get_icons_by_filter(**kwargs)
         self.assertThat(len(icons), Equals(1))
 
-    def wait_for_bamf_daemon(self):
-        """Wait until the bamf daemon has been started."""
-        for i in range(10):
-            #pgrep returns 0 if it matched something:
-            if call(["pgrep", "bamfdaemon"]) == 0:
-                return
-            sleep(1)
-
-    def test_killing_bamfdaemon_does_not_duplicate_desktop_ids(self):
-        """Killing bamfdaemon should not duplicate any desktop ids in the model."""
-        self.start_test_apps()
-
+    def wait_for_process_started(self, app):
+        """Wait until the application app has been started."""
+        for i in range(10):
+            sleep(1)
+            #pgrep returns 0 if it matched something:
+            if call(["pgrep", app]) == 0:
+                return
+
+    def wait_for_process_killed(self, app):
+        """Wait until the application app has been killed."""
+        for i in range(10):
+            #pgrep returns 0 if it matched something:
+            if call(["pgrep", app]) != 0:
+                return
+            sleep(1)
+
+    def kill_and_restart_bamfdaemon(self):
+        """Kills and resumes bamfdaemon."""
         call(["pkill", "bamfdaemon"])
-        sleep(1)
+        self.wait_for_process_killed("bamfdaemon")
 
         # trigger the bamfdaemon to be reloaded again, and wait for it to appear:
         self.bamf = Bamf()
-        self.wait_for_bamf_daemon()
+        self.wait_for_process_started("bamfdaemon")
+
+    def test_killing_bamfdaemon_does_not_duplicate_desktop_ids(self):
+        """Killing bamfdaemon should not duplicate any desktop ids in the model."""
+        self.start_test_apps()
+        self.kill_and_restart_bamfdaemon()
 
         for test_app in self.get_test_apps():
+            logger.info("Checking for duplicated launcher icon for application %s", test_app.name)
             self.assertOnlyOneLauncherIcon(desktop_id=test_app.desktop_file)
 
+    def test_killing_bamfdaemon_does_not_duplicate_application_xids(self):
+        """Killing bamfdaemon should not duplicate any xid in the model."""
+        self.start_test_apps()
+        self.start_desktopless_test_apps()
+        self.kill_and_restart_bamfdaemon()
+
+        test_apps = self.get_test_apps() + self.get_desktopless_test_apps()
+
+        for test_app in test_apps:
+            logger.info("Checking for duplicated launcher icon for application %s", test_app.name)
+            test_windows = [w.x_id for w in test_app.get_windows()]
+            self.assertOnlyOneLauncherIcon(xids=test_windows)
+
+    def test_killing_bamfdaemon_does_not_duplicate_any_icon_application_id(self):
+        """Killing bamfdaemon should not duplicate any application ids in the model."""
+        self.start_test_apps()
+        self.start_desktopless_test_apps()
+        self.kill_and_restart_bamfdaemon()
+
+        for icon in self.launcher.model.get_bamf_launcher_icons():
+            logger.info("Checking for duplicated launcher icon %s", icon.tooltip_text)
+            self.assertOnlyOneLauncherIcon(application_id=icon.application_id)
+
 
 class LauncherCaptureTests(AutopilotTestCase):
     """Test the launchers ability to capture/not capture the mouse."""