Merge lp:~3v1n0/unity/avoid-duplicate-icons into lp:unity

[Marco Trevisan (Treviño)]

We need to flag a bamf view that we control using an icon using the "unity-seen" qdata, or unity will duplicate it on bamfdaemon respawn. (LP: #928912)

We need to flag a bamf view that we control using an icon using the "unity-seen" qdata, or unity will duplicate it on bamfdaemon respawn.

AP tests included.

This MP was superseeded to use thomi's test fixes from lp:~thomir/unity/avoid-duplicate-icons

[Thomi Richards (thomir)]

Hi,

Please try and replace the call() lines with this:

call("kill `pidof %s`" % (app['process-name']), shell=True)

Don't use pkill -f - it's dangerous. Once making these changes, please ensure that all the tests still pass.

130 + self.start_app("Calculator")
131 + self.start_app("System Settings")
132 + os.spawnlp(os.P_NOWAIT, "xterm", "xterm", "-title", "Autopilot XTerm", "-e", "sh")
133 + self.addCleanup(call, ["killall", "xterm"])
134 +
135 + # FIXME bamf emulator should wait until a window is open
136 + sleep(1)
137 + [xterm_win] = [w for w in self.bamf.get_open_windows() if w.name == "Autopilot XTerm"]
138 + self.assertTrue(xterm_win.is_focused)

Please put all this in a method called "start_test_apps or something similar, then call it from your test.

154 + same_desktop = [i for i in bamf_icons if i.desktop_file == icon.desktop_file]
155 + self.assertThat(len(same_desktop), Equals(1))

Can you use 'get_icon_by_desktop_file' on the launcher model, to avoid this?

157 + same_appid = [i for i in bamf_icons if i.application_id == icon.application_id]
158 + self.assertThat(len(same_appid), Equals(1))

Similar thing here... we should probable add something to the launcher model like this:

def get_icon_by_filter(self, **kwargs):
        """Gets a launcher icon that matches the filter specified.

        Returns None if there is no such launcher icon.
        """
        icons = self.get_children_by_type(SimpleLauncherIcon, **kwargs)
        if len(icons):
            return icons[0]

        return None

review: Needs Fixing

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

+1

review: Approve

[Thomi Richards (thomir)]

This has a few problems:

1) I can't verify this without the libbamf fix (remember this isn't my branch, I'm just fixing the AP tests).
2) (possibly the same as above) - unity keeps the xclock icon around forever.

review: Needs Fixing

[Tim Penhey (thumper)]

Looks like the requested changes were done.

review: Approve

[Unity Merger (unity-merger)]

No commit message specified.

[Michal Hruby (mhr3)]

The unity side looks safe, tests pass with the not-yet-merged bamf branch.

review: Approve

=== modified file 'plugins/unityshell/src/BamfLauncherIcon.cpp'
--- plugins/unityshell/src/BamfLauncherIcon.cpp	2012-04-26 05:56:55 +0000
+++ plugins/unityshell/src/BamfLauncherIcon.cpp	2012-04-26 06:48:19 +0000
@@ -144,7 +144,7 @@
 {
   g_object_set_qdata(G_OBJECT(_bamf_app.RawPtr()),
                      g_quark_from_static_string("unity-seen"),
-                     GINT_TO_POINTER(0));
+                     GUINT_TO_POINTER(0));
 
   if (_fill_supported_types_id != 0)
     g_source_remove(_fill_supported_types_id);
@@ -500,6 +500,7 @@
   variant::BuilderWrapper(builder)
     .add("desktop_file", DesktopFile())
     .add("desktop_id", GetDesktopID())
+    .add("application_id", GPOINTER_TO_UINT(_bamf_app.RawPtr()))
     .add("xids", g_variant_builder_end(&xids_builder))
     .add("sticky", IsSticky());
 }

=== modified file 'plugins/unityshell/src/LauncherController.cpp'
--- plugins/unityshell/src/LauncherController.cpp	2012-04-24 21:57:01 +0000
+++ plugins/unityshell/src/LauncherController.cpp	2012-04-26 06:48:19 +0000
@@ -740,6 +740,7 @@
     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_++);
@@ -762,9 +763,8 @@
     return result;
   }
 
-  g_object_set_qdata(G_OBJECT(app), g_quark_from_static_string("unity-seen"), GINT_TO_POINTER(1));
-
   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,6 +790,7 @@
   }
 
   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_++);
 
@@ -831,8 +832,8 @@
 
     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"), GINT_TO_POINTER(1));
 
+    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-16 00:34:04 +0000
+++ tests/autopilot/autopilot/emulators/bamf.py	2012-04-26 06:48:19 +0000
@@ -254,6 +254,16 @@
         return self._x_win
 
     @property
