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
