Code review comment for lp:~3v1n0/unity/hud-lock-out-monitors-tests

Revision history for this message
Thomi Richards (thomir-deactivatedaccount) wrote :

Hi,

Almost there :)

Please change this:

81 + .add("desktop_id", glib::String(g_path_get_basename(DesktopFile().c_str())).Str())

to use the methods on the std::string. Something like this (untested, but should work):

std::string desktop_file = DesktopFile();
std::string desktop_id = desktop_file.substr(desktop_file.rfind('/'));

The current code involves way too much casting, uses c functions when we have c++ alternatives, and looks awful.

This docstringsd needs a period at the end:

762 + """Returns True if `rect` is _entirely_ on the specified monitor, with no overlap"""

Please add a blank line between the class docstring at the property:

797 """Proxy object for the hud view child of the controller."""
798 + @property
799 + def geometry(self):

Please add a docstring here:

814 + def get_embedded_icon(self):

...detailing what the possible return values are.

Please rename this:

858 + @property
859 + def center_geometry(self):

since you're not returning the geometry, just a point. Maybe call it "position", "center_position", or "center_point" or something. Also, please add a docstring, since you're returning x, y AND z properties, which might confuse people (who only expect X & Y).

Please revert this change:

966 - scenario_name = ','.join(names)
967 + scenario_name = ', '.join(names)

multiply_scenarios is from testscenarios. I won't want two functions that behave differently.

This should go in AutopilotTestCase:

1034 + screen_geo = ScreenGeometry()

Please add a blank line betweeb the class definition and the setUp method:

1081 +class HudBehaviorTests(HudTestsBase):
1082 + def setUp(self):

This test:

1245 + def test_hud_desaturates_launcher_icons(self):
1246 + """Tests that the launcher icons are desaturates when HUD is open"""
1247 +
1248 + self.reveal_hud()
1249 + sleep(.5)
1250 +
1251 + for icon in self.launcher.model.get_launcher_icons():
1252 + if not isinstance(icon, HudLauncherIcon):
1253 + self.assertTrue(icon.desaturated)

Is good, but we probably want another one that asserts that the hud icon is NOT desaturated.

As discussed on IRC, please change this:

975 + def start_app_c_locale(self, app_name, files=[]):
976 + """Start one of the known apps using the C locale, and kill it on tear down.
977 +
978 + if files is specified, start the application with the specified files.
979 + """
980 + os.putenv("LC_ALL", "C")
981 + self.start_app(app_name, files)
982 + self.addCleanup(os.unsetenv, "LC_ALL")

to something like this (changing the current start_app):

def start_app(self, app_name, files=[], locale=None):
        """Start one of the known apps, and kill it on tear down.

        If files is specified, start the application with the specified files.
        If locale is specified, the locale will be set when the application is launched.
        """
        if locale:
            os.putenv("LC_ALL", "C")
            self.addCleanup(os.unsetenv, "LC_ALL")
            logger.info("Starting application '%s' with files %r in locale %s", app_name, files, locale)
        else:
            logger.info("Starting application '%s' with files %r", app_name, files)
        app = self.KNOWN_APPS[app_name]
        self.bamf.launch_application(app['desktop-file'], files)
        self.addCleanup(call, ["killall", app['process-name']])

Then change these:

1132 - self.start_app('Text Editor', files=['/tmp/autopilot_gedit_undo_test_temp_file.txt'])
1133 + self.start_app_c_locale('Text Editor', files=['/tmp/autopilot_gedit_undo_test_temp_file.txt'])

1362 - self.start_app('Character Map')
1363 - self.start_app('Calculator')
1364 + self.start_app_c_locale('Character Map')
1365 + self.start_app_c_locale('Calculator')

to be like this:

self.start_app('Calculator', locale='C')

review: Needs Fixing

« Back to merge proposal