HI Dermot.
Lots of good stuff!
Here are sections 1,4,6,7:
4. Welcome Screen and Confirmation Screen
=============
usr/src/cmd/gui-install/src/welcome_screen.py
---------------------------------------------
37: I wish this URL didn't have to be hardwired. But...
The fact that it is out of date is probably only once-in-a-lifetime
since the
SUN link would probably be good if Oracle didn't take over, so an update
to it
would probably suffice.
50: Just curious: Does this have to match the "Welcome" on line 57 of
base_screen.py? If so, it would be helpful to remove the duplication of
hardwired values in two files.
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?
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...
Optimization: Have a single string that gets passed to 174 and 176, like
what
you already do on 223 and 226.
223: What if profile.loginname is None. What will the root password be?
246 and 253: I think you mean method which overrides the abstract method
in the
super class. The comments are not clear. You could just say "method to
override super class abstract method" or say simply "method that deals
with the
back button getting pressed".
7. User Screen
==========
usr/src/cmd/gui-install/src/user_screen.py
------------------------------------------
159: Nit: if data==None, this method will crash on line 172. Remove the
default of None.
285-286: can be boiled down to: char not in "-_."
293-294: Should this message be a warning instead of an error, since a
default
is returned?
usr/src/cmd/gui-install/src/user_support.py: Not reviewed
-------------------------------------------
6. Timezone screen
=============
usr/src/cmd/gui-install/src/timezone_screen.py:
----------------------------------------------
69: Custome -> Custom
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.)
usr/src/cmd/gui-install/src/timezone.py: OK...
---------------------------------------
usr/src/cmd/gui-install/src/map.py:
----------------------------------
334: Nit: Rename unset_hoverd_timezone() to unset_hovered_timezone() in this
file and in timezone.py line 291. After all, "hovered" is a real word that
describes what "hoverd" is.
449: Nit: sentence is grammatically incorrect.
450, 452: Nit: put "scale" on 1st line of statement to group with the
first ()
value it's multiplied with.
454: Put 452-453 into an else clause here. Move comment at 447 as
appropriate.
Can update_offset_with_scale() be used where update_offset() is used,
taking
scaling value of zero from self (or maybe turning off scaling via an
argument)?
If so, you can get rid of 19 lines of code (update_offset()).
Nit: 486: This line is not needed as set_magnifier_cursor() has that line as
well. (If you want to keep it in there for documentation purposes, that's
fine.)
553: Does it make sense to combine
TZContinent.__init__(), set_values_from_libzoneinfo_struct() and
populate() into
one method? They are all called in succession. Code could be condensed
a bit by not initializing things to None
and then to something else... Another reason for combining: populate()
depends
on set_values...().
605: Likewise for TZCountry __init__(),
set_values_from_libzoneinfo_struct() and
populate()
667: Likewise for TZTimezone __init__ and
set_values_from_libzoneinfo_struct().
694-697: Delete two of these lines?
722: self.longitude = -self.longitude
728: self.latitude = -self.latitude
755: Where does print print? Should this line be removed?
132-134, 358, 397: Remove commented debugging print lines.
usr/src/cmd/gui-install/src/libzoneinfo_ctypes.py
-------------------------------------------------
41-43, 45-57: lat_degree, lat_minute, lat_second, long_degree, long_minute,
long_second are all declared in struct tz_coord as unsigned int. Please
change
their declaration from C.c_int to C.c_uint.
There are a number of functions set up here which are not used, but I agree
that it is an eye to the future to implement all of the API here rather than
just the parts needed right now...
1. Makefiles and manifest
==================
usr/src/Makefile.master: OK
-----------------------
usr/src/cmd/Makefile: OK
--------------------
usr/src/cmd/Makefile.targ
-------------------------
189-194: Put theese after ROOTAUTOINSTSCPROFILES (which should go near
ROOTAUTOINSTMANIFEST).
It would also be nice for someone to add a comment to this file at 162
explaining the difference between the lines <162 and the lines >162
(which, I
believe is that there are no .pyc files delivered in the directories at
lines
>162).
usr/src/cmd/gui-aux/Makefile: OK
----------------------------
usr/src/cmd/gui-install/Makefile: OK
--------------------------------
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)
72: I don't understand why two waits are needed here.
If you set "all" up as above, I think you can do this:
install: all $(ROOTPYTHONVENDOR) $(ROOTPYTHONVENDORSOLINSTALL) \
$(ROOTPYTHONVENDORSOLINSTALLGUI) \
.WAIT \
$(ROOTPROGS) $(ROOTPYMODULES) $(ROOTPYCMODULES)
usr/src/cmd/gui-install/pixmaps/Makefile
----------------------------------------
Just curious: what does 53 do?
usr/src/cmd/gui-install/xml/Makefile
------------------------------------
If pixmaps/Makefile line 53 can be removed, then so can line 45 of this
file.
usr/src/pkg/manifests/system-install-gui-install.mf: OK
---------------------------------------------------
Thanks,
Jack
On 06/01/11 10:12, Dermot McCluskey wrote:
Hi,
I'd like to request a code review for the GUI Install -> CUD project.
I need to set a timeout of Friday June 10th for this first round of
comments.
Please let me know if you intend to review, and also which of the
sections
listed below you will be looking at.
The webrev is here:
http://cr.opensolaris.org/~dermot/webrev-cud-gui-round-1/
The latest iso and usb images for the project are here:
file:///net/indiana-build.us.oracle.com/var/lib/hudson/jobs/dc/cud_gui/media
Below is my suggested grouping of the sources. If reviewers think a code
walk-through would be helpful, then let me know. I could do this at, eg,
9am PT on Friday, 3rd or Monday, 6th or some other time if that doesn't
suit.
Thanks,
- Dermot
1. Makefiles and manifest
==================
usr/src/Makefile.master
usr/src/cmd/Makefile
usr/src/cmd/Makefile.targ
usr/src/cmd/gui-aux/Makefile
usr/src/cmd/gui-install/Makefile
usr/src/cmd/gui-install/src/Makefile
usr/src/cmd/gui-install/pixmaps/Makefile
usr/src/cmd/gui-install/xml/Makefile
usr/src/pkg/manifests/system-install-gui-install.mf
2. Glade UI Definition file
==================
(These files are basically the old *.glade files which define the Gtk+
widgets
and layout of each screen. They have been run through the
gtk-builder-convert
utility to convert them to the newer Gtk+Builder format, plus some minor
tidying up.)
usr/src/cmd/gui-install/xml/confirmation.xml
usr/src/cmd/gui-install/xml/date-time-zone.xml
usr/src/cmd/gui-install/xml/failure.xml
usr/src/cmd/gui-install/xml/gui-install.xml
usr/src/cmd/gui-install/xml/installation.xml
usr/src/cmd/gui-install/xml/installationdisk.xml
usr/src/cmd/gui-install/xml/users.xml
3. App startup and checkpoint setup; handle screens; misc.
===========================================
usr/src/cmd/gui-install/src/gui-install.py
usr/src/cmd/gui-install/src/__init__.py
usr/src/cmd/gui-install/src/base_screen.py
usr/src/cmd/gui-install/src/screen_manager.py
usr/src/cmd/gui-install/src/gui_install_common.py
usr/src/cmd/gui-install/src/help_dialog.py
usr/src/cmd/gui-install/src/install_profile.py
4. Welcome Screen and Confirmation Screen
=============
usr/src/cmd/gui-install/src/welcome_screen.py
usr/src/cmd/gui-install/src/confirm_screen.py
5. Disk Screen
==========
usr/src/cmd/gui-install/src/disk_screen.py
usr/src/cmd/gui-install/src/fdisk_panel.py
usr/src/cmd/gui-install/src/target_utils.py
usr/src/lib/install_target/controller.py
6. Timezone screen
=============
usr/src/cmd/gui-install/src/timezone_screen.py
usr/src/cmd/gui-install/src/timezone.py
usr/src/cmd/gui-install/src/map.py
usr/src/cmd/gui-install/src/libzoneinfo_ctypes.py
7. User Screen
==========
usr/src/cmd/gui-install/src/user_screen.py
usr/src/cmd/gui-install/src/user_support.py
8. Progress Screen / Perform Installation
===========================
usr/src/cmd/gui-install/src/progress_screen.py
usr/src/cmd/gui-install/src/failure_screen.py
usr/src/cmd/gui-install/src/finish_screen.py
usr/src/cmd/gui-install/src/textview_dialog.py
usr/src/cmd/gui-aux/imageurl.txt
usr/src/cmd/system-config/profile/user_info.py
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
