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

[Alexander Scherbatiy]

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.
>>>>
>>>
>>
>