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?
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.
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.
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.
Hi,
There are a few things that need to be fixed:
712 - .add("x", _center[0].x) windows" , (int)Windows( ).size( )) text", tooltip_text()) priority" , _sort_priority) active" , GetQuirk( QUIRK_ACTIVE) ) visible" , GetQuirk( QUIRK_VISIBLE) ) urgent" , GetQuirk( QUIRK_URGENT) ) running" , GetQuirk( QUIRK_RUNNING) ) presented" , GetQuirk( QUIRK_PRESENTED )); windows" , static_ cast<unsigned int>(Windows( ).size( ))) text", tooltip_text()) priority" , _sort_priority) QUIRK_ACTIVE) ) QUIRK_VISIBLE) ) QUIRK_URGENT) ) QUIRK_RUNNING) ) QUIRK_STARTING) ) QUIRK_DESAT) ) QUIRK_PRESENTED ));
713 - .add("y", _center[0].y)
714 - .add("z", _center[0].z)
715 - .add("related-
716 - .add("icon-type", _icon_type)
717 - .add("tooltip-
718 - .add("sort-
719 - .add("quirk-
720 - .add("quirk-
721 - .add("quirk-
722 - .add("quirk-
723 - .add("quirk-
724 + .add("center_x", _center[0].x)
725 + .add("center_y", _center[0].y)
726 + .add("center_z", _center[0].z)
727 + .add("related_
728 + .add("icon_type", _icon_type)
729 + .add("tooltip_
730 + .add("sort_
731 + .add("active", GetQuirk(
732 + .add("visible", GetQuirk(
733 + .add("urgent", GetQuirk(
734 + .add("running", GetQuirk(
735 + .add("starting", GetQuirk(
736 + .add("desaturated", GetQuirk(
737 + .add("presented", GetQuirk(
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): monitor_ geometry( monitor_ number) monitor_ geometry( monitor_ number)
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_
830 +
831 + (m_x, m_y, m_w, m_h) = self.get_
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 icon(self) :
911 + def show_embedded_
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( SimpleLauncherI con, 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(): geometry. get_num_ monitors( ) hide_mode' : 0, 'launcher_ primary_ only': False}), hide_mode' : 1, 'launcher_ primary_ only': False}), monitors) : hide_mode' : 0, 'launcher_ primary_ only': False} append( ('Monitor %d, Launcher never hide, on all monitors' % (i), scenario_setting)) hide_mode' : 0, 'launcher_ primary_ only': True} append( ('Monitor %d, Launcher never hide, only on primary monitor' % (i), scenario_setting)) hide_mode' : 1, 'launcher_ primary_ only': True} append( ('Monitor %d, Launcher autohide, on all monitors' % (i), scenario_setting)) hide_mode' : 1, 'launcher_ primary_ only': True} append( ('Monitor %d, Launcher autohide, only on primary monitor' % (i), scenario_setting))
1017 + screen_geometry = ScreenGeometry()
1018 + num_monitors = screen_
1019 +
1020 + scenarios = []
1021 +
1022 + if num_monitors == 1:
1023 + scenarios = [
1024 + ('Single Monitor, Launcher never hide', {'hud_monitor': 0, 'launcher_
1025 + ('Single Monitor, Launcher autohide', {'hud_monitor': 0, 'launcher_
1026 + ]
1027 + else:
1028 + for i in range(num_
1029 + scenario_setting = {'hud_monitor': i, 'launcher_
1030 + scenarios.
1031 +
1032 + scenario_setting = {'hud_monitor': i, 'launcher_
1033 + scenarios.
1034 +
1035 + scenario_setting = {'hud_monitor': i, 'launcher_
1036 + scenarios.
1037 +
1038 + scenario_setting = {'hud_monitor': i, 'launcher_
1039 + scenarios.
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,