On 02/ 4/10 06:00 PM, Susan Sohn wrote:
> The incremental webrev for the text installer project has been posted at:
> http://cr.opensolaris.org/~kemitche/text_v3_incremental/
>
> This webrev shows the diffs from the original code review with certain
> exceptions as described in the NOTE below.
>
> The full webrev to date is located at:
> http://cr.opensolaris.org/~kemitche/text_v3/
>
> We would like to get feedback on this round of changes no later than COB
> Tuesday, 2/9, if at all possible. If you would like to review these
> changes but
> can't be done with your comments by then please contact me and we'll work
> something out.
>
> Thanks,
> Sue
>
>
> NOTE:
> For the following files, an incremental webrev was not easily available,
> so all
> changes are shown:
> README
> cmd/Makefile
> cmd/Makefile.cmd
> cmd/Makefile.targ
> cmd/distro_const/auto_install/ai_sparc_image.xml
> cmd/distro_const/auto_install/ai_x86_image.xml
> cmd/distro_const/utils/Makefile
> cmd/distro_const/utils/grub_setup.py
> cmd/slim-install/finish/install-finish
> cmd/slim-install/svc/media-fs-root
> lib/Makefile
> lib/libtd/td_api.h
> lib/libtd/td_dd.c
> lib/libtd/test_td.c
> pkgdefs/SUNWdistro-const/prototype_com
>
> --------
> File Groupings:
>
> 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/text-install.py
> cmd/text-install/osol_install/text_install/ti_install.py
> cmd/text-install/osol_install/text_install/ti_install_utils.py
>
> Image/Distro-const:
> cmd/distro_const/auto_install/ai_generic_live.xml
> cmd/distro_const/auto_install/ai_sparc_image.xml
> cmd/distro_const/auto_install/ai_x86_image.xml
> cmd/distro_const/slim_cd/Makefile
> cmd/distro_const/slim_cd/all_lang_slim_cd_x86.xml
> cmd/distro_const/slim_cd/slim_cd_x86.xml
> cmd/distro_const/slim_cd/slimcd_generic_live.xml
> cmd/distro_const/text_install/Makefile
> 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/Makefile
> cmd/distro_const/utils/common_generic_live.xml
> cmd/distro_const/utils/gen_cd_content
> cmd/distro_const/slim_cd/slimcd_gen_cd_content
> cmd/distro_const/utils/grub_setup.py
> cmd/distro_const/utils/plat_setup.py
> cmd/distro_const/utils/post_boot_archive_pkg_image_mod_custom
> cmd/slim-install/config/set_lang
> cmd/slim-install/finish/install-finish
> cmd/slim-install/svc/media-fs-root
> cmd/text-install/svc/text-mode-menu.ksh
> cmd/text-install/text-mode-menu/text-mode-menu.ksh
> cmd/text-install/svc/text-mode-menu.xml
>
> Installation Profile:
> cmd/text-install/osol_install/profile/disk_info.py
> cmd/text-install/osol_install/profile/disk_space.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/partition_info.py
> cmd/text-install/osol_install/profile/disk_info.py
> cmd/text-install/osol_install/profile/slice_info.py
> cmd/text-install/osol_install/profile/disk_info.py
> cmd/text-install/osol_install/profile/system_info.py
> cmd/text-install/osol_install/profile/user_info.py
> cmd/text-install/osol_install/text_install/Makefile
>
> 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/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
>
> 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/inner_window.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
>
> Library Changes:
> lib/libict_pymod/ict.py
> lib/libtarget_pymod/Makefile
> 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/Makefile
> 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/libtransfer/transfer_mod.py
> lib/libzoneinfo_pymod/Makefile
> 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/text-install/Makefile
> cmd/text-install/osol_install/profile/Makefile
> cmd/text-install/svc/Makefile
> cmd/text-install/text-mode-menu/Makefile
> lib/Makefile
> lib/libict/Makefile
> pkgdefs/Makefile
> pkgdefs/SUNWdistro-const/prototype_com
> pkgdefs/SUNWinstall/prototype_com
> pkgdefs/SUNWtext-install/Makefile
> pkgdefs/SUNWtext-install/depend
> pkgdefs/SUNWtext-install/pkginfo.tmpl
> pkgdefs/SUNWtext-install/prototype_com
> pkgdefs/SUNWtext-install/prototype_i386
> pkgdefs/SUNWtext-install/prototype_sparc
> tools/env/install.pylintrc
>
> Other: (Help Files, miscellaneous)*
> cmd/text-install/helpfiles/Makefile
> 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_select.txt
> cmd/text-install/osol_install/__init__.py**
>
> * The help text files have been reviewed separately.
> ** 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.
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
I looked at the deltas to the screens files. I also compared the changes
to the comments I had made in code review round 1.
Most of this looks good to me. I only have a couple of small issues.
Joe
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
The list of files I looked at:
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
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/inner_window.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
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
cmd/text-install/osol_install/text_install/date_time.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
cmd/text-install/osol_install/text_install/disk_selection.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
cmd/text-install/osol_install/text_install/fdisk_partitions.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
cmd/text-install/osol_install/text_install/help_screen.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
cmd/text-install/osol_install/text_install/inner_window.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Comment:
--------
When invoking logging.log(5, ...
It might be clearer to assign "5" to a meaningfully named variable
e.g.:
440 logging.log(5, "InnerWindow.on_key_down\n" + str(type(self)))
What does lvl 5 result in?
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
cmd/text-install/osol_install/text_install/install_progress.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Comment:
--------
Could you please add a comment what the arguments represent?
55 self.status_loc = (4, 12, 50)
78 self.init_status_bar(6, 10, 50)
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
cmd/text-install/osol_install/text_install/install_status.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
cmd/text-install/osol_install/text_install/log_viewer.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
DID cmd/text-install/osol_install/text_install/network_nic_configure.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Comment:
--------
161 list_item.data_obj = label
Still the same comment as I had from the 1st code review:
Could the ListItem constructor set this even though it is in inner_window?
Comment:
--------
230 # pylint: disable-msg=C0103
Is is going to be accepted practice to disable pylint messages
or just allow a score less than 10>
Also you didn't enable-msg=C0103
Sure ... this is at the end of the file but perhaps disable-msg=C0103,
for completeness...
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
DID cmd/text-install/osol_install/text_install/network_nic_select.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK. No new comments.
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
DID cmd/text-install/osol_install/text_install/network_type.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Comment:
--------
From the 1st round code review I had 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.
I thought we talked about this and you were going to add the commnet.
Did I remember this incorrectly?
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
DID cmd/text-install/osol_install/text_install/partition_edit_screen.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Comment:
--------
From the 1st round code review I had 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.
I thought we talked about this and you were going to add the commnet.
Did I remember this incorrectly?
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
cmd/text-install/osol_install/text_install/summary.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
DID cmd/text-install/osol_install/text_install/timezone.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
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
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Question/Comment:
-----------------
278 '''Ensure the username is not "root" or "jack"'''
Why is user jack not allowed?
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
cmd/text-install/osol_install/text_install/welcome.py
+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=+-=
Looks OK
