Keith,
This is a review of the files in the UI Components
section.
I didn't find any definite errors, so these are just
questions, nit-picks, and suggestions:
usr/src/cmd/text-install/osol_install/text_install/color_theme.py
- the variable name error_msg might be misleading - I
expected it to be a string, but I think it's a set of
bitwise OR'ed text attributes for error messages?
usr/src/cmd/text-install/osol_install/text_install/edit_field.py
- this comment seems misleading:
104 area.lines is overridden to 1.
when what actually happens is:
165 if area.lines != 1:
166 raise ValueError("area.lines must be 1")
- this comment is also applicable is several other
instances where is_valid() is called without checking
the return value, so perhaps the comment belongs in
the function itself:
244 self.is_valid()
245 # Run self.is_valid here so that any functional side
effects can
246 # occur, but don't check the return value (removing a
character
247 # from a valid string should never be invalid, and even
if it were,
248 # it would not make sense to add the deleted character
back in)
- this function:
335 def run_on_exit(self):
returns a value if line 340 is TRUE, but doesn't reach a
return statement if it is FALSE. If you return a value in
some circumstances, you should do it always.
- several places throughout this file use values like:
365 self.textbox.do_command(ord(ctrl('a')))
If these are special, I'd suggest defining them in global
variables, like in inner_window.py, eg:
40 KEY_TAB = ord(ctrl('i'))
usr/src/cmd/text-install/osol_install/text_install/inner_window.py
- in function add_text(), you only raise an exception for
the text being too long to fit, if it is *centered*.
Couldn't it also be too long to fit if centered=False?
- do convert_paragraph() and translate_input() need to
be @staticmethod, or could they just be module functions
defined outside class InnerWindow? (ie are they called
from outside this file?)
usr/src/cmd/text-install/osol_install/text_install/main_window.py.html
- remove commented-out code:
196 # self.do_update()
- is 4 a special value here:
212 y_loc = 4
if so, a comment would be helpful
usr/src/cmd/text-install/osol_install/text_install/window_area.py
- I find this construct unusual:
50 result = ("lines=%s, columns=%s, y_loc=%s, x_loc=%s, "
51 "scrollable_lines=%s")
52 result = result % (self.lines, self.columns, self.y_loc,
self.x_loc,
53 self.scrollable_lines)
why the parentheses on lines 50/51? And why not combine
both statements into 1?
- Dermot
