Keith,
I checked the updated webrev and all my comments
still apply there, except for the two comments on
main_window.py.
- Dermot
Dermot McCluskey wrote:
> 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
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
