Hi Dermot.
I have no further comments on all omitted replies. I had some cut and
paste issues, so I've numbered the individual items below to keep items
partitioned clearly.
1) From your original reply:
- - -
usr/src/cmd/gui-install/src/confirm_screen.py
---------------------------------------------
86: Would there ever be a non-error case where nextbutton is None or
installbutton is None?
No - they will only be None if the Glade .xml file has been tampered with.
This may be ratholing, but what would be appropriate error handing if
installbutton is None? Termination? If nextbutton happens to be
None, we can
just skip its hide_all() I suppose...
I think termination would be the only option.
We should probably have a consistent method of handling where
the widgets cannot be fetched from Glade in all the app's screens - I
look into doing this.
- - -
Did anything become of this?
usr/src/cmd/gui-install/src/timezone_screen.py:
-----------------------------------------------
2) I noticed imports of gobject and gtk were moved. Can imports be
alphabetized? (I was asked to do this when I did my Derived Manifests
push.) I also had one group of public python module imports, followed
by another group of our own module imports.
3) From your original reply:
- - -
285-292: Just curious: There is only set_value for year, whereas there is
set_text and set_value for the other time fields. I think this is
because
there is no way for the user to set the year, correct? (set_value is
needed to
check for leap-ears as ints, to verify the number of days in
February.) But if
we allow setting
other fields, why not allow setting the year too? (If we allow
setting the
month to December if it's May, why not allow setting the year to
something
other than the current year? It would be more consistent.)
No - the year can also be changed. set_text is called for the other
widgets
to ensure that values less than 10 are zero-padded. Year values don't have
zero-padding.
- - -
Please add a comment about this to clarify why this is done.
usr/src/cmd/gui-install/src/map.py:
----------------------------------
4) I notice the same handling of imports in this file as in
timezone_screen.py... Order?
5) From your original reply:
- - -
usr/src/cmd/gui-install/src/Makefile
------------------------------------
65: Does this line handle only gui-install and not the PYMODULES?
Looks like
it should be:
all: python $(PROGS) $(PYCMODULES)
done
- - -
I don't see this change.
Thanks,
Jack
On 06/14/11 06:29 AM, Dermot McCluskey wrote:
Hi,
This is round #2 of the code review for GUI Install -> CUD.
The new webrev, incorporating fixes from the previous round,
plus various other fixes from the past few weeks, is here:
http://cr.opensolaris.org/~dermot/webrev-cud-gui-round-2/
And an incremental webrev is here:
http://cr.opensolaris.org/~dermot/webrev-cud-gui-round-2_vs_round_1/
(this also lists unchanged file, unfortunately)
I would like to receive final comments by COB this Friday, June 17th,
with an aim to pushing the code early next week, in time for build 169.
Thanks,
- Dermot
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