+    def name(self):
+        """Get the window name.
+
+        Note: This may change according to the current locale. If you want a unique
+        string to match windows against, use the x_id instead.
+
+        """
+        return self._view_iface.Name()
+
+    @property
     def title(self):
         """Get the window title.
 

=== modified file 'tests/autopilot/autopilot/emulators/unity/launcher.py'
--- tests/autopilot/autopilot/emulators/unity/launcher.py	2012-04-23 16:41:53 +0000
+++ tests/autopilot/autopilot/emulators/unity/launcher.py	2012-04-26 06:48:19 +0000
@@ -327,17 +327,6 @@
                 return icon
         return None
 
-    def get_icon_by_desktop_file(self, desktop_file):
-        """Gets a launcher icon with the specified desktop file.
-
-        Returns None if there is no such launcher icon.
-        """
-        icons = self.get_children_by_type(SimpleLauncherIcon, desktop_file=desktop_file)
-        if len(icons):
-            return icons[0]
-
-        return None
-
     def get_icon_by_desktop_id(self, desktop_id):
         """Gets a launcher icon with the specified desktop id.
 
@@ -349,6 +338,19 @@
 
         return None
 
+    def get_icons_by_filter(self, **kwargs):
+        """Get a list of icons that satisfy the given filters.
+
+        For example:
+
+        >>> get_icons_by_filter(tooltip_text="My Application")
+        ... [...]
+
+        Returns an empty list if no icons matched the filter.
+
+        """
+        return self.get_children_by_type(SimpleLauncherIcon, **kwargs)
+
     def num_launcher_icons(self):
         """Get the number of icons in the launcher model."""
         return len(self.get_launcher_icons())

=== modified file 'tests/autopilot/autopilot/tests/__init__.py'
--- tests/autopilot/autopilot/tests/__init__.py	2012-04-24 22:26:55 +0000
+++ tests/autopilot/autopilot/tests/__init__.py	2012-04-26 06:48:19 +0000
@@ -224,6 +224,10 @@
             'desktop-file': 'remmina.desktop',
             'process-name': 'remmina',
             },
+        'System Settings' : {
+            'desktop-file': 'gnome-control-center.desktop',
+            'process-name': 'gnome-control-center',
+            },
         'Text Editor' : {
             'desktop-file': 'gedit.desktop',
             'process-name': 'gedit',
@@ -266,14 +270,14 @@
         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.addCleanup(call, "kill `pidof %s`" % (app['process-name']), shell=True)
         self.assertThat(len(apps), Equals(1))
         return apps[0]
 
     def close_all_app(self, app_name):
         """Close all instances of the app_name."""
         app = self.KNOWN_APPS[app_name]
-        call(["killall", app['process-name']])
+        self.addCleanup(call, "kill `pidof %s`" % (app['process-name']), shell=True)
         super(LoggedTestCase, self).tearDown()
 
     def get_app_instances(self, app_name):

=== modified file 'tests/autopilot/autopilot/tests/test_launcher.py'
--- tests/autopilot/autopilot/tests/test_launcher.py	2012-04-23 16:41:53 +0000
+++ tests/autopilot/autopilot/tests/test_launcher.py	2012-04-26 06:48:19 +0000
@@ -8,9 +8,12 @@
 # by the Free Software Foundation.
 
 import logging
+import os
+from subprocess import call
 from testtools.matchers import Equals, NotEquals, LessThan, GreaterThan
 from time import sleep
 
+from autopilot.emulators.bamf import Bamf
 from autopilot.emulators.unity.icons import BFBLauncherIcon
 from autopilot.emulators.X11 import ScreenGeometry
 from autopilot.matchers import Eventually
@@ -568,6 +571,52 @@
         for icon in self.launcher.model.get_launcher_icons():
             self.assertThat(icon.desaturated, Eventually(Equals(False)))
 
+class BamfDaemonTests(LauncherTestCase):
+    """Test interaction between the launcher and the BAMF Daemon."""
+
+    def start_test_apps(self):
+        """Starts some test applications."""
+        self.start_app("Calculator")
+        self.start_app("System Settings")
+
+    def get_test_apps(self):
+        """Return a tuple of test application instances.
+
+        We don't store these since when we kill the bamf daemon all references
+        to the old apps will die.
+
+        """
+        [calc] = self.get_app_instances("Calculator")
+        [sys_settings] = self.get_app_instances("System Settings")
+        return (calc, sys_settings)
+
+    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()
+
+        call(["pkill", "bamfdaemon"])
+        sleep(1)
+
+        # trigger the bamfdaemon to be reloaded again, and wait for it to appear:
+        self.bamf = Bamf()
+        self.wait_for_bamf_daemon()
+
+        for test_app in self.get_test_apps():
+            self.assertOnlyOneLauncherIcon(desktop_id=test_app.desktop_file)
+
 
 class LauncherCaptureTests(AutopilotTestCase):
     """Test the launchers ability to capture/not capture the mouse."""