Code review comment for lp://qastaging/~3v1n0/unity/panel-p-tests

Hi,

16 void PanelIndicatorEntryView::AddProperties(GVariantBuilder* builder)
17 {
18 + std::string type_name;
19 +
20 + switch (GetType())
21 + {
22 + case INDICATOR:
23 + type_name = "indicator";
24 + break;
25 + case MENU:
26 + type_name = "menu";
27 + break;
28 + default:
29 + type_name = "other";
30 + }

Can we make this conformant to the OCP? Create a 'virtual std::string GetTypeName() const', and have derived classes return their type string. That way, when you add a new indicator type, this code won't break.

...... on the python side:

220 +from autopilot.keybindings import KeybindingsHelper
221 +from autopilot.emulators.X11 import Mouse, ScreenGeometry
222 +from autopilot.emulators.unity import UnityIntrospectionObject

Imports must be alphabetical, so like this:

 from autopilot.emulators.X11 import Mouse, ScreenGeometry
 from autopilot.emulators.unity import UnityIntrospectionObject
 from autopilot.keybindings import KeybindingsHelper

230 + def get_panel_for_monitor(self, monitor_num):
231 + """Return an instance of panel for the specified monitor, or None."""
232 + panels = self.get_children_by_type(UnityPanel, monitor=monitor_num)
233 + return panels[0] if panels else None

Isn't it guaranteed that there will be exactly one panel for every monitor? If so, this code should be:

 def get_panel_for_monitor(self, monitor_num):
     """Return an instance of panel for the specified monitor."""
         panels = self.get_children_by_type(UnityPanel, monitor=monitor_num)
     assert( len(panels) == 1)
     return panels[0]

235 + def get_active_panel(self):
236 + """Return the active panel, or None."""
237 + panels = self.get_children_by_type(UnityPanel, active=True)
238 + assert(len(panels) == 1)
239 + return panels[0] if panels else None

You can change the return statement here to be 'return panels[0]'. If panels is empty, the return statement won't ever get hit, thanks to the assert.

241 + def get_panels(self):
242 + """Return the available panels, or None."""
243 + return self.get_children_by_type(UnityPanel)

This might be nicer as a property? like this:

 @property
 def get_panels(self):
     """Return the available panels, or None."""
     return self.get_children_by_type(UnityPanel)

244 +
245 +class UnityPanel(UnityIntrospectionObject, KeybindingsHelper):

PEP8 needs two spaces between items at the module-level, so please add ane xtra blank line here.

293 + if menu_entries and len(menu_entries) > 0:

An empty list evaluates to False, so you can just write 'if menu_entries:'

last_x = menu_entries[len(menu_entries)-1].x + menu_entries[len(menu_entries)-1].width / 2

this is better written as this:

last_x = menu_entries[-1].x + menu_entries[-1].width / 2

317 + def get_indicator_entry(self, id):
318 + """Returns the indicator entry for the given ID or None"""
319 + for en in self.get_indicator_entries():
320 + if en.id == id:
321 + return en
322 +
323 + return None

This can be simplified:

entries = filter(lambda e: e.id == id, self.get_indicator_entries())
return entries[0] if entries else None

443 + entries = []

You don't need this line ^^

553 +from autopilot.tests import AutopilotTestCase
554 +from autopilot.emulators.bamf import BamfWindow
555 +from autopilot.emulators.X11 import ScreenGeometry

Please re-order imports. 'tests' > 'emulators'

577 + #self.active_panel = self.panel.get_active_panel()

Please remove...

581 + #FIXME, remove it as soon as the better implementation goes in trunk
582 + def get_bamf_application(self, name):
583 + apps = self.get_app_instances(name)
584 + self.assertThat(len(apps), Equals(1))
585 + return apps[0]

Why not merge in the branch that has this code, and list it as a prerequisite branch on the MP?

587 + def move_window_to_monitor(self, window, monitor):

This method should go somewhere else, it doesn't belong in the test case class. I suggest putting it in the ScreenGeometry class instead.

Otherwsie, great work! THanks.