Hi,

There are a few things that need to be fixed:

712	- .add("x", _center[0].x)
713	- .add("y", _center[0].y)
714	- .add("z", _center[0].z)
715	- .add("related-windows", (int)Windows().size())
716	- .add("icon-type", _icon_type)
717	- .add("tooltip-text", tooltip_text())
718	- .add("sort-priority", _sort_priority)
719	- .add("quirk-active", GetQuirk(QUIRK_ACTIVE))
720	- .add("quirk-visible", GetQuirk(QUIRK_VISIBLE))
721	- .add("quirk-urgent", GetQuirk(QUIRK_URGENT))
722	- .add("quirk-running", GetQuirk(QUIRK_RUNNING))
723	- .add("quirk-presented", GetQuirk(QUIRK_PRESENTED));
724	+ .add("center_x", _center[0].x)
725	+ .add("center_y", _center[0].y)
726	+ .add("center_z", _center[0].z)
727	+ .add("related_windows", static_cast<unsigned int>(Windows().size()))
728	+ .add("icon_type", _icon_type)
729	+ .add("tooltip_text", tooltip_text())
730	+ .add("sort_priority", _sort_priority)
731	+ .add("active", GetQuirk(QUIRK_ACTIVE))
732	+ .add("visible", GetQuirk(QUIRK_VISIBLE))
733	+ .add("urgent", GetQuirk(QUIRK_URGENT))
734	+ .add("running", GetQuirk(QUIRK_RUNNING))
735	+ .add("starting", GetQuirk(QUIRK_STARTING))
736	+ .add("desaturated", GetQuirk(QUIRK_DESAT))
737	+ .add("presented", GetQuirk(QUIRK_PRESENTED));

Hyphens ('-') get translated to underscores ('_') in python automatically. You don't need to do this in the C++ code. It doesn't hurt, but you don't need to do it. More importantly, I don't think renaming 'x', 'y', 'z' to center_x, center_y, and center_z is a good idea. It seems natural to me that the x,y & z properties represent the center of the icon. If you are going to change this, please make sure that all the autopilot tests are updated accordingly.

There's something very wrong with this code:

823	+ def is_rect_on_monitor(self, monitor_number, rect):
824	+ """Returns True if `rect` is _entirely_ on the specified monitor, with no overlap"""
825	+
826	+ if type(rect) is not tuple or len(rect) != 4:
827	+ raise TypeError("rect must be a tuple of 4 int elements.")
828	+
829	+ (x, y, w, h) = self.get_monitor_geometry(monitor_number)
830	+
831	+ (m_x, m_y, m_w, m_h) = self.get_monitor_geometry(monitor_number)
832	+ return (x >= m_x and x + w <= m_x + m_w and y >= m_y and y + h <= m_y + m_h)
833	+

...Can you spot it? What exactly is 'rect' used for?

THis property:

910	+ @property
911	+ def show_embedded_icon(self):

Should be renamed. Either 'showing_embedded_icon' or 'has_embedded_icon', or maybe even 'displaying_embedded_icon'. 'show' is a verb, and shouldn't be used in property names.

I notice that you've changed this code:

970	icons = self.get_children_by_type(SimpleLauncherIcon, desktop_file=desktop_file)
971	- return icons or None
972	+ if len(icons):
973	+ return icons[0]
974	+
975	+ return None
976	+

It used to return a list or none, now it returns an instance or none. Have you updated everywhere that called this method to cope with the change? Further, are you *absolutely* sure that it's impossible for there to be more than one launcher icon with the same desktop file? I'm pretty sure Jason said that this could happen, whcih is why I was returning a list.

This:

1016	+def _make_scenarios():
1017	+ screen_geometry = ScreenGeometry()
1018	+ num_monitors = screen_geometry.get_num_monitors()
1019	+
1020	+ scenarios = []
1021	+
1022	+ if num_monitors == 1:
1023	+ scenarios = [
1024	+ ('Single Monitor, Launcher never hide', {'hud_monitor': 0, 'launcher_hide_mode': 0, 'launcher_primary_only': False}),
1025	+ ('Single Monitor, Launcher autohide', {'hud_monitor': 0, 'launcher_hide_mode': 1, 'launcher_primary_only': False}),
1026	+ ]
1027	+ else:
1028	+ for i in range(num_monitors):
1029	+ scenario_setting = {'hud_monitor': i, 'launcher_hide_mode': 0, 'launcher_primary_only': False}
1030	+ scenarios.append(('Monitor %d, Launcher never hide, on all monitors' % (i), scenario_setting))
1031	+
1032	+ scenario_setting = {'hud_monitor': i, 'launcher_hide_mode': 0, 'launcher_primary_only': True}
1033	+ scenarios.append(('Monitor %d, Launcher never hide, only on primary monitor' % (i), scenario_setting))
1034	+
1035	+ scenario_setting = {'hud_monitor': i, 'launcher_hide_mode': 1, 'launcher_primary_only': True}
1036	+ scenarios.append(('Monitor %d, Launcher autohide, on all monitors' % (i), scenario_setting))
1037	+
1038	+ scenario_setting = {'hud_monitor': i, 'launcher_hide_mode': 1, 'launcher_primary_only': True}
1039	+ scenarios.append(('Monitor %d, Launcher autohide, only on primary monitor' % (i), scenario_setting))
1040	+
1041	+ return scenarios
1042	+

Is better written using 'multiply_scenarios', imported from autopilot.tests. You pass it a list of scenarios for each axis, and it produces the dot product of all the scenarios. So you'll need a list for launcher_hide_mode, a list for launcher_primary_only, and a list for each monitor currently configured. We use this function in the ibus tests, so look there for an example.

Please don't print things like this:

1202	+ print calc.icon;

If you want to log something, use the logging framework. That way it gets attached to test results.

I think that this code:

1211	+ icons = BFBLauncherIcon.get_all_instances()
1212	+ self.assertEqual(1, len(icons))
1213	+ bfb_icon = icons[0]

Is useful enough to get it's own method in the model, don't you? THere should only ever be one BFB icon... 


Several of your tests do this:

1227	+ if not self.hud.is_locked_to_launcher:
1228	+ self.skipTest("This test needs a locked launcher")

Can you please take all these tests and put them in their own class, and change the scenario generation so we don't need to issue skipTest exceptions? Skipping a test should be a last resort.


Finally, since you have made several changes to the autopilot infrastructure, please run all the autopilot tests and verify that they all pass for you.


Cheers,