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