RFR: 7017818 NLS: JConsoleResources.java cannot be handled by translation team
Erik Gahlin
erik.gahlin at oracle.com
Thu May 24 12:40:53 PDT 2012
Mandy Chung skrev 2012-05-24 19:00:
> Thumbs up. Minor comment (no need for new webrev if you fix it):
>
Great! Thanks for taking the time doing the review, it was a lot of
files to go through,
> Resources.java L46: the AssertionError is not needed since it can't
> get there.
The constructor can be invoked from within the class, a corner case but
still.
> L133: would it be better to throw an internal error here to catch any
> regression e.g. if isWriteField is modified incorrectly for whatever
> reason?
>
It's probably better to throw an error but I never liked InternalError,
since it's supposed to signal an unexpected error in the Java Virtual
Machine. It's not like we are running out of resources. I will take
Error, unless you disagree.
> Messages.java - it'd be good to add a comment there to describe that
> the names are auto-generated and there are some names with double
> underscore or ends with '_' that will help anyone who has question if
> they are typos or some convention we should follow.
I will add a comment .
Thanks again
Erik
>
> Mandy
>
> On 5/24/2012 9:20 AM, Erik Gahlin wrote:
>> 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/57866004/attachment.html
More information about the serviceability-dev
mailing list