Merge lp:~azzar1/unity/set-startup-id into lp:unity

[Andrea Azzarone]

Set startupId when launching application from the launcher.

== Problem ==
Removal of 21_correct_timestamp_.patch returns focus issues in Unity session.

== Fix ==
Set startupId when launching application from the launcher.
Please not that something similar needs to be done for the apps lens too.

== Test ==
AP test added.

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

92 + if self.app_is_running("Calculator"):
93 + self.skipTest("Calculator is already running.")

Why not closing it if, instead of skipping the test?

Anyway looks good.

[Andrea Azzarone (azzar1)]

> 92 + if self.app_is_running("Calculator"):
> 93 + self.skipTest("Calculator is already running.")
>
> Why not closing it if, instead of skipping the test?
>
> Anyway looks good.

Because a test should leave the system in the same state than it found it.

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

Ok, as you prefer...

review: Approve

[Thomi Richards (thomir-deactivatedaccount)]

Hi,

65 +

This probably doesn't need to be there...

77 + Time startup_notification_timestamp_;

This is inconsistent with the other member variables (trailing '_' rather than leading '_'). Please either follow the existing style, or update the other variables as well. Don't leave a file with both styles in place :)

92 + if self.app_is_running("Calculator"):
93 + self.skipTest("Calculator is already running.")

It's fine to kill the calculator if it's running - lots of tests do this already. In fact...

92 + if self.app_is_running("Calculator"):
93 + self.skipTest("Calculator is already running.")
94 +
95 + desktop_file = self.KNOWN_APPS["Calculator"]['desktop-file']
96 + calc_icon = self.launcher.model.get_icon(desktop_id=desktop_file)
97 +
98 + if calc_icon == None:
99 + calc = self.start_app("Calculator")
100 + desktop_file = calc.desktop_file
101 + calc_icon = self.launcher.model.get_icon(desktop_id=desktop_file)
102 +
103 + self.addCleanup(self.launcher_instance.unlock_from_launcher, calc_icon)
104 + self.launcher_instance.lock_to_launcher(calc_icon)
105 + self.close_all_app("Calculator")

This whole part should go in a utility function, and be called from the test. I think you want to refactor it slightly as well. Why not something like this:

def ensure_calculator_in_launcher_and_not_running(self):
 calc = self.start_app("Calculator")
 calc_icon = self.launcher.model.get_icon(desktop_id=calc.desktop_file)
 self.addCleanup(self.launcher_instance.unlock_from_launcher, calc_icon)
 self.launcher_instance.lock_to_launcher(calc_icon)
 self.close_all_app("Calculator")
 self.assertThat(lambda: self.app_is_running("Calculator"), Eventually(Equals(False)))
 return calc_icon

I think you should also move the X11 stuff into a separate function:

def get_startup_notification_timestamp(self, bamf_window):
 atom = display.Display().intern_atom('_NET_WM_USER_TIME')
 atom_type = display.Display().intern_atom('CARDINAL')
 return bamf_window.x_win.get_property(atom, atom_type, 0, 1024).value[0]

Then your test becomes a bit simpler:

def test_launcher_uses_startup_notification(self):
 """Tests that unity uses startup notification protocol."""
 calc_icon = self.ensure_calculator_in_launcher_and_not_running()
 self.addCleanup(self.close_all_app, "Calculator")
 self.launcher_instance.click_launcher_icon(calc_icon)

 # note - don't re-create bamf - just use it:
 calc_app = self.bamf.get_running_applications_by_desktop_file(desktop_file)[0]
 calc_window = calc_app.get_windows()[0]

 self.assertThat(lambda: self.get_startup_notification_timestamp(calc_window), Eventually(Equals(calc_icon.startup_notification_timestamp)))

Cheers,

review: Needs Fixing

[Andrea Azzarone (azzar1)]

