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

Pavel Porvatov pavel.porvatov at oracle.com
Wed Apr 4 11:21:57 UTC 2012 

Hi Alexander,

Looks great, I approve that! Could you please attach your test to CR 
7093156? And one minor notice: you should update javadoc of the 
TextAndMnemonicHashMap class. 
"FileChooser.saveButtonTextAndMnemonic=&Save" should be 
"FileChooser.saveButton.textAndMnemonic=&Save"

Regards, Pavel
>
> webrev: http://cr.openjdk.java.net/~alexsch/7093156/webrev.03/
> test: http://cr.openjdk.java.net/~alexsch/7093156/test/02/
>
> Now all properties are generated with the following suffixes: 
> xxx.textAndMnemonic and xxx.titleAndMnemonic
> - patterns which are generated to properties with the 
> xxx.textAndMnemonic suffix:
>   (xxxNameText, xxxNameMnemonic)
>   (xxxNameText, xxxMnemonic)
>   (xxx.nameText, xxx.mnemonic)
>   (xxxText, xxxMnemonic)
>
> - pattern which is generated to properties with the 
> xxx.titleAndMnemonic suffix:
>    (xxxTitle, xxxMnemonic)
>
> After that the extended hashmap check a key suffix and generate a 
> compositeKey which allows to get a property in a new format and 
> extract a text, mnemonic or mnemonic index from it.
>
>  The test gets a path to the old properties and L&F class. If L&F is 
> not null than it is set. After that the test gets all properties and 
> it's values and check that values are equal to the values which are 
> got from the  UIManager.get(key, locale).
>
>  There are some exceptions which are included into the exclude list in 
> the end of the test.
>  For example a mnemonic can be set to zero or to a character instead 
> of using the character integer key value.
>  Or there can be defined just mnemonics without a text.
>
> Thanks,
> Alexandr.
>
> On 3/30/2012 12:56 PM, Alexander Scherbatiy wrote:
>>
>> webrev: http://cr.openjdk.java.net/~alexsch/7093156/webrev.01/
>>
>> There is the updated webrev  which is used the TextAndMnemonicPattern 
>> class and separate getTextProperty and getMnemonicProperty methods. 
>> The getMnemonicFromProperty returns null in case if a mnemonic has 
>> not been found.
>>
>>  See other comments inline...
>>
>> On 3/29/2012 5:28 PM, Pavel Porvatov wrote:
>>> 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.
>>
>>   I have added the TextAndMnemonicPattern class and updated the code 
>> to use the following patterns:
>>   ( texts: { "NameText", "Text", "Title"} , mnemonic: "Mnemonic")
>>   ( texts: { "nameText" } , mnemonic: "mnemonic")
>>
>>   Now the "for each" loop is used and for each mnemonic there is only 
>> one iteration of the text.
>>
>>>
>>> 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...
>>
>>   1)   There are real collisions if we will use the same suffix like 
>> xxx.textAndMnemonic for all properties. For example see the 
>> gtk.properties:
>>    FileChooser.renameFileErrorTitle=Error
>>    FileChooser.renameFileErrorText=Error renaming file "{0}" to "{1}"
>>
>>   Changing it to the same suffix will lead to:
>>      FileChooser.renameFileError.textAndMnemonic=Error
>>      FileChooser.renameFileError.textAndMnemonic=Error renaming file 
>> "{0}" to "{1}"
>>
>>    So one value will be lost.
>>
>>   2)  We really need to check all these text suffixes.
>>
>>     There are 3 examples from the swing property files:
>>      ColorChooser.hsvNameText=HSV
>>      ColorChooser.hsvMnemonic=72
>>      FileChooser.helpButtonText=Help
>>      FileChooser.helpButtonMnemonic=72
>>      GTKColorChooserPanel.nameText=GTK Color Chooser
>>      GTKColorChooserPanel.mnemonic=71
>>
>>     which are converted to the new format:
>>     FileChooser.helpButtonTextAndMnemonic=&Help
>>     ColorChooser.hsvNameTextAndMnemonic=&HSV
>>     GTKColorChooserPanel.nameTextAndMnemonic=&GTK Color Chooser
>>
>>      Now on the left side there are requests and on the right side 
>> are our actions:
>>         ColorChooser.hsvNameText  |  check that it is request to the 
>> text (has NameText suffix) and convert to new format 
>> ColorChooser.hsvNameTextAndMnemonic (add AndMnemonic  suffix)
>>         GTKColorChooserPanel.nameText | check that it is request to 
>> the text (has nameText) and convert to new format 
>> GTKColorChooserPanel.nameTextAndMnemonicadd AndMnemonic  suffix)
>>         FileChooser.helpButtonMnemonic | check that it is a request 
>> to the mnemonic (has Mnemonic  suffix) and convert to new format 
>> (remove Mnemonic suffix and add TextAndMnemonic suffix)
>>         GTKColorChooserPanel.mnemonic | check that it is a request to 
>> the mnemonic (has mnemonic  suffix) and convert to new format (remove 
>> mnemonic suffix and add nameTextAndMnemonic suffix)
>>
>>     The suffixes for the text can be Text and Title as well.
>>     So the "NameText", "Text", "Title" and "nameText" suffixes are 
>> already used in swing components and their unification requires to 
>> updating a lot of java code.
>>>
>>>>
>>>>>
>>>>> 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
>>
>>     Updated the method to return null if a mnemonic has not been found.
>>
>>    Thanks,
>>   Alexandr.
>>
>>>>>
>>>>> 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.
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>

More information about the swing-dev mailing list