RFR: 7017818 NLS: JConsoleResources.java cannot be handled by translation team

[Mandy Chung]

Thumbs up. Minor comment (no need for new webrev if you fix it):

Resources.java L46: the AssertionError is not needed since it can't get 
there.
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?

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.

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/672786d2/attachment-0001.html