Hi Niall,
Thank you for the review. Please see my response inline.
On 07/21/11 08:21 AM, Niall Power wrote:
Hi Karen,
usr/src/cmd/system-config/profile/system_info.py:
----------------------------------------------------------------
217 def determine_terminal_type():
218 '''Determine default value for terminal type from type
219 of running environment.
220 Use following algorithm:
221 - If connected to SPARC via keyboard/monitor, use "sun"
222 - If connected to X86 via keyboard/monitor, use "sun-color"
223 - If connected to X86 and console devprop is not set, use
"sun-color"
224 - If running on serial console, use "vt100"
225 - Otherwise, use "vt100"'''
The description of the algorithm doesn't match that of the actual
algorithm,
which to me now looks more like:
if X86 keyboard & monitor:
"sun-color"
if SPARC keyboard & monitor:
"sun"
otherwise:
"vt100"
Your list above missed one case, which is that
if X86 and if we can't figure out whether we are
running from physical or serial console, we want
to set the term type to sun-color too.
I am not sure why the original code does that,
but I am trying to preserve exactly the same logic.
233 if platform.processor() == "i386":
234 if console_type != SERIAL_CONSOLE:
235 term_type = "sun-color"
236 else:
237 # SPARC
238 if console_type == PHYSICAL_CONSOLE:
239 term_type = "sun"
You're testing essentially the same thing but using inverted logic and
different values to do this, which
makes the code a bit harder to follow.
How about switching the i386 case at line 234 to:
234 if console_type == PHYSICAL_CONSOLE:
Because of the reason above, I can't just change it to your suggestion.
If I want to test for PHYSICAL_CONSOLE here, I would have to do:
if console_type == PHYSICAL_CONSOLE or console_type == UNKNOWN_CONSOLE:
term_type = "sun-color"
Since this makes the line very long, I decided to use the
inverse logic to save some typing. :-)
----------------------------------------------------------------
usr/src/cmd/text-install/__init__.py:
----------------------------------------------------------------
51 # Defined here to avoid circular import errors
52 #_ = gettext.translation("textinstall", "/usr/share/locale",
53 # fallback=True).ugettext
54 _ = gettext.translation("textinstall", "/tmp/locale",
302 #gettext.install("textinstall", "/usr/share/locale", unicode=True)
303 gettext.install("textinstall", "/tmp/locale", unicode=True)
"/tmp/locale" seems like an unusual place to be looking for message translation
files.
Is this the expected location? And why are the old values commented out instead
of removed?
Oops, /tmp/locale is what I tried to use for debugging. I will
uncomment the original
code and remove my debugging statements.
Otheriwse, looks good to me.
Thank you again for the review.
--Karen
Cheers,
Nial
On 07/21/11 02:42 PM, Karen Tung wrote:
Please review my changes for the following 2 bugs:
6985652 <http://monaco.us.oracle.com/detail.jsf?cr=6985652> changes in text
installer iso image for safe provision of localization without garbled display
7068464 <http://monaco.us.oracle.com/detail.jsf?cr=7068464>
determine_terminal_type() function in sysconfig.profile.system_info doesn't work in
some cases
webrev:
https://cr.opensolaris.org/action/browse/caiman/ktung/locale_bugs/webrev/
Takeshi from the localization team tested and verified the changes
work as expected.
The changes made to network_info.py is not related to these 2 bugs.
It's a couple
of pep8 errors that I saw and fixed.
Thanks,
--Karen
_______________________________________________
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
