Hi Niall,
Thanks for reviewing (despite the conflict of interest in reviewing
code you contributed to ;) ).
Responses below.
On 06/10/11 16:24, Niall Power wrote:
[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!
Thanks!
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?
done
/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
agreed. also, sun_logo was referenced in gui-install.xml, so that bit
needs to be
deleted also.
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?
done
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?
agreed. replaced return with:
raise RuntimeError("INTERNAL ERROR: Unable to retrieve "
"InstallProfile from DataObjectCache")
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.
fixed
usr/src/cmd/gui-install/src/disk_screen.py:
36-57: Alphabetize imports
fixed
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
done - also fixed similar in base_screen.py and fdisk_panel.py
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)?
done
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"
It won't raise an error, as it doesn't directly access the dictionary with
that string as the key - it presumably just doesn't work correctly.
Anyway: fixed.
usr/src/cmd/gui-install/src/fdisk_panel.py:
36-41 Alpahabetize imports
done
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.)
Leaving for Niall...
just kidding: used locale.atof, as per its use in previous
method, size_insert_text()
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?
I think similar methods must have been present in the old code and
I wrote those stubs, intending to add functionality as needed. However,
I found that no special handling was needed.
I have removed them now.
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
ok - removed
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
agreed - changed as suggested
1041 ui_partition.compute_mix_size()
s/mix/min
fixed
usr/src/cmd/gui-install/src/finish_screen.py:
29 import logging
30 import shutil
31 import os
Alphabetize imports
done
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?
done - also reworded text
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?
I did not. Would that be more robust? This method
seems to work pretty well - I've tested it quite a bit.
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__.
agree - fixed as suggested
map.py:330: def unset_hoverd_timezone(self):
timezone.py:292: self.map.unset_hoverd_timezone()
s/hoverd/hovered
done
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.
Yes, I agree. There was a strange, intermittent hang on the Disk
Screen that went away when you fixed it as above, so I think it's
good practice to do this on the progress screen, even if there haven't
been any reported problems yet.
will fix as you suggest.
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.
As I understand it, it's *lib*glade that is deprecated, the Glade
application
is alive and well? And the UI was generated, and will continue to be
generated, using Glade, right?
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:)
will fix later ;)
214 bgcolor = gtk.gdk.Color("white")
215 viewport.modify_bg(gtk.STATE_NORMAL, bgcolor)
Why not use the WHITE definition from gui_install_common?
done
/usr/src/cmd/gui-install/src/target_utils.py:
this entire module has not been removed and this functionality
is now provided by the Disk class.
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 :-)
The possible values here are from the list defined in
install_target/libdiskmgt/const.py in CTYPE_*
I did check that this conversion would produce suitable
values!
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.
fixed as suggested
usr/src/cmd/gui-install/src/user_screen.py:
32-37 Alphabetize imports
done
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?
agreed - removed variables: user_password, confirm_password, 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.
That won't work. What's happening here is it shows everything,
then selectively hides the error images within toplevel.
If show_all is moved to the bottom, it will un-do the
hiding and show the error images.
276 '''valitdates the hostname'''
s/valitdates/validates
- dermot
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
