<i18n dev> RFR: 7017818 NLS: JConsoleResources.java cannot be handled by translation team

Mandy Chung mandy.chung at oracle.com
Mon May 21 14:13:14 PDT 2012


I like your idea of trying to define the string constants
for the keys to benefit from the compiler checking and
catch any issue due to typos at compile time.  My review
comments below.

Your webrev includes other cleanup.  While it's good to
do the simple minor cleanup with this fix, I'd like to
the warnings cleanup changes be separated from this patch
and done by a separate CR.

>    * added generic types where needed.
>    * added suppress serialization warning where it was needed.
>    * removed suppress serialization annotation where it was not needed.

Specifically, I'm concerned with @SuppressWarning("serial").
Tab.java L31 - it should not be suppressed and instead be
fixed by defining the serialVersionUID (you can find the
value by running serialver tool). Same to XOperations.java and
XTable.java.  I notice that the current code does suppress
"serial" warning that you should check if it should be fixed
as well.

As for the generics, ProxyClient.java and XTree.java (and
maybe some other places). When I saw the use of Iterator,
I typically would also suggest to replacethe Iterator and
while loop with foreach (L601-617 and L710, 736).  Anyway,
the warning cleanup deserves a separate CR and review.

Below are comments on the change to support translateability.

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

   The initializeMessage  method uses the field name as the
   key and initializes its value to its localized message via
   reflection. Such approach seems strange.

   Have you considered about defining the constants with
   the key as the value (i.e. the variable name and its
   value are the same).  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.

   I just don't see it's worth the complexity to initialize
   the static fields via reflection to get rid of a convenience

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

   L52-54 and also other @param, nit:generally we put
   the description in the same line as the @param.

   L255: this doesn't look right - I think it should be
   Comparable<CompositeData>.  However, this method has
   @SuppressWarning("unchecked") and why was this line
   changed?  Did you get other warning?

Formatting nits: it's good to go through your changes and
fix the formatting nits.  The lines need to be updated to
aligned, a few examples are:
   BorderedComponent.java L152
   CreateMBeanDialog.java L77-78, 165-167, 169-171
   MemoryTab.java L120-121, 126-127
   OverviewTab.java L59-60
   Plotter.java L319-321, 329-331

I didn't include all and please check the files not mentioned above.

On 5/21/2012 4:18 AM, Erik Gahlin wrote:
> Thanks Michael,
> When it comes to ALL CAPs or SturdlyCaps, it's okay if those strings 
> are not translated.
> Here is an updated webrev. I removed strings that were no longer in 
> use and made some changes to the make-files.
> http://cr.openjdk.java.net/~egahlin/7017818_4/
> /E
> Michael Fang skrev 2012-05-11 18:16:
>> Thanks so much Erik for the fix! The messages.properties file look 
>> good. I only reviewed the English *messages.properties* file. WPTG 
>> will re-translate the ja and zh_CN files. They will attempt to 
>> leverage existing translation from *JConsoleResources_xx.java*.
>> I have a comments about another translatability rule that WPTG follows:
>> By Oracle software development guideline, ALL CAPs or StudlyCaps 
>> strings will not be translated. Examples are:
>>   149 MBEAN_INFO=MBeanInfo
>>   150 MBEAN_NOTIFICATION_INFO=MBeanNotificationInfo
>>   151 MBEAN_OPERATION_INFO=MBeanOperationInfo
>>   206 OBJECT_NAME=ObjectName
>>   241 SEQ_NUM=SeqNum
>> If any of the above such as ACTION or UNKNOWN needs to be translated, 
>> they should not be ALL CAPS. The translators will not translate those 
>> lines by default.
>> The translators have the ability to see comments (or special 
>> requests) if a comment line is inserted immediately prior to any of 
>> the resources (each resource string and one comment line immediately 
>> before it is stored in translation memory). If you must leave ACTION 
>> or UNKNOWN in CAPS, you can try to insert comments.However, WPTG does 
>> not guarantee they will be followed.
>> thanks,
>> -michael
>> On 12?05?11? 07:36 ??, Erik Gahlin wrote:
>>> Could you please review? I also need a sponsor.
>>> http://cr.openjdk.java.net/~egahlin/7156518/1_0/
>>> http://monaco.us.oracle.com/detail.jsf?cr=7017818
>>> The patch is for JDK8, but it needs to ported to 7u6 before 5/16.
>>> Thanks!
>>> Erik
>>> Changes:
>>> - Moved localization messages to property files, one message per 
>>> line, as needed.
>>> - Added '&' to messages so mnemonics could be identified.
>>> - Introduced Message class with static fields corresponding to the 
>>> keys in the property files.
>>> - Added map for looking up mnemonics.
>>> Testing:
>>> - Verified programmatically that all the messages and mnemonics are 
>>> "compatible" with the previous mechanism, for Chinese, Japanese and 
>>> the default locale.
>>> - The intention is to run through the GUI to confirm that everything 
>>> looks ok, but I'm waiting for a build.
>>> Other:
>>> - Fixed a typo in the MemoryPoolStat class, the method 
>>> getAfterGcUsage returned this.beforeGCUsage instead of 
>>> this.afterGcUsage
>>> - When going through all the code I did some minor clean up that 
>>> should not impact the program flow:
>>>   * removed unused imports.
>>>   * inlined temporary variables holding messages.
>>>   * removed private member variables that were not accessed.
>>>   * removed private methods that were not referenced.
>>>   * removed local variables that were not used.
>>>   * added generic types where needed.
>>>   * static methods are now called statically.
>>>   * added suppress serialization warning where it was needed.
>>>   * removed suppress serialization annotation where it was not needed.
>>> - In the Message class, the comment "remove? not found in code" will 
>>> be removed once I know those message are not needed for 7u6.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/i18n-dev/attachments/20120521/d712a0df/attachment.html 

More information about the i18n-dev mailing list