<Swing Dev> Review request for 7093156: NLS: Please change the mnemonic assignment system to avoid translation issue (Swing files)

[Pavel Porvatov]

Hi Alexander,
> On 3/29/2012 1:19 PM, Pavel Porvatov wrote:
>> Hi Alexander,
>>
>> I have several questions about the patch.
>>
>> File TextAndMnemonicHashMap:
>> 1. MNEMONIC_SUFFIX[] = {"Mnemonic", "mnemonic", "Mnemonic", "Mnemonic",}
>> what's the reason have three "Mnemonic" values?
>>
>> 2. TEXT_SUFFIX[] = {"NameText", "nameText", "Text", "Title"}
>> What is the reason to have "NameText", "nameText". It looks there is 
>> no need in that because that's a particular case of "Text"
>
>      The reason to have two arrays is because there are several 
> patterns that are used for the text and mnemonic declaration.
>      They are (xxxNameText, xxxMnemonic), (xxx.nameText, 
> xxx.mnemonic),(xxxText, xxxMnemonic) and (xxxTitle, xxxMnemonic).
I'll try explain what I meant below...

Lets take the first loop:
+                for (int i = 0; i < TEXT_SUFFIX.length; i++) {
+                    String suffix = TEXT_SUFFIX[i];
+                    if (stringKey.endsWith(suffix)) {
+                        value = super.get(stringKey + AND_MNEMONIC);
+                        if (value != null) {
+                            return getText(value.toString());
+                        }
+                    }
+                }
and lets stringKey = BlaBlaBlaNameText. Then you'll invoke super.get() 
twice and the loop will have 4 iterations. So if you will use for the 
first loop only "Text", "Title" you will have only 2 loop iterations and 
no unnecessary super.get() invocations.

About "They are (xxxNameText, xxxMnemonic), (xxx.nameText, 
xxx.mnemonic),(xxxText, xxxMnemonic) and (xxxTitle, xxxMnemonic)": can 
we don't use "Name" for pair (xxxNameText, xxxMnemonic), like 
xxxTextAndMnemonic? For consistency we could  use for pair 
(xxx.nameText, xxx.mnemonic) xxx.textAndMnemonic...

>
>>
>> 3. Could you pleas use "for each" loops (if possible), they are more 
>> compact and readable
>
>     In the second loop the i index is used to add a TEXT_SUFFIX after 
> cutting a MNEMONIC_SUFFIX.
>     So the first loop can use the "for each" loop.
Yep! That's what I meant...
>
>    May be it is better to have a TextAndMnemonic class that holds 
> values for the corresponded text and mnemonic suffixes?
>
>>
>> 4. Integer.toString((int) Character.toUpperCase(c)) looks a little 
>> bit tricky. Can we use something like this:
>> String.valueOf(Character.toUpperCase(c)) ?
>      For example  there is the property:
>        ColorChooser.rgbRedTextAndMnemonic=Re&d
>     The code that needs a mnemonic expect that it gets the integer 
> value |68| in the string instead of  char D.
>     So the right mnemonic value should be
>        ColorChooser.rgbRedMnemonic=68
>
Ok. BTW: I think "return null" will be more correct in the getMnemonic 
method
>>
>> 5. What is the reason to have the second loop? 
>      The first loop checks is it a request to a text value. The second 
> loop checks is it a request to a mnemonic value.
>
>> Can we just check
>> stringKey.endsWith("mnemonic") || stringKey.endsWith("Mnemonic")?
>      There might be collisions. It is better to explicitly check that 
> a text suffix from a pattern have the certain suffix from the mnemonic 
> pattern.

Regards, Pavel
>
>    Thanks,
>    Alexandr.
>
>>
>> Regards, Pavel
>>
>>> bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7093156
>>> webrev: http://cr.openjdk.java.net/~alexsch/7093156/webrev.00/
>>>
>>> The properties in the swing resources files are changed from the
>>>   xxxText=ABC
>>>   xxxMnemonic=B
>>> to
>>>   xxxTextAndMnemonic=A&BC
>>>
>>> where Text suffix can be one of the following: "NameText", 
>>> "nameText", "Text" and  "Title"
>>>
>>>
>>> The hashmap in UIDefaults class is extended to return the correct 
>>> values for the keys xxxText and xxxMnemonic from the stored 
>>> xxxTextAndMnemonic string.
>>>
>>> There is no html text in the swing resource files so this case is 
>>> not considered.
>>> The fix covers only the swing resource files in the JDK:
>>>   src/share/classes/com/sun/swing/internal/plaf
>>>   src/share/classes/com/sun/java/swing/plaf
>>>
>>> The demo resources will be fixed in the separated issue.
>>>
>>> Thanks,
>>> Alexandr.
>>>
>>
>