On 28/02/2012 13:43, Dermot McCluskey wrote: > Darren, > > Thanks for reviewing. Responses below. > > > On 02/28/12 11:54, Darren Kenny wrote: >> Hi Jack/Dermot. >> >> I focused on the GUI installer side (apart from the whole support.xml file) >> of things, here are my comments from this: >> >> gui_install_common.py: >> >> - Lines 90 and 259 - 273: >> >> NIT: This makes use of firefox directly, it would probably be more >> correct to use the 'gnome-open' application since this would allow for >> things to be more agnostic should anything change later? > > As per our discussion just now, it seems gnome-open > might not currently be as reliable, so I'll leave it as is.
Agreed, surprised to see it failed TBH, but it did... > >> - Generally, is there really any use-case where we wouldn't want to have a >> URI specified for opening? > > Possibly not, but it seemed like an easy and obvious option > to add. Would you prefer it be removed? No necessarily, just wondering was there any use-case envisioned that might need it... I think that it's ok to leave it there. > >> support_screen.py: >> >> - Line 60: >> >> Do you really expect the [email protected] e-mail address to be >> localized? Is it even visible to the user? > > I did ask this question and have not heard back, so > I went ahead and presumed it should be localized. > Yes - the user will see it - it is the default, pre-populated > value for the Email field. Also, it will not matter if the > actual value if different in different locales - all > non-authorized email address are treated similarly. OK, but just seems like something that wouldn't be localized. > >> - Lines 363 - 386: >> >> Is there not a possibility that any changes to aggregation_chosen or its >> sub-elements is missed if the user had originally configured a proxy? >> >> Just seems like some of the elifs should be extracted from this list to a >> separate: >> >> if not any_changes: >> if aggregation_chosen: >> ... > > It works correctly as is, but I will add a comment > to clarify that 'no_proxy_chosen', 'proxy_chosen' > and 'aggregation_chosen' are mutually exclusive - > only one of them can be True. (I found the coding > easier and clearer if I used 3 booleans rather than > one enumerate var for these fields.) OK, when you showed me the UI an how it worked here, the logic made sense. Adding the commend should at least make it clearer. > >> - Lines 559 - 589: >> >> Why do we limit to 5 characters? Just wondering, but would seem more >> correct to limit to at most a value of 65535 rather than 5 characters. > > Yes. Will change as suggested. Ok, thanks. > >> - Lines 581 - 584: >> >> I'm not sure that I like the idea of automatically jumping to the next >> field on entering the 5th character - could be quite confusing for >> people. > > This was copied from some previous code I wrote > for 4xIP fields, where it was decided to auto-advance > when a third digit is entered in each field. I agree it's > not intuitive here and will remove. In the IP case I can see it's use, but I'm glad you're removing it here since it would be potentially confusing for people. Thanks, Darren. > > Thanks, > - Dermot > > >> Thanks, >> >> Darren. >> >> On 28/02/2012 06:00, Jack Schwartz wrote: >>> Hi everyone. >>> >>> This is a webrev for the OCM/ASR configuration project. >>> >>> OCM and ASR are services which send limited system configuration and fault >>> information to Oracle, to offer better customer support. This project >>> provides installer and sysconfig support for configuring the OCM and ASR >>> services. >>> >>> There are two parts to this project: >>> text screens: >>> - usr/src/cmd/system-config/Makefile >>> - usr/src/cmd/system-config/__init__.py >>> - usr/src/cmd/system-config/helpfiles/Makefile >>> - usr/src/cmd/system-config/profile/Makefile >>> - usr/src/cmd/system-config/profile/__init__.py >>> - usr/src/cmd/system-config/profile/support_info.py >>> - usr/src/cmd/system-config/summary.py >>> - usr/src/cmd/system-config/support.py >>> - usr/src/cmd/text-install/summary.py >>> - usr/src/pkg/manifests/system-install-configuration.mf >>> >>> GUI screens (courtesy of Dermot): >>> - usr/src/cmd/gui-install/aux/INSTALL_SUPPORT_PANEL.txt >>> - usr/src/cmd/gui-install/aux/Makefile >>> - usr/src/cmd/gui-install/src/Makefile >>> - usr/src/cmd/gui-install/src/base_screen.py >>> - usr/src/cmd/gui-install/src/gui_install_common.py >>> - usr/src/cmd/gui-install/src/help_dialog.py >>> - usr/src/cmd/gui-install/src/screen_manager.py >>> - usr/src/cmd/gui-install/src/support_screen.py >>> - usr/src/cmd/gui-install/xml/Makefile >>> - usr/src/cmd/gui-install/xml/gui-install.xml >>> - usr/src/cmd/gui-install/xml/support.xml >>> - usr/src/pkg/manifests/system-install-gui-install.mf >>> >>> Please review. Dermot and I would like people familiar with sysconfig >>> and/or text installer to review the text screens, and people familiar with >>> GUIs to review the GUI screens. Each part is about 2000 lines, so please >>> pick a section to review. >>> >>> Dermot and I would like this round of review to be completed by next Monday >>> 3/5 COB if possible. >>> >>> Webrev:https://cr.opensolaris.org/action/browse/caiman/schwartz/7104766_1 >>> >>> Thanks, >>> Jack and Dermot >>> >>> >>> _______________________________________________ >>> caiman-discuss mailing list >>> [email protected] >>> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss >> _______________________________________________ >> caiman-discuss mailing list >> [email protected] >> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
