Thank you for the review Dermot. I'll address your comments in-line
below. Did you have any new comments based on the updated webrev?
- Keith
Dermot McCluskey wrote:
> 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?
All fields of this class are bitwise OR'ed attributes that can be
interpreted by curses calls. I'll review and update the comments in this
file to make sure that's more clear.
>>
>>
>> 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")
Thank you for catching that. I'll update the comment.
>>
>> - 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)
The only other instance I see is in EditField._set_text(). I need to
review and make sure that calling is_valid() there is still necessary,
and will add an appropriate comment.
>>
>> - 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.
Good catch. I'll update it to return True if self.on_exit is not callable.
>>
>> - 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'))
I'll make this change. I'm not sure why I did it the other way in the
first place, but your suggestion will be more readable and more consistent.
>>
>>
>> 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?
Actually, I'll remove the exception. When centered is False, extra text
is trimmed off instead of exceptions being thrown. This was done so
that, for example, if the translated version of some text doesn't fit,
it gets cut off instead of causing the program abort from an obscure
Python exception.
>>
>> - 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?)
convert_paragraph is tied closely to add_paragraph, so it makes sense
that they are both part of the class.
translate_input is used by EditField, a sub-class of InnerWindow, so
leaving it as a staticmethod makes more sense to keep it in the class
hierarchy.
>>
>>
>> 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?
The parentheses provide simply one way for cleanly wrapping a long line
of text to a second line. Without the parentheses, that line would be:
result = "lines=%s, columns=%s, y_loc=%s, x_loc=%s, " + \
"scrollable_lines=%s"
I try to avoid wrapping lines using the trailing slashes. It also
provides consistency with other wrapped strings - any code that will
need translation will look similar.
>> And why not combine
>> both statements into 1?
I split it into two lines to break it up instead of having a single, 3
line long statement:
return ("lines=%s, columns=%s, y_loc=%s, x_loc=%s, "
"scrollable_lines=%s") % (self.lines, self.columns,
self.y_loc,
self.x_loc, self.scrollable_lines)
Perhaps more readable would be something like:
result = ("lines=%s, columns=%s, y_loc=%s, x_loc=%s, "
"scrollable_lines=%s")
vals = (self.lines, self.columns, self.y_loc, self.x_loc,
self.scrollable_lines)
return result % vals
- Keith
>>
>>
>> - Dermot
>> _______________________________________________
>> caiman-discuss mailing list
>> caiman-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
>
