Jack, (& question for John)
Thanks again for reviewing. Responses below.
- Dermot
On 06/16/11 21:52, Jack Schwartz wrote:
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?
Ok - I've addressed this now. Basically in all screens, where I call
GtkBuilder.get_object(), I have moved all these calls to the __init__
method; if any of the widgets are not found, then it displays an error
dialog and then raises RuntimeError, eg:
def __init__(self, builder):
super(ConfirmScreen, self).__init__(builder)
self.name = "Confirmation Screen"
self.logger = logging.getLogger(INSTALL_LOGGER_NAME)
self.confirmviewport = self.builder.get_object("confirmviewport")
self.nextbutton = self.builder.get_object("nextbutton")
self.installbutton = self.builder.get_object("installbutton")
self.licenseagreementlinkbutton = self.builder.get_object(
"licenseagreementlinkbutton")
self.licensecheck = self.builder.get_object("licensecheckbutton")
self.diskvbox = self.builder.get_object("diskvbox")
self.softwarevbox = self.builder.get_object("softwarevbox")
self.timezonevbox = self.builder.get_object("timezonevbox")
self.languagesvbox = self.builder.get_object("languagesvbox")
self.accountvbox = self.builder.get_object("accountvbox")
if None in [self.confirmviewport, self.nextbutton,
self.installbutton,
self.licenseagreementlinkbutton, self.licensecheck,
self.diskvbox, self.softwarevbox, self.timezonevbox,
self.languagesvbox, self.accountvbox]:
modal_dialog(_("Internal error"), GLADE_ERROR_MSG)
raise RuntimeError(GLADE_ERROR_MSG)
So, basically, if the Glade files have been tampered with, or do not
contain the expected content, the app will not start up for normal use.
I think this is the only sensible way to handle this situation.
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.
Drew asked me to do it this way. Other than the pygtk import,
which is treated as special and occurs first, all the other imports
are grouped into: standard, 3rd party, ours, and are alphabetized within
those groups.
I've just noticed that one of the imports in timezone_screen.py was still
not alphabetized (solaris_install.engine) and have fixed that now.
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.
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?
I think these are all consistent with the schema described above.
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.
Umm - I made this change and John un-did it when he
fixed some other stuff I broke, so perhaps it wasn't valid?
John - can you recall?
Thanks,
- Dermot
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
