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

Hi,

44 + def is_rect_on_monitor(self, monitor_number, x, y, w, h):
45 + (m_x, m_y, m_w, m_h) = self.get_monitor_geometry(monitor_number)
46 + return (x >= m_x and x + w <= m_x + m_w and y >= m_y and y + h <= m_y + m_h)
47 +

Instead of passing x,y,w & h as separate parameters, just pass 'rect', and make sure it's a tuple in the function:
def is_rect_on_monitor(self, monitor_number, rect):
    """Returns True if `rect` is _entirely_ on the specified monitor, with no overlap"""
    if type(rect) is not tuple:
        raise TypeError("rect must be a tuple.")

...if you want you can also check that monitor_number is a valid integer, and that the tuple values are all ints.

This:

59 + def get_geometry(self):
60 + return (self.x, self.y, self.width, self.height)
61 +

should be a property, like this:

58 + @property
59 + def geometry(self):
60 + return (self.x, self.y, self.width, self.height)
61 +

this:

84 + screen_geometry = ScreenGeometry()
85 + num_monitors = screen_geometry.get_num_monitors()
86 +
87 + if num_monitors == 1:
88 + scenarios = [
89 + ('Single Monitor, Launcher never hide', {'hud_monitor': 0, 'launcher_hide_mode': 0}),
90 + ('Single Monitor, Launcher autohide', {'hud_monitor': 0, 'launcher_hide_mode': 1}),
91 + ]
92 + else:
93 + scenarios = []
94 + for i in range(num_monitors):
95 + scenarios.append(('Monitor %d, Launcher never hide' % (i), {'hud_monitor': i, 'launcher_hide_mode': 0}))
96 + scenarios.append(('Monitor %d, Launcher autohide' % (i), {'hud_monitor': i, 'launcher_hide_mode': 1}))

is kind of ugly. A better way to do it is how we do it in the launcher tests - use a function to generate the scenarios.

The first two changes I mentioned mean that you can rewrite this:

112 + (x, y, w, h) = self.hud.get_geometry()
113 + self.assertTrue(self.screen_geometry.is_rect_on_monitor(self.hud_monitor, x, y, w, h))

like this:

self.assertTrue(self.screen_geometry.is_rect_on_monitor(self.hud_monitor, self.hud.geometry)

The new test:

131 + def test_hud_dont_change_launcher_status(self):

...needs a docstring.

Finally, please use matchers for everything except simple true/false tests, so this:

139 + self.assertEqual(launcher_shows_pre, launcher_shows_post)

becomes:

self.assertThat(launcher_shows_pre, Equals(launcher_shows_post))

Cheers,