Ooops... Myabe I was too tired yesterday night, I figured there was still some debugging code around, sorry.

> Hi,
> 
> There are a few things that need to be fixed:
> 
> 728     + .add("icon_type", _icon_type)

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

Yes, I know, but it also was to keep the same way of naming the introspection parameters.
Plus, there was no need to include the "quirk-" prefix.

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

Yes, maybe you are, but I'm not sure we're all since there was this code:
  target_y = icon.y + (self.icon_size / 2)
  self._mouse.move(target_x, target_y )
  self._mouse.click(button)

Also, considering that everywhere we consider the X / Y points as the coordinates of the top left edge, we should just apply this to icons too.

> If you are going to change this, please make sure that all the autopilot tests
> are updated accordingly.

Yes, I've done that.

> There's something very wrong with this code:
> 
> 823     + def is_rect_on_monitor(self, monitor_number, rect):


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

Yes, sorry it should have been used here:

> 829     + (x, y, w, h) = self.get_monitor_geometry(monitor_number)

Maybe a copy&paste typo.

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

Yes, actually I should have removed that, since I don't use it anymore (I used on previous revisions).
I've updated it against new API and new name, btw.

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

Yes, it was called only once in an HUD test, that I've updated moving to desktop_id

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

No, it's currently impossible (at least, it shouldn't happen unless we have bamf crashes, but this something to be fixed elsewhere) that two icons have the same desktop file, considering by "desktop file" the .desktop full path.
If it happens, btw it's safe to just take the first icon.
Otherwise we should just rename it into get_icons_by_desktop_file (plural form, I mean) since the current name could lead to some misunderstanding.
Anyway I'm for the only-one-icon way: it simplifies the usage and just works.

> This:
> 
> 1016    +def _make_scenarios():
> 1017    + screen_geometry = ScreenGeometry()
> 1018    + num_monitors = screen_geometry.get_num_monitors()

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

Ok, thanks... I'll look to that.

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

Actually I just forgot to remove that :/

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

Yes, but I was wondering where it was better to add it...

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

Yes, I was wondering the same...

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

They pass to me, at least the ones that always passed...