RFR: 7017818 NLS: JConsoleResources.java cannot be handled by translation team
Erik Gahlin
erik.gahlin at oracle.com
Tue May 22 12:40:08 PDT 2012
Mandy Chung skrev 2012-05-21 23:13:
> Erik,
>
> 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.
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).
>
>> * 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.
I agree, I was trying to get rid of all the warnings so I could see the
real
problems more easily. Adding serialVersionUID might be beyond the scope of
the CR. I can bring back the warnings, if you like, now that I know my
code is
free of warnings.
>
> 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).
I agree, I was trying to get rid of all the warnings so I could see the
real
problems. Foreach is prefered, but I think it's beyond the scope of the CR
to replace all those.
Also, adding generics doesn't change the byte code and should be safe.
Rewritings loops could potentially introduce other bugs and to me there is
little benefit.
> 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 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.
>
> 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.
>
> 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 since the
character
you can use for a variable name is limited, for instance a constant can't
contain the ':'-character.
> 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 :)
>
> 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.
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.
I will go over the message keys and clean up the names, but they were
not that
clean before either :)
>
> sun/tools/jconsole/Resources.java
> L52-54 and also other @param, nit:generally we put
> the description in the same line as the @param.
Will fix!
> XOpenTypeViewer.java
> 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?
>
CompositeData#get returns Object and because of type erasure it can't be
guaranteed to be Comparable<CompositeData> without an explicit cast, which
I didn't dare to add, so I put in Object.
Yes, I got a rawtypes warning on the Comparable.
> 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.
>
Will do!
> 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:
>>> 19 ACTION_CAPITALIZED=ACTION
>>> 20 ACTION_INFO_CAPITALIZED=ACTION_INFO
>>> 149 MBEAN_INFO=MBeanInfo
>>> 150 MBEAN_NOTIFICATION_INFO=MBeanNotificationInfo
>>> 151 MBEAN_OPERATION_INFO=MBeanOperationInfo
>>> 206 OBJECT_NAME=ObjectName
>>> 231 R_FORWARD_SLASH_W_CAPITALIZED=R/W
>>>
>>> 241 SEQ_NUM=SeqNum
>>> 249 SUMMARY_TAB_HEADER_DATE_TIME_FORMAT=FULL,FULL
>>>
>>> 273 UNKNOWN_CAPITALIZED=UNKNOWN
>>>
>>> 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/serviceability-dev/attachments/20120522/1a50514c/attachment-0001.html
More information about the serviceability-dev
mailing list