[caiman-discuss] Code Review request: GUI Install -> CUD
Hi Dermot, I've reviewed the majority of the code but I ran out of time
to get everything done. The things I haven't covered are the timezone
related files (I reivewed map.py briefly), user_support.py, the manifest
file and the glade/gtkbuilder XML files. IMO, since the XML builder
files are autogenerated from glade-3 I think it's much more important
that they be visually verified in the running app rather than reviewed
line by line in a code review. Anyway, here are my comments.... Thanks,
Niall <http://opensolaris.org/jive/thread.jspa?threadID=139576&tstart=0>
Hi Dermot,
I've reviewed the majority of the code but I ran out of time to get
everything covered. The things I haven't covered are timezone files
(reviewed map.py briefly),
user_support.py, the manifest file and the glade/gtkbuilder XML. IMO the
XML builder files are not really worth reviewing in much detail since
they auto-generated - better to visually inspect the end result in the
running app. Anyway here are my comments.
Overall it looks excellent... nice job!
usr/src/cmd/gui-aux/Makefile:
57 ROOTIMGURLMAP=
$(IMGURLMAP:%=$(ROOTUSRSHARE)/gui-install/installmessages/%)
Should $(ROOTUSRSHARE)/gui-install be substituted with $(ROOTGUIINST) as
defined in Makefile.master?
/usr/src/cmd/gui-install/pixmaps/Makefile:
31 diskupdate-selected.png \
32 diskupdate-unselected.png \
...
37 sun_curve_hacky2.png \
38 sun_logo.png \
I don't think we use or need these images any more and are artifacts
from Dwarf Caiman
usr/src/cmd/gui-install/src/confirm_screen.py:
80 if viewport is not None:
81 bg_color = gtk.gdk.Color("white")
82 viewport.modify_bg(gtk.STATE_NORMAL, bg_color)
Can we use the definition of COLOR_WHITE from
solaris_install.gui_install.gui_install_common here?
161 if profile is None:
162 return
This seems to handle a failure silently and result in a blank summary
screen being displayed to the user.
Is this really the desired result or should this be more fatal?
233 def licensechecked_callback(self, widget, data=None):
234 '''callback for "clicked" event on release notes button'''
The docstring should refer to the license check button rather than the
release notes button.
usr/src/cmd/gui-install/src/disk_screen.py:
36-57: Alphabetize imports
82 BUSY_MARKUP = "<span font_desc=\"Bold\">%s</span>" % BUSY_TEXT
743 markup = "<span font_desc=\"Bold\">%s</span>" % \
You can replace the outer "" quotes with single quotes '' which allows
you to then remove the leading '\'
escape character from the markup eg.
BUSY_MARKUP = '<span font_desc="Bold">%s</span>' % BUSY_TEXT
267 for slice in all_slices:
268 if slice.in_zpool == ROOT_POOL:
269 solaris_slice = slice
"slice" is a python reserved word. Can you change too "slc" instead
(like TD/TI uses)?
usr/src/cmd/gui-install/src/failure_screen.py
52 self.activate_stage_label("finishstaglabel")
This looks like it would raise a KeyError from activate_stage_label()..
"finishstagelabel" should be -> "finishstagelabel"
usr/src/cmd/gui-install/src/fdisk_panel.py:
36-41 Alpahabetize imports
397 try:
398 new_val = float(new_text)
399 except ValueError:
400 return
This is my fault but I think instead of using float() directly here we
should be using locale.atof() to account
for locale specific decimal place characters ("." "," etc.)
406 def size_focus_in(self, widget, event, index):
407 ''' Signal handler for when the partition size SpinButtons
408 receive a "focus-in-event" signal.
409 '''
410
411 pass
412
413 def size_focus_out(self, widget, event, index):
414 ''' Signal handler for when the partition size SpinButtons
415 receive a "focus-out-event" signal.
416 '''
417
418 pass
Why are these callbacks defined if they don't do anything? What is their
purpose?
509 self._add_footer_to_table(index)
510 index += 1
511
512 self._fdisktable.show()
513
It is unnecessary to increment 'index' at line 510 because it is no
longer referenced in the method
after line 509
986 # logicals run from ID 5 to 36, inc.
987 max_logicals = 36 - 5 + 1
I think max_logicals could be better expressed by using
solaris_install.target.libadm.MAX_EXT_PARTS:
MAX_EXT_PARTS = 32 # number of logical partitions
1041 ui_partition.compute_mix_size()
s/mix/min
usr/src/cmd/gui-install/src/finish_screen.py:
29 import logging
30 import shutil
31 import os
Alphabetize imports
111 '''method from the super that deals with the update
112 button being pressed'''
Can you move the closing docstring ''' comment delimiter to a new line?
usr/src/cmd/gui-install/src/gui_install_common.py:
other_instance_is_running():
This is a general, high level comment about the PIDFILE mechanism and
not a critical issue IMO, but
did you consider using a lock file mechanism using fcntl.flock() instead?
usr/src/cmd/gui-install/src/help_dialog.py:
88 def display(self, screen):
89 '''show the helpdialog and update the help text'''
90 self.helpdialog.show_all()
91
92 window = self.helpdialog.window
93 if window:
94 window.set_functions(gtk.gdk.FUNC_CLOSE)
Is it necessary to keep setting the window functions property? I would
expect it only needs to set once when the window is created in __init__.
map.py:330: def unset_hoverd_timezone(self):
timezone.py:292: self.map.unset_hoverd_timezone()
s/hoverd/hovered
usr/src/cmd/gui-install/src/progress_screen.py:
240 if fraction >= 1.0:
241 self.set_progress_fraction(1.0)
242 break
243 self.installationinfolabel.set_label(msg)
244 self.set_progress_fraction(fraction)
Can we move the UI updating back into the main thread (like we did for
target discovery completion) instead
of this separate thread? Updating the UI from outside the main gtk event
loop thread has caused problems in
the past. In particular accessiblity technologies don't seem to notice
updates happening outside of the main
event loop thread. If we copy the approach used in the td_completion
case, we could set up a main loop timer
event in setup_progress_handling() after starting the server thread.
335 def install_callback(self, status, failed_cps):
Same as above. Updates the UI from outside the main event loop.
usr/src/cmd/gui-install/src/screen_manager.py:
General - There is a lot of reference to "glade" which is now defunct
and replaced by gtkbuilder.
We should probably stop referring to glade and substitute with
"gtkbuilder" or "builder" more consistently.
140 "on_upgradebutton_clicked": self.no_op,
Upgrade has been dropped so we should remove items associated with it
from the gtkbuilder XML instead of giving them no-op signal handlers.
We can do this later if pressed for time though - won't break anything:)
214 bgcolor = gtk.gdk.Color("white")
215 viewport.modify_bg(gtk.STATE_NORMAL, bgcolor)
Why not use the WHITE definition from gui_install_common?
/usr/src/cmd/gui-install/src/target_utils.py:
39-41 Alphabetize imports
81 if primary:
82 lowest_name = 1
83 highest_name = 4
84 else:
85 lowest_name = 5
86 highest_name = 36
87
Please use the const definitions FD_NUMPART and MAX_EXT_PARTS from
solaris_install.target.libadm
163 # A couple of quick cosmetic changes to the value:
164 # - just take the first word, eg fibre channel
-> fibre
165 # - use uppercase (prefer ATA, SCSI to ata,
scsi, etc)
166 ctrl_type = ctrl_type.split()[0]
167 ctrl_type = ctrl_type.upper()
Looks a bit dodgy just randomly word chopping strings up and hoping for
the best :-)
usr/src/cmd/gui-install/src/textview_dialog.py:
55 def display(self):
56 '''show the helpdialog and update the help text'''
57
58 self.textviewdialog.show_all()
59 self.quitapp.append(self.textviewdialog)
60
Showing the textviewdialog should be done at the end rather than the
beginning. If you show it first, then
modify all it's properties and fill it with content then the user will
probably observe flicker.
usr/src/cmd/gui-install/src/user_screen.py:
32-37 Alphabetize imports
77-92 self.password = user_password = \
78 self.builder.get_object("userpassword1entry")
etc.
Why are you creating 2 references to the same object/widget for
password, confirm_password and host_name?
103 toplevel.show_all()
104 # hide the various error widgets
105 for widget_name in ["loginnameinfoimage",
"userpasswordinfoimage",
106 "userpasswordinfolabel",
"hostnameinfoimage"]:
107 widget = self.builder.get_object(widget_name)
108 widget.hide_all()
I'd move toplevel.show_all() down to the bottom of this block for the
same reasons mentioned just above,
it can cause flicker that the user might see.
276 '''valitdates the hostname'''
s/valitdates/validates
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
