Jack, (& John)
Thanks for reviewing. Responses below.
On 06/10/11 02:45, Jack Schwartz wrote:
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.
I've been in touch with Marketing and they will provide
us with the new URL next week.
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.
No - one is the title that appears over the main area of the application's
main window, the other is one of the labels down the left-hand side of
the window that show which stage of the install you are currently at.
For most screens these two are the same, but sometimes they are not.
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.
Optimization: Have a single string that gets passed to 174 and 176,
like what
you already do on 223 and 226.
Fixed
223: What if profile.loginname is None. What will the root password be?
The code in user_screen.py will prevent the user moving past that screen
if loginname is None, so this is just a sanity check, really.
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".
Changed to:
''' Deals with the Back button being pressed.
Overrides abstract method defined in superclass.'''
and
''' Deals with the Install button being pressed.
Overrides abstract method defined in superclass.'''
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.
This has just recently been fixed. It now returns immediately
if data is None. We need to retain the default value, as these
methods are implementing Gtk+ signal handlers whose signature
is already defined.
285-286: can be boiled down to: char not in "-_."
Has recently been changed to:
viable = string.letters + string.digits + "-_."
if " " in hostname or hostname.translate(None, viable):
return False
293-294: Should this message be a warning instead of an error, since a
default
is returned?
We're just copying the behavior of the old app here.
It also resets the default hostname and notifies the user of the "error".
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
fixed
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.
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.
Fixed (I was just re-using the function names from the original map.c
code, which also has this "anomaly".)
449: Nit: sentence is grammatically incorrect.
removed (again, that comment came directly from the original .c code)
450, 452: Nit: put "scale" on 1st line of statement to group with the
first ()
value it's multiplied with.
done
454: Put 452-453 into an else clause here. Move comment at 447 as
appropriate.
done
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()).
No - despite the similar names, these methods are more different:
update_offset() updates self.x, self.y, self.xoffset and self.yoffset, while
update_offset_with_scale() only updates self.xoffset and self.yoffset
I've added docstring comments to both methods to indicate this.
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.)
removed
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...().
Not quite - you 'll see that TZWorld adds a "fake" continent for the GMT/UTC
timezone, and for which neither set_values_...() nor populate() are called.
However, I can see the benefit of what you are suggesting, so I'm making the
following change:
TZWorld: combine __init__() and populate()
TZContinent: combine get_values_...() and populate()
TZCountry: combine get_values_...() and populate()
TZTimezone: rename get_values_...() to populate() (for consistency)
(I could combine the remaining __init__ and populate methods of TZCountry
and TZTimezone, but I prefer to keep them separate, for symmetry with
TZContinent,
if that's OK?)
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?
done
722: self.longitude = -self.longitude
done
728: self.latitude = -self.latitude
done
755: Where does print print? Should this line be removed?
removed
132-134, 358, 397: Remove commented debugging print lines.
removed
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.
done
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...
agree
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).
done
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).
agree - but I'm not certain either, so I won't do so now.
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)
done
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)
done
usr/src/cmd/gui-install/pixmaps/Makefile
----------------------------------------
Just curious: what does 53 do?
Haven't a clue!
Leaving for John.
usr/src/cmd/gui-install/xml/Makefile
------------------------------------
If pixmaps/Makefile line 53 can be removed, then so can line 45 of
this file.
Leaving for John.
usr/src/pkg/manifests/system-install-gui-install.mf: OK
---------------------------------------------------
Thanks,
Jack
Many thanks,
- Dermot
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
