Hi Dermot,
Thanks for the re-review. Responses inline.
An updated webrev will be sent out after addressing yours and others'
comments.
- Keith
On 02/ 5/10 10:06 AM, Dermot McCluskey wrote:
> Keith, Sue,
>
> I have re-reviewed the files in the "UI Components" section.
> I have confirmed that all the issues raised previously have
> been addressed satisfactorily.
>
> I have only a few minor further comments to make:
>
>
> cmd/text-install/osol_install/text_install/error_window.py:
> 76 logging.debug("displaying err '%s'" % text)
> You don't need to interpolate substitution variables before
> calling logging.debug, you can pass them in as params, eg:
> 76 logging.debug("displaying err '%s'", text)
> In fact, pylint usually gives a warning if you use the former,
> so presumably the latter is the preferred way.
> This also applies to several logging.debug calls in
> inner_window.py and scroll_window.py.
You're correct. And I can even explain why - it's an optimization thing.
String formatting operations are slow. By passing the string formatting
arguments to the logger, the logger can skip the formatting operation if
the logging level is too high. e.g., if the logger's level is set to
"warn", then logging.debug(...) statements won't be formatted - however,
if the string formatting is done before the call to logging.debug(...),
then it's done every time, regardless of logger level.
This is mostly important in functions that are run frequently in
high-performance environments, but it's good practice to follow everywhere.
>
>
> cmd/text-install/osol_install/text_install/main_window.py:
> 109 # central_border_area = WindowArea(win_size_y - 4,
> win_size_x - 2, 2, 1)
> 110 # self.central_border = InnerWindow(central_border_area,
> 111 # color_theme=self.theme)
> 112 # central_win_area = WindowArea(win_size_y - 4, win_size_x
> - 6, 2, 3)
> You've added some commented-out code. It should be used
> or deleted.
I've removed those lines; thanks for pointing them out.
>
>
> cmd/text-install/osol_install/text_install/scroll_window.py:
> 99 self.window.vline(0, 0, ord(" "),
> self.area.scrollable_lines)
> Can you use InnerWindow.BKGD_CHAR instead of ord(" ")?
Yes I can and will.
>
>
>
> - Dermot
>
>
>
>
> On 02/04/10 23:00, 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
> _______________________________________________
> caiman-discuss mailing list
> caiman-discuss at opensolaris.org
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
