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,