Code review comment for lp:~azzar1/unity/set-startup-id

> 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.