> Hi,
>
> 65 +
>
> This probably doesn't need to be there...
>
> 77 + Time startup_notification_timestamp_;
>
> This is inconsistent with the other member variables (trailing '_' rather than
> leading '_'). Please either follow the existing style, or update the other
> variables as well. Don't leave a file with both styles in place :)
>
> 92 + if self.app_is_running("Calculator"):
> 93 + self.skipTest("Calculator is already running.")
>
> It's fine to kill the calculator if it's running - lots of tests do this
> already. In fact...
>
> 92 + if self.app_is_running("Calculator"):
> 93 + self.skipTest("Calculator is already running.")
> 94 +
> 95 + desktop_file = self.KNOWN_APPS["Calculator"]['desktop-file']
> 96 + calc_icon = self.launcher.model.get_icon(desktop_id=desktop_file)
> 97 +
> 98 + if calc_icon == None:
> 99 + calc = self.start_app("Calculator")
> 100 + desktop_file = calc.desktop_file
> 101 + calc_icon = self.launcher.model.get_icon(desktop_id=desktop_file)
> 102 +
> 103 + self.addCleanup(self.launcher_instance.unlock_from_launcher,
> calc_icon)
> 104 + self.launcher_instance.lock_to_launcher(calc_icon)
> 105 + self.close_all_app("Calculator")
>
> This whole part should go in a utility function, and be called from the test.
> I think you want to refactor it slightly as well. Why not something like this:
>
>
> def ensure_calculator_in_launcher_and_not_running(self):
> calc = self.start_app("Calculator")
> calc_icon = self.launcher.model.get_icon(desktop_id=calc.desktop_file)
> self.addCleanup(self.launcher_instance.unlock_from_launcher,
> calc_icon)
> self.launcher_instance.lock_to_launcher(calc_icon)
> self.close_all_app("Calculator")
> self.assertThat(lambda: self.app_is_running("Calculator"),
> Eventually(Equals(False)))
> return calc_icon
>
> I think you should also move the X11 stuff into a separate function:
>
> def get_startup_notification_timestamp(self, bamf_window):
> atom = display.Display().intern_atom('_NET_WM_USER_TIME')
> atom_type = display.Display().intern_atom('CARDINAL')
> return bamf_window.x_win.get_property(atom, atom_type, 0,
> 1024).value[0]
>
> Then your test becomes a bit simpler:
>
> def test_launcher_uses_startup_notification(self):
> """Tests that unity uses startup notification protocol."""
> calc_icon = self.ensure_calculator_in_launcher_and_not_running()
> self.addCleanup(self.close_all_app, "Calculator")
> self.launcher_instance.click_launcher_icon(calc_icon)
>
> # note - don't re-create bamf - just use it:
> calc_app =
> self.bamf.get_running_applications_by_desktop_file(desktop_file)[0]
> calc_window = calc_app.get_windows()[0]
>
> self.assertThat(lambda:
> self.get_startup_notification_timestamp(calc_window),
> Eventually(Equals(calc_icon.startup_notification_timestamp)))
>
>
> Cheers,

Done.

[Thomi Richards (thomir-deactivatedaccount)]

Looks super-awesome.

review: Approve (quality)

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/unity-mbs-autolanding/326/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mbs-autolanding/./build=pbuilder,distribution=raring,flavor=amd64/326/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mbs-autolanding/./build=pbuilder,distribution=raring,flavor=i386/326/console

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/unity-mbs-autolanding/327/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mbs-autolanding/./build=pbuilder,distribution=raring,flavor=amd64/327/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mbs-autolanding/./build=pbuilder,distribution=raring,flavor=i386/327/console

review: Needs Fixing (continuous-integration)

=== modified file 'launcher/ApplicationLauncherIcon.cpp'
--- launcher/ApplicationLauncherIcon.cpp	2012-12-18 17:57:48 +0000
+++ launcher/ApplicationLauncherIcon.cpp	2013-01-22 01:30:30 +0000
@@ -58,6 +58,7 @@
 ApplicationLauncherIcon::ApplicationLauncherIcon(ApplicationPtr const& app)
   : SimpleLauncherIcon(IconType::APPLICATION)
   , app_(app)
+  , _startup_notification_timestamp(0)
   , use_custom_bg_color_(false)
   , bg_color_(nux::color::White)
 {
@@ -479,16 +480,22 @@
     .add("desktop_file", DesktopFile())
     .add("desktop_id", GetDesktopID())
     .add("xids", g_variant_builder_end(&xids_builder))
-    .add("sticky", IsSticky());
+    .add("sticky", IsSticky())
+    .add("startup_notification_timestamp", _startup_notification_timestamp);
 }
 
