RFR: 7017818 NLS: JConsoleResources.java cannot be handled by translation team
Erik Gahlin
erik.gahlin at oracle.com
Thu May 24 09:20:05 PDT 2012
Here is an updated webrev:
http://cr.openjdk.java.net/~egahlin/7017818_6/
Changes from previous webrev:
- Removed all @SuppressWarning changes
- Moved the Messages class from sun.tools.jconsole.resource to
sun.tools.jconsole and updated the makefile
- Aligned parameters in the method calls.
- Changed CONNECTION_LOST back to CONNECTION_LOST1 and kept message keys
as is since they should work with the property files sent away.
Erik
Mandy Chung skrev 2012-05-22 23:49:
> Erik,
>
> I approve what you have - please go through and fix the formatting
> nits. I inlined my comment below and you can follow up them later if
> needed.
>
> On 5/22/2012 12:40 PM, Erik Gahlin wrote:
>> Thanks for reviewing,
>>
>> I got a lot of unused import warnings from the IDE when I changed the
>> Resources class. When I cleaned them up I thought I might as well
>> clean up the
>> others too, so I could more easily see that I had taken care of all the
>> warnings I caused. Same thing with unused member variables and unused
>> methods.
>>
>> Import-statements will not change how the program executes, so I
>> thought it
>> was safe to remove them (no risk of introducing new bugs).
>>
>> ...
>>> Anyway,
>>> the warning cleanup deserves a separate CR and review.
>> Breaking it up into seperate patches and CRs will take time. Is it really
>> necessary?
>
> I'm okay for this patch to include the removal of unused imports and
> dead code fixes. The warnings I referred to are the
> @SuppressedWarnings("serial") and rawtypes changes since it may
> require a separate pair of eyes to review them. As a general advice,
> when the number of warnings cleaned up is not small, it's always
> recommended to separate them as two separate CRs for bug management
> and backport and it also helps the reviewers :)
>
> Since you have made the change along with the fix for this CR, I can
> understand why you said it will take time. I also understand that
> you're under a time pressure. I can go with what you have but please
> consider in the future when to separate the change in a separate CR.
>
>
>>
>> I cleaned up the code so I could make the fix more easily. I don't
>> think it's
>> worth the hazzle to create a separate bug and patch. I might as well
>> revert
>> the clean ups.
>>
>
> BTW, backporting to an update release might request not to include the
> warnings cleanup. I'm not sure the putback approval requirements for 7u6.
>
>>>
>>> Below are comments on the change to support translateability.
>>>
>>> sun/tools/jconsole/resources/Messages.java
>>> I suggest to move Messages to sun.tools.jconsole since
>>> it's a utility class and conventionally resources are put
>>> in a "resources" subpackage (i.e. sun.tools.jconsole.resources
>>> in this case).
>> I think one package private Message class for each package and separate
>> resource files for each package would be the best, but I didn't want
>> this CR to blow up, so I put the Message class where the other Java
>> classes
>> related to resources were before.
>>
>> I can move it to sun.tools.jconsole, if you think that's better.
>>
>
> I think so so that sun.tools.jconsole.resources.* are resource files.
>
>>>
>>> The initializeMessage method uses the field name as the
>>> key and initializes its value to its localized message via
>>> reflection. Such approach seems strange.
>> I like to enforce one-to-one mapping between the keys in the property
>> file and
>> the keys used in the Java code. When going through the fields in the
>> Message
>> class, using reflection, I can ensure that all fields have a
>> corresponding
>> property value in the file, and vice versa.
>>
>> With this approrach it's not necessary click through all the GUI to
>> verify that
>> all keys exists in the property file. It's also possible to detect if
>> a value
>> in a property file is no longer in use.
>>
>> The code that does the one-to-one check was removed, but it should
>> probably be
>> added back so similar problems can be catched automatically in the
>> future.
>>
>>>
>>> Have you considered about defining the constants with
>>> the key as the value (i.e. the variable name and its
>>> value are the same).
>> The Java constants can't be the same as the property value
>
> I meant the key value in messages.properties (not the property value).
> Essentially variable and the value is the same e.g. static final
> String ONE_DAY = "ONE_DAY";
>
>>> Instead of initializing each
>>> static field of the Messages class, you can build
>>> a map of a key to the localized message + itsmnemonic
>>> key (like what you have done in building the MNEMONIC_LOOKUP
>>> map - why not change such hash map to map from a string to
>>> an object {message+mnemonic}). In that case, the MNEMONIC_LOOKUP
>>> doesn't need to be a synchronized map and could be done
>>> as the class initialization of Resources class.
>>>
>>> It would only need to keep
>>> Resources.getText(String) method that returns the localized
>>> message, e.g.
>>> Resources.getText(Messages.HELP_ABOUT_DIALOG_TITLE)
>>>
>>> I just don't see it's worth the complexity to initialize
>>> the static fields via reflection to get rid of a convenience
>>> method.
>>
>> The synchronization is not really needed, if you always use the keys to
>> lookup the messages. The static initializer in the Message class should
>> ensure correct ordering.
>>
>> Looking up messages "dynamically" means you have to trigger all the
>> code in the GUI that needs a translated message to be sure you got
>> things
>> correct. Since there are several hundred messages I think the
>> static-fields-reflection approach is better.
>>
>>>
>>>
>>> It is only my suggestion and I understand that this fix needs
>>> to be backport to 7u6. If you agree that replacing this
>>> static field initialization logic with a separate map,
>>> I'm okay with pushing this approach to 7u6 and push
>>> a better fix to jdk8. Or I miss the benefit you were
>>> considering :)
>>
>> You are missing it :)
>>
>> .. and it's probably because I removed the code that did the actual
>> check :)
>>
>
> I agree that checking one-to-one mapping between the keys in the
> property file and
> the keys used in the Java code is good. What you need is to compare
> the list of constants with the keys in messages.properties. Anyway,
> my suggestion was just to simplify such initialization. You can go
> with what you have.
>
>>>
>>> There are a few names with '_' suffix e.g. L93, 97, 104, 160
>>> and also some names with '__' (L97, 159). Do you want to
>>> embed the space of the message in the key name? In any case,
>>> the key names with '_' suffix or double underscores '__' is
>>> a little confusing. It would be better just to use '_' for
>>> separating words of a key name and no need for '_' suffix.
>>> The names 'CHART_COLON', 'ERROR_COLON_MBEANS...', 'JCONSOLE_COLON',
>>> and the ones with 'COLON' to describe its message with ":"
>>> are strange. If ":" was removed from the message in the
>>> future, the name would need to be modified to follow this
>>> naming convention which is overkill.
>> The keys were generated programmatically.
>>
>
> Ah - that's what I guess.
>
>> The 'COLON' was needed so I could differentiate between message that
>> looked
>> the same, except for the ':' at the end. The pattern was applied to all
>> messages. Some of those message were removed (since they were no
>> longer in use)
>>
>> I can remove the 'COLON' suffix where it's not needed, same thing
>> with spaces.
>>
>
> That'd be good since "COLON" and '_' is meaningless w.r.t. the key
> name. IDE refactoring feature should make this renaming effortless :)
>
>> I will go over the message keys and clean up the names, but they were
>> not that
>> clean before either :)
>
> Thanks.
> Mandy
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20120524/dafcf13c/attachment-0001.html
More information about the serviceability-dev
mailing list