Keith,
On 12/17/09 00:11, Keith Mitchell wrote:
> 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?
No - I've reviewed the updated webrev and have no additional
comments on it.
- Dermot
>
> - 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
>>