-
 void ApplicationLauncherIcon::OpenInstanceWithUris(std::set<std::string> const& uris)
 {
   glib::Error error;
   glib::Object<GDesktopAppInfo> desktopInfo(g_desktop_app_info_new_from_filename(DesktopFile().c_str()));
   auto appInfo = glib::object_cast<GAppInfo>(desktopInfo);
 
+  GdkDisplay* display = gdk_display_get_default();
+  glib::Object<GdkAppLaunchContext> app_launch_context(gdk_display_get_app_launch_context(display));
+
+  _startup_notification_timestamp = nux::GetWindowThread()->GetGraphicsDisplay().GetCurrentEvent().x11_timestamp;
+  gdk_app_launch_context_set_timestamp(app_launch_context, _startup_notification_timestamp);
+
   if (g_app_info_supports_uris(appInfo))
   {
     GList* list = nullptr;
@@ -496,7 +503,7 @@
     for (auto  it : uris)
       list = g_list_prepend(list, g_strdup(it.c_str()));
 
-    g_app_info_launch_uris(appInfo, list, nullptr, &error);
+    g_app_info_launch_uris(appInfo, list, glib::object_cast<GAppLaunchContext>(app_launch_context), &error);
     g_list_free_full(list, g_free);
   }
   else if (g_app_info_supports_files(appInfo))
@@ -509,12 +516,12 @@
       list = g_list_prepend(list, file);
     }
 
-    g_app_info_launch(appInfo, list, nullptr, &error);
+    g_app_info_launch(appInfo, list, glib::object_cast<GAppLaunchContext>(app_launch_context), &error);
     g_list_free_full(list, g_object_unref);
   }
   else
   {
-    g_app_info_launch(appInfo, nullptr, nullptr, &error);
+    g_app_info_launch(appInfo, nullptr, glib::object_cast<GAppLaunchContext>(app_launch_context), &error);
   }
 
   if (error)

=== modified file 'launcher/ApplicationLauncherIcon.h'
--- launcher/ApplicationLauncherIcon.h	2012-12-05 10:24:53 +0000
+++ launcher/ApplicationLauncherIcon.h	2013-01-22 01:30:30 +0000
@@ -123,6 +123,7 @@
   std::string GetDesktopID();
 
   std::string _remote_uri;
+  Time _startup_notification_timestamp;
   std::set<std::string> _supported_types;
   std::map<std::string, glib::Object<DbusmenuClient>> _menu_clients;
   std::map<std::string, glib::Object<DbusmenuMenuitem>> _menu_items;

=== modified file 'tests/autopilot/unity/tests/launcher/test_icon_behavior.py'
--- tests/autopilot/unity/tests/launcher/test_icon_behavior.py	2013-01-14 10:58:35 +0000
+++ tests/autopilot/unity/tests/launcher/test_icon_behavior.py	2013-01-22 01:30:30 +0000
@@ -19,6 +19,7 @@
 from unity.emulators.launcher import IconDragType
 from unity.tests.launcher import LauncherTestCase, _make_scenarios
 
+from Xlib import display
 from Xlib import Xutil
 
 logger = logging.getLogger(__name__)
@@ -50,6 +51,20 @@
 
         return icon
 
+    def ensure_calculator_in_launcher_and_not_running(self):
+        calc = self.start_app("Calculator")
+        calc_icon = self.launcher.model.get_icon(desktop_id=calc.desktop_file)
+        self.addCleanup(self.launcher_instance.unlock_from_launcher, calc_icon)
+        self.launcher_instance.lock_to_launcher(calc_icon)
+        self.close_all_app("Calculator")
+        self.assertThat(lambda: self.app_is_running("Calculator"), Eventually(Equals(False)))
+        return calc_icon
+
+    def get_startup_notification_timestamp(self, bamf_window):
+        atom = display.Display().intern_atom('_NET_WM_USER_TIME')
+        atom_type = display.Display().intern_atom('CARDINAL')
+        return bamf_window.x_win.get_property(atom, atom_type, 0, 1024).value[0]
+
     def test_bfb_tooltip_disappear_when_dash_is_opened(self):
         """Tests that the bfb tooltip disappear when the dash is opened."""
         bfb = self.launcher.model.get_bfb_icon()
@@ -123,6 +138,17 @@
         self.assertThat(lambda: char_win2.is_hidden, Eventually(Equals(True)))
         self.assertVisibleWindowStack([char_win1, calc_win])
 
+    def test_launcher_uses_startup_notification(self):
+        """Tests that unity uses startup notification protocol."""
+        calc_icon = self.ensure_calculator_in_launcher_and_not_running()
+        self.addCleanup(self.close_all_app, "Calculator")
+        self.launcher_instance.click_launcher_icon(calc_icon)
+
+        calc_app = self.bamf.get_running_applications_by_desktop_file(calc_icon.desktop_id)[0]
+        calc_window = calc_app.get_windows()[0]
+
+        self.assertThat(lambda: self.get_startup_notification_timestamp(calc_window), Eventually(Equals(calc_icon.startup_notification_timestamp)))
+
     def test_clicking_icon_twice_initiates_spread(self):
         """This tests shows that when you click on a launcher icon twice,
         when an application window is focused, the spread is initiated.