Merge lp://qastaging/~3v1n0/unity/panel-p-tests into lp://qastaging/unity

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

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

Currently both the MENU and INDICATOR IndicatorEntry types are managed by the PanelIndicatorEntry class, that's why I've just added that, since I only needed to get an human readable type on AP side.
So I can't just add a such virtual method, other than one that does basically the same of what I'm doing here: manually checks for the type and sets a string.

> ...... on the python side:
> 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?

I've already set another branch as prerequisite... I couldn't do that. :(

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

Mh, ok... I just left that there for debugging now, I'll move to the proper class as you suggested soon.

Thanks a lot for your tips, Thomi ;)

> plugins/unityshell/src/DashView.cpp

  std::string form_factor;

Lets give this a sensible default, like:

  std::string form_factor("unknown");

Do we really need drag_window_to_monitor?

> tests/autopilot/autopilot/emulators/unity/dash.py

PEP-257 says:

    def monitor(self):
        """The monitor where the dash is."""

> tests/autopilot/autopilot/emulators/unity/panel.py

We try to keep system includes together, so:
  from time import sleep
should be the line below
  import logging

I'd also prefer that instead of the emulator having its
own mouse, the move_mouse_* methods took a mouse parameter.

Same for the other emulators.

There is a lot of duplication in the emulators, but given
the need for expediency, I'm ok with this for now.

You don't need:
  panel_monitor = 0 in the base test case.

You shouldn't need a sleep at the start of a test method.

PanelMenuTests?

Are they coming?

> Lets give this a sensible default, like:
>
> std::string form_factor("unknown");

Fine.

> Do we really need drag_window_to_monitor?

Yes, we don't have any other way to move a window to a monitor or to be sure that it will be opened in a specified monitor, so this allow to do tests being quite sure that the window will be on the monitor we're focusing on.
We discussed with Thomi about that. Moving it programmatically can work, but it would reduce the real simulation that AP does.

> > tests/autopilot/autopilot/emulators/unity/dash.py
>
> PEP-257 says:
>
> def monitor(self):
> """The monitor where the dash is."""

Sorry, bad copy & paste...

> > tests/autopilot/autopilot/emulators/unity/panel.py
>
> We try to keep system includes together, so:
> from time import sleep
> should be the line below
> import logging

Fine

> I'd also prefer that instead of the emulator having its
> own mouse, the move_mouse_* methods took a mouse parameter.
>
> Same for the other emulators.

Mh, I don't like that too much... :)
Honestly I'd prefer to have a base class that inherits from UnityIntrospectionObject that has already these mouse-management methods included, to be able to just extend it to simply have the bits to get mouse control over any introspectable widget.

> You don't need:
> panel_monitor = 0 in the base test case.

Well, actually I need it, otherwise the non-scenario based tests won't work, since the self.panel_monitor value is not set, while it should fallback to 0 if the scenarios don't overwrite it.

> You shouldn't need a sleep at the start of a test method.

I shouldn't... Right. But "Show Desktop" is evil, and if I don't do that, when running all the tests with a multimonitor setup (so that they get repeated), it won't work.
I think this should fixed it somewhere else, and I preferred the trick than having a false-negative on the tests results.

> PanelMenuTests?
>
> Are they coming?

Sure.

The tests aren't perfect, and for sure there could be some cleanup. I've had intermittent failures with a few tests, but they pass when run individually.

No proposals found for merge of lp:~3v1n0/unity/panel-p-cleanup into lp:unity.

Now that the other branch is merged and not rejected ;)