Code review comment for lp:~alexlauni/unity/unity.ap-switcher-emu

Hi,

A few things:

== switcher.py ==

1) Diff has several bits of trailing whitespace:

116 + @property
170 +
310 +
330 +

2) Your docstrings need to be PEP257 compliant. Specifically:

"""One line docstrings look like this."""

"""Multi-line docstrings start with a single sentence.

Then a newline, followed by one or more sentences. Multi-line
docstrings end with a blank line before the closing quotes as
well, like this.

"""

3) The docstring for 'Switcher.property' should be changes, as should the implementation. Currently it will sometimes return a typle of items, and sometimes return a single item. I suggest the following:

    @property
    def mode(self):
        """Returns a tuple of SwitcherMode items that describe the mode the switcher is in.

        For example, to check whether the switcher is in details mode:

        >>> SwitcherMode.DETAIL in self.switcher.mode
        ... True

        """
        if not self.visible:
            return tuple()
        if self.controller.model.detail_selection and not self.controller.model.only_detail_on_viewport:
            return (SwitcherMode.DETAIL, SwitcherMode.ALL)
        elif self.controller.model.detail_selection:
            return (SwitcherMode.DETAIL,)
        elif not self.controller.model.only_detail_on_viewport:
            return (SwitcherMode.ALL,)
        else:
            return (SwitcherMode.NORMAL,)

== test_switcher.py ==

1) Unused import: NotEquals

2) PEP8 compliance: need 2 blank lines between module-level blocks. So 2 blank lines between the end of one class and the start of the next, and between the end of the import block and the start of the first class.

3) Docstring issues here similar to the switcher.py file.

4) Lots of trailing whitespace in this file!

5) Should probably use Eventually in the first few tests of SwitcherTests class, for example:

    def test_witcher_starts_in_normal_mode(self):
        """Switcher must start in normal (i.e.- not details) mode.

        """
        self.start_app("Character Map")
        sleep(1)

        self.switcher.initiate()
        self.addCleanup(self.switcher.terminate)
        self.assertThat(self.switcher.mode, Eventually(Equals(SwitcherMode.NORMAL)))

6) Is the alt+Tab timeout value an introspectable property? If not, can we make it one? THen we can replace this:

370 + def set_timeout_setting(self, value):
371 + self.set_unity_option("alt_tab_timeout", value)
372 + sleep(1)

With this:

370 + def set_timeout_setting(self, value):
371 + self.set_unity_option("alt_tab_timeout", value)
372 + self.switcher.alt_tab_timeout.wait_for(value)

Otherwise, looks good - all switcher tests pass for me locally.