Keith Mitchell wrote:
> Hi Glenn and all other reviewers,
>
> I have posted an updated webrev at:
> http://cr.opensolaris.org/~kemitche/text_v2/
>
> Additionally, the opensolaris.org project gate has been updated to match
> those changes.
> hg clone ssh://anon at hg.opensolaris.org/hg/caiman/text_installer
>
> Please disregard the initial webrev. The updated webrev contains the
> most recent changes project files, and some of those files have changed
> significantly since the first webrev. An updated breakdown of which
> files fall into which sections is below.
>
> Thanks,
> Keith
>
> Breakdown of file groupings:
>
> Installation Profile:
> cmd/text-install/osol_install/profile/__init__.py
> cmd/text-install/osol_install/profile/disk_info.py*
> cmd/text-install/osol_install/profile/install_profile.py
> cmd/text-install/osol_install/profile/ip_address.py
> cmd/text-install/osol_install/profile/network_info.py
> cmd/text-install/osol_install/profile/system_info.py
> cmd/text-install/osol_install/profile/user_info.py
>
> * The changes mentioned in previous emails are present in the updated
> webrev; please DO review this file
>
> UI Components:
> cmd/text-install/osol_install/text_install/action.py
> cmd/text-install/osol_install/text_install/base_screen.py*
> cmd/text-install/osol_install/text_install/color_theme.py
> cmd/text-install/osol_install/text_install/edit_field.py
> cmd/text-install/osol_install/text_install/error_window.py
> cmd/text-install/osol_install/text_install/inner_window.py
> cmd/text-install/osol_install/text_install/list_item.py
> cmd/text-install/osol_install/text_install/main_window.py
> cmd/text-install/osol_install/text_install/scroll_window.py
> cmd/text-install/osol_install/text_install/window_area.py
>
> * This file is also relevant to the "Screens" group
>
> Screens:
> cmd/text-install/osol_install/text_install/date_time.py
> cmd/text-install/osol_install/text_install/disk_selection.py
> cmd/text-install/osol_install/text_install/fdisk_partitions.py
> cmd/text-install/osol_install/text_install/help_screen.py
> cmd/text-install/osol_install/text_install/install_progress.py
> cmd/text-install/osol_install/text_install/install_status.py
> cmd/text-install/osol_install/text_install/log_viewer.py
> cmd/text-install/osol_install/text_install/network_nic_configure.py
> cmd/text-install/osol_install/text_install/network_nic_select.py
> cmd/text-install/osol_install/text_install/network_type.py
> cmd/text-install/osol_install/text_install/partition_edit_screen.py
> cmd/text-install/osol_install/text_install/summary.py
> cmd/text-install/osol_install/text_install/timezone.py
> cmd/text-install/osol_install/text_install/timezone_locations.py
> cmd/text-install/osol_install/text_install/timezone_regions.py
> cmd/text-install/osol_install/text_install/users.py
> cmd/text-install/osol_install/text_install/welcome.py
>
> Text Install python core:
> cmd/text-install/osol_install/text_install/__init__.py
> cmd/text-install/osol_install/text_install/disk_window.py*
> cmd/text-install/osol_install/text_install/screen_list.py
> cmd/text-install/osol_install/text_install/text-install.py**
> cmd/text-install/osol_install/text_install/ti_install_utils.py
> cmd/text-install/osol_install/text_install/ti_install.py
>
> * This subclass of "InnerWindow" has enough knowledge of Text Installer
> specifics that it can't be classified as a 'generic' UI component
> ** This is the 'main' program
>
> Image/Distro-const:
> cmd/distro_const/auto_install/ai_plat_setup.py
> cmd/distro_const/auto_install/ai_post_boot_archive_pkg_image_mod
> cmd/distro_const/auto_install/ai_sparc_image.xml
> cmd/distro_const/auto_install/ai_x86_image.xml
> cmd/distro_const/text_install/text_mode_sparc.xml
> cmd/distro_const/text_install/text_mode_x86.xml
> cmd/distro_const/text_install/tm_gen_cd_content
> cmd/distro_const/text_install/tm_generic_live.xml
> cmd/distro_const/text_install/tm_pre_boot_archive_pkg_image_mod
> cmd/distro_const/utils/grub_setup.py
> cmd/distro_const/utils/Makefile
> cmd/distro_const/utils/plat_setup.py
> cmd/distro_const/utils/post_boot_archive_pkg_image_mod_custom
> cmd/slim-install/finish/install-finish
> cmd/slim-install/svc/media-fs-root
> cmd/text-install/svc/text-mode-menu.xml
> cmd/text-install/text-mode-menu/text-mode-menu.ksh
>
> Library Changes:
> lib/libtarget_pymod/discover.c
> lib/libtarget_pymod/disk.c
> lib/libtarget_pymod/disk.h
> lib/libtarget_pymod/geometry.c
> lib/libtarget_pymod/geometry.h
> lib/libtarget_pymod/instantiate.c
> lib/libtarget_pymod/instantiate.h
> lib/libtarget_pymod/mapfile-vers
> lib/libtarget_pymod/partition.c
> lib/libtarget_pymod/partition.h
> lib/libtarget_pymod/slice.c
> lib/libtarget_pymod/slice.h
> lib/libtarget_pymod/tgt.c
> lib/libtarget_pymod/tgt.h
> lib/libtarget_pymod/zpool.c
> lib/libtarget_pymod/zpool.h
> lib/libtd/td_api.h
> lib/libtd/td_dd.c
> lib/libtd/td_dd.h
> lib/libtd/test_td.c
> lib/libti/ti_api.h
> lib/libzoneinfo_pymod/libzoneinfo.c
>
> Packaging/Makefiles: Package definitions, dependencies, makefiles, and
> so forth
> Makefile.master
> README
> Targetdirs
> cmd/Makefile
> cmd/Makefile.cmd
> cmd/Makefile.targ
> cmd/distro_const/Makefile
> cmd/distro_const/auto_install/Makefile
> cmd/distro_const/text_install/Makefile
> cmd/text-install/Makefile
> cmd/text-install/helpfiles/Makefile
> cmd/text-install/osol_install/profile/Makefile
> cmd/text-install/osol_install/text_install/Makefile
> cmd/text-install/svc/Makefile
> cmd/text-install/text-mode-menu/Makefile
> lib/Makefile
> lib/libict/Makefile
> lib/libtarget_pymod/Makefile
> lib/libtd/Makefile
> lib/libzoneinfo_pymod/Makefile
>
> Other: (Help Files, miscellaneous)*
> cmd/text-install/helpfiles/date_time.txt
> cmd/text-install/helpfiles/disks.txt
> cmd/text-install/helpfiles/network.txt
> cmd/text-install/helpfiles/network_manual.txt
> cmd/text-install/helpfiles/sparc_solaris_slices.txt
> cmd/text-install/helpfiles/sparc_solaris_slices_select.txt
> cmd/text-install/helpfiles/summary.txt
> cmd/text-install/helpfiles/timezone.txt
> cmd/text-install/helpfiles/users.txt
> cmd/text-install/helpfiles/welcome.txt
> cmd/text-install/helpfiles/x86_fdisk_partitions.txt
> cmd/text-install/helpfiles/x86_fdisk_partitions_select.txt
> cmd/text-install/helpfiles/x86_fdisk_slices.txt
> cmd/text-install/helpfiles/x86_fdisk_slices_select.txt
> cmd/text-install/osol_install/__init__.py**
>
> * The help text files are under separate review.
> ** This file is not packaged, as osol_install/__init__.py is delivered
> by SUNWinstall. It exists so that the UI can be run directly from the
> workspace.
>
> Glenn Lagasse wrote:
>> Hi Keith,
>>
>> * Keith Mitchell (Keith.Mitchell at Sun.COM) wrote:
>>
>>> All,
>>>
>>> As of yet, we've not received any code review comments for the first
>>> round of review. I realize that there is a lot of code, so to
>>> facilitate reviewing, I've broken down the files into the
>>> subsections (see below for which files fall into which subsections).
>>> Reviewers may find it easier to select one or two subsections for
>>> review.
>>>
>>
>> Is this [1] link still the link to use for reviewing? I ask because the
>> DC xml files for building the TM image don't appear to be updated for
>> the post-VMC integration (specifically they don't include the im-pop
>> finalizer script).
>>
>> Cheers,
>>
>>
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
Hey Keith,
Here is my feedback. Many of my comments are nit/questions. My Python
experience is limited still. So many of my comments my be due to naivety.
I may have learned more from doing this review than I provide.
Hope this help.
Joe
---
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
From code walkthrough:
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Comment:
--------
scroll_window.py
I have a comment about init_scroll_bar.
disk_window.py has:
self.right_win.init_scroll_bar()
Why init_scroll_bar is sometimes passed, FALSE, TRUE and
nothing indicates to me this is not a BOOLIAN. It actually has
3 values. I think the argument should accept something like:
DRAW_SCROLL_BAR, REMOVE_SCROLL_BAR, UNALTER_SCROLL_BAR
Comment:
--------
Why the x_loc/y_loc level window management? Isn't there already
something which exists that can be used by Python to do this on
a char. based screen?
e.g.: http://excess.org/urwid/
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Comment from code I deskcheck reviewed:
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
I reviewed:
cmd/text-install/osol_install/text_install/date_time.py
cmd/text-install/osol_install/text_install/help_screen.py
cmd/text-install/osol_install/text_install/network_nic_configure.py
cmd/text-install/osol_install/text_install/network_nic_select.py
cmd/text-install/osol_install/text_install/network_type.py
cmd/text-install/osol_install/text_install/partition_edit_screen.py
cmd/text-install/osol_install/text_install/timezone.py
cmd/text-install/osol_install/text_install/timezone_locations.py
cmd/text-install/osol_install/text_install/timezone_regions.py
General comments
-------------------------------------------------------------------------------
Comment:
--------
Is this code supposted to be PEP8 ?
e.g.:
pylint --include-ids=y --rcfile=../../tools/env/install.pylintrc
help_screen.py
...
Your code has been rated at 3.65/10
pylint --include-ids=y --rcfile=../../tools/env/install.pylintrc
partition_edit_screen.py
...
Your code has been rated at 0.75/10
pylint --include-ids=y --rcfile=../../tools/env/install.pylintrc
network_nic_select.py
...
Your code has been rated at -0.53/10
Comment:
--------
Please add pydoc usable comments for each file.
e.g.:
% pydoc network_nic_configure.py
no Python documentation found for 'network_nic_configure.py'
Comment:
--------
This is a comment I made during the code walkthrough sessions. I think
the manipulation of the x & y coordinates should be done at a lower
level. Pehaps a text_install/CoursePosition class could be defined.
Maybe not worth it. But since it came up in the walkthrough sessions
I thought I would note it.
cmd/text-install/osol_install/text_install/date_time.py
-------------------------------------------------------------------------------
Comment:
--------
It seems you may be reinventing the wheel here. his seems like basic
functionality
to me.
Perhaps exploring what Python base classes already exhist that could handle
more of the functionality defined in date_time.
Comment:
--------
It also seems to me that the x & y courser manipulation seels like basic
functionality that might be already available somewhere. It's low level
functionality that seems to me should belong at a lower level.
Comment:
--------
I found these varible names not very indicative:
55 YYYY = _("(YYYY)")
YEAR maybe better
60 OFFSET = 3
OFFSET to what?
Comment:
--------
61 YEAR_DIGITS = 4
Comment:
--------
84 self.year_edit = None
85 self.year_err = None
86 self.year_list = None
_edit, _err, _list are repeated. Do you think it might be better to put
these in a
dictionary with keys: edit, err and list?
Comment:
--------
178 self.edit_area.columns = 3
Hardcoding things like this raises a flag for maintenance issues.
Would 60 OFFSET = 3 work here?
Comment:
--------
151 y_loc += 1
....
260 self.center_win.add_text("(0-59)", y_loc, x_loc,
self.win_size_x - 3)
The code between lines 151 and 260 looks repetitious. Would it make sense
to create parameterized supporting code which could be invoked multiple
times
to:
As needed set things like:
self.error_area.y_loc
self.list_area.y_loc
self.list_area.columns
self.edit_area.columns
self.edit_area.x_loc
invoke:
ErrorWindow()
ListItem()
EditField()
comment:
--------
326 def get_days_in_month(month_edit, year_edit, date_time):
Could calendar.py monthrange be leveraged here?
comment:
--------
345 if (len(cur_year) == 4 and year_valid(year_edit)):
Could calendar.py isleap be leveraged here?
cmd/text-install/osol_install/text_install/help_screen.py
-------------------------------------------------------------------------------
Comment:
--------
67 self.help_info = []
68 self.help_dict = None
Does it makes sense to set these empty and to None when:
73 self.setup_help_data()
will fill them in?
Comment:
--------
92 "NICSelect": ("network_manual.txt",
...
94 "NICConfigure": ("network_manual.txt",
Are both of these supposed to have the same helpfile: "network_manual.txt"?
Maybe yes eh?
Comment:
--------
96 "TimeZoneRegions": ("timezone.txt", _("Time Zone")),
97 "TimeZoneLocations": ("timezone.txt", _("Time Zone")),
98 "TimeZone": ("timezone.txt", _("Time Zone")),
Are all of these supposed to have the same header? I could see maybe the
same help file but should the headers be different?
Maybe no eh?
Comment:
--------
105 if self.is_x86:
130 if self.is_x86:
Could/Should these two conditionals be combined?
Comment:
--------
202 help_text = self.get_help_topic(info[0])
...
207 help_topic = info[0]
I see an inconsistency:
help_text is set using get_help_topic yet help_topic is not. Is this wrong
or is the named backwards?
cmd/text-install/osol_install/text_install/network_nic_configure.py
-------------------------------------------------------------------------------
Comment:
--------
101 find_defaults = self.nic.find_defaults
Why is it necessarey to have this in a try/except block? You control
self.nic.find_defaults.
This may be a Python thing I just don't understand yet.
Comment:
--------
157 self.center_win.activate_object(0)
What does the "0" represent? Could a defintion be created for index 0 so
an indicative name could be seen here?
Comment:
--------
168 list_item.data_obj = label
Could the ListItem constructor set this even though it is in inner_window?
Comment:
--------
170 validate = incremental_validate
Is an arguement needed to incremental_validate?
Comment:
--------
238 def incremental_validate(edit_field):
I find this name misleading. incremental_validate what?
Comment:
--------
222 def validate_ip(edit_field):
238 def incremental_validate(edit_field):
Wouldn't these be better defined in ../profile/ip_address.py ?
Comment:
--------
Support for IPv6?
cmd/text-install/osol_install/text_install/network_nic_select.py
-------------------------------------------------------------------------------
Looks good. No new comments.
cmd/text-install/osol_install/text_install/network_type.py
-------------------------------------------------------------------------------
Comment:
--------
149 if self.install_profile.no_install_mode:
I think it would be valuabel to add a comment describing what
no_install_mode is - manual vs no_instal
Comment:
--------
184 def validate(self):
Can you add a comment that this function overrides BaseScreen.validate?
This applies to all functions which override base class functions.
cmd/text-install/osol_install/text_install/partition_edit_screen.py
-------------------------------------------------------------------------------
Comment:
--------
81 self.slice_mode = slice_mode
If this is this x86 specific would the name self.slice_mode_x86 be helpul?
Comment:
--------
85 if self.slice_mode: # x86, Slice within a partition
...
90 elif self.is_x86: # x86, Partition on disk
...
94 else: # SPARC (Slice on disk)
Does this logic miss a possible error case where it's not slice_mode or
is_x86 or SPARC?
Suggestion:
if self.slice_mode: # x86, Slice within a partition
...
elif self.is_x86: # x86, Partition on disk
...
elif self.is_sparc: <- would need to add this
...
else:
handle error/exception
Comment:
--------
122 def _show(self):
and
214 def validate(self):
Can you add a comment that this function overrides BaseScreen's?
This applies to all functions which override base class functions.
Comment:
--------
139 logging.error('not skipping slice')
Is this a: "all is well" message? If so it could end up confusing the user.
Comment:
--------
143 except AttributeError:
Whey doesn't this raise or report an error? Please add a comment describing
this.
Comment:
--------
220 if self.is_x86 and not self.slice_mode:
Is a condition to handle SPARC needed here?
cmd/text-install/osol_install/text_install/timezone.py
-------------------------------------------------------------------------------
Comment:
--------
132 if len(item[2]) > 0:
...
135 elif len(item[1]) > 0:
Why not use this?
if item[2]:
...
elif item[1]:
...
cmd/text-install/osol_install/text_install/timezone_locations.py
-------------------------------------------------------------------------------
Comment:
--------
142 if len(item[2]) > 0:
...
145 elif len(item[1]) > 0:
Why not use this?
if item[2]:
...
elif item[1]:
...
cmd/text-install/osol_install/text_install/timezone_regions.py
-------------------------------------------------------------------------------
116 if len(item[2]) > 0:
...
119 elif len(item[1]) > 0:
Why not use this?
if item[2]:
...
elif item[1]:
...
Comment:
--------
No other new comments, except:
The code in timezone.py, timezone_locations.py and timezone_regions.py
seem to have a lot in common. Would developing a common base class, for
shared code, make sense?
