Merge lp:~azzar1/unity/fix-778499 into lp:unity

[Andrea Azzarone]

Set a shortcut for the new apps on the launcher too.

== Problem ==
New apps on the launcher have no shortcut key until something is being closed.

== Fix ==
Call LauncherController::SortAndUpdate when a new bamf icon has been added to the launcher.

== Test ==
App test added.
./tools/autopilot run autopilot.tests.test_launcher.LauncherRevealTests.test_new_icon_has_the_shortcut

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

Unity code looks good to me.

The test needs some tuning:

111 + desktop_file = self.KNOWN_APPS['Calculator']['desktop-file']
112 + if self.launcher.model.get_icon_by_desktop_id(desktop_file) != None:
113 + self.skip("Calculator icon is already on the launcher.")
114 +
115 + self.start_app('Calculator')
116 + icon = self.launcher.model.get_icon_by_desktop_id(desktop_file)
117 + self.assertThat(icon.shortcut, GreaterThan(0))

Instead of skipping the test when the calculator is already opened, just close it and you can just do:

self.close_all_app('Calculator')
calc = self.start_app('Calculator')
icon = self.launcher.model.get_icon_by_desktop_id(calc.desktop_file)
self.assertThat(icon.shortcut, Eventually(GreaterThan(0)))

(using eventually to avoid false-negatives)

review: Needs Fixing

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

> self.close_all_app('Calculator')

Put this even before the "num_bamf_launcher_icons" check, so that it won't fail just for that icon ;)

[Andrea Azzarone (azzar1)]

> Unity code looks good to me.
>
> The test needs some tuning:
>
> 111 + desktop_file = self.KNOWN_APPS['Calculator']['desktop-file']
> 112 + if self.launcher.model.get_icon_by_desktop_id(desktop_file)
> != None:
> 113 + self.skip("Calculator icon is already on the launcher.")
> 114 +
> 115 + self.start_app('Calculator')
> 116 + icon =
> self.launcher.model.get_icon_by_desktop_id(desktop_file)
> 117 + self.assertThat(icon.shortcut, GreaterThan(0))
>
> Instead of skipping the test when the calculator is already opened, just close
> it and you can just do:
>
> self.close_all_app('Calculator')
> calc = self.start_app('Calculator')
> icon = self.launcher.model.get_icon_by_desktop_id(calc.desktop_file)
> self.assertThat(icon.shortcut, Eventually(GreaterThan(0)))
>
> (using eventually to avoid false-negatives)

If the Calculator is pinned, closing it will not remove the icon from the launcher.

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

You're right... But you can still check for that...

[Andrea Azzarone (azzar1)]

Yeah but keep in mind that tests should be as clean as possible, and adding another if statement will add clutter to the test. But I will do as you want...

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

Ok.. fair enough.

review: Approve

=== modified file 'plugins/unityshell/src/AbstractLauncherIcon.h'
--- plugins/unityshell/src/AbstractLauncherIcon.h	2012-04-11 15:49:58 +0000
+++ plugins/unityshell/src/AbstractLauncherIcon.h	2012-04-24 18:13:37 +0000
@@ -215,6 +215,7 @@
 
   sigc::signal<void, AbstractLauncherIcon::Ptr> needs_redraw;
   sigc::signal<void, AbstractLauncherIcon::Ptr> remove;
+  sigc::signal<void> visibility_changed;
 
   sigc::connection needs_redraw_connection;
   sigc::connection on_icon_added_connection;

=== modified file 'plugins/unityshell/src/LauncherController.cpp'
--- plugins/unityshell/src/LauncherController.cpp	2012-04-21 21:32:26 +0000
+++ plugins/unityshell/src/LauncherController.cpp	2012-04-24 18:13:37 +0000
@@ -497,7 +497,7 @@
       std::stringstream shortcut_string;
       shortcut_string << (shortcut % 10);
       icon->SetShortcut(shortcut_string.str()[0]);
-      shortcut++;
+      ++shortcut;
     }
     // reset shortcut
     else
@@ -607,6 +607,8 @@
     else
       model_->ReorderBefore(result, other, false);
   }
+
+  SortAndUpdate();
 }
 
 void Controller::Impl::OnFavoriteStoreFavoriteRemoved(std::string const& entry)
@@ -738,10 +740,11 @@
     return;
   }
 
-  AbstractLauncherIcon::Ptr icon (new BamfLauncherIcon(app));
+  AbstractLauncherIcon::Ptr icon(new BamfLauncherIcon(app));
+  icon->visibility_changed.connect(sigc::mem_fun(self, &Impl::SortAndUpdate));
   icon->SetSortPriority(self->sort_priority_++);
-
   self->RegisterIcon(icon);
+  self->SortAndUpdate();
 }
 
 AbstractLauncherIcon::Ptr Controller::Impl::CreateFavorite(const char* file_path)

=== modified file 'plugins/unityshell/src/LauncherIcon.cpp'
--- plugins/unityshell/src/LauncherIcon.cpp	2012-04-11 15:49:58 +0000
+++ plugins/unityshell/src/LauncherIcon.cpp	2012-04-24 18:13:37 +0000
@@ -202,6 +202,7 @@
   .add("icon_type", _icon_type)
   .add("tooltip_text", tooltip_text())
   .add("sort_priority", _sort_priority)
+  .add("shortcut", _shortcut)
   .add("monitors_visibility", g_variant_builder_end(&monitors_builder))
   .add("active", GetQuirk(QUIRK_ACTIVE))
   .add("visible", GetQuirk(QUIRK_VISIBLE))
@@ -853,6 +854,11 @@
     UBusServer* ubus = ubus_server_get_default();
     ubus_server_send_message(ubus, UBUS_LAUNCHER_ICON_URGENT_CHANGED, g_variant_new_boolean(value));
   }
+
+  if (quirk == QUIRK_VISIBLE)
+  {
+     visibility_changed.emit();
+  }
 }
 
 gboolean

=== modified file 'tests/autopilot/autopilot/emulators/unity/launcher.py'
--- tests/autopilot/autopilot/emulators/unity/launcher.py	2012-04-17 23:17:32 +0000
+++ tests/autopilot/autopilot/emulators/unity/launcher.py	2012-04-24 18:13:37 +0000
@@ -301,6 +301,13 @@
         else:
             return self.get_children_by_type(SimpleLauncherIcon)
 
+    def get_bamf_launcher_icons(self, visible_only=True):
+        """Get a list of bamf launcher icons in this launcher."""
+        if visible_only:
+            return self.get_children_by_type(BamfLauncherIcon, visible=True)
+        else:
+            return self.get_children_by_type(BamfLauncherIcon)
+
     def get_launcher_icons_for_monitor(self, monitor, visible_only=True):
         """Get a list of launcher icons for provided monitor."""
         icons = []
@@ -345,3 +352,7 @@
     def num_launcher_icons(self):
         """Get the number of icons in the launcher model."""
         return len(self.get_launcher_icons())
+
+    def num_bamf_launcher_icons(self, visible_only=True):
+        """Get the number of bamf icons in the launcher model."""
+        return len(self.get_bamf_launcher_icons(visible_only))

=== modified file 'tests/autopilot/autopilot/tests/test_launcher.py'
--- tests/autopilot/autopilot/tests/test_launcher.py	2012-04-18 07:33:15 +0000
+++ tests/autopilot/autopilot/tests/test_launcher.py	2012-04-24 18:13:37 +0000
@@ -499,6 +499,19 @@
         sleep(5)
         self.assertThat(self.launcher_instance.is_showing, Equals(False))
 
+    def test_new_icon_has_the_shortcut(self):
+         """New icons should have an associated shortcut"""
+         if self.launcher.model.num_bamf_launcher_icons() >= 10:
+             self.skip("There are already more than 9 icons in the launcher")
+
+         desktop_file = self.KNOWN_APPS['Calculator']['desktop-file']
+         if self.launcher.model.get_icon_by_desktop_id(desktop_file) != None:
+             self.skip("Calculator icon is already on the launcher.")
+
+         self.start_app('Calculator')
+         icon = self.launcher.model.get_icon_by_desktop_id(desktop_file)
+         self.assertThat(icon.shortcut, GreaterThan(0))
+
 
 class LauncherVisualTests(LauncherTestCase):
     """Tests for visual aspects of the launcher (icon saturation etc.)."""