<html>
  <head>
    <meta content="text/html; charset=ISO-8859-1"
      http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    Erik,<br>
    <br>
    I approve what you have - please go through and fix the formatting
    nits.&nbsp; I inlined my comment below and you can follow up them later
    if needed.<br>
    <br>
    On 5/22/2012 12:40 PM, Erik Gahlin wrote:
    <blockquote cite="mid:4FBBEB98.50704@oracle.com" type="cite">
      <meta content="text/html; charset=ISO-8859-1"
        http-equiv="Content-Type">
      <span class="new">
      </span>
      <blockquote cite="mid:4FBAAFEA.6090305@oracle.com" type="cite"> </blockquote>
      Thanks for reviewing,<br>
      <br>
      I got a lot of unused import warnings from the IDE when I changed
      the<br>
      Resources class. When I cleaned them up I thought I might as well
      clean up the<br>
      others too, so I could more easily see that I had taken care of
      all the <br>
      warnings I caused. Same thing with unused member variables and
      unused methods.<br>
      <br>
      Import-statements will not change how the program executes, so I
      thought it <br>
      was safe to remove them (no risk of introducing new bugs).<br>
      <br>
    </blockquote>
    <blockquote cite="mid:4FBBEB98.50704@oracle.com" type="cite"> ...<br>
      <blockquote cite="mid:4FBAAFEA.6090305@oracle.com" type="cite">
        <pre><span class="new"> Anyway,
the warning cleanup deserves a separate CR and review.
</span></pre>
      </blockquote>
      Breaking it up into seperate patches and CRs will take time. Is it
      really<br>
      necessary?<br>
    </blockquote>
    <br>
    I'm okay for this patch to include the removal of unused imports and
    dead code fixes.&nbsp; The warnings I referred to are the
    @SuppressedWarnings("serial") and rawtypes changes since it may
    require a separate pair of eyes to review them. &nbsp; 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 :)<br>
    <br>
    Since you have made the change along with the fix for this CR, I can
    understand why you said it will take time.&nbsp; I also understand that
    you're under a time pressure.&nbsp; I can go with what you have but
    please consider in the future when to separate the change in a
    separate CR.<br>
    <br>
    <br>
    <blockquote cite="mid:4FBBEB98.50704@oracle.com" type="cite"> <br>
      I cleaned up the code so I could make the fix more easily. I don't
      think it's<br>
      worth the hazzle to create a separate bug and patch. I might as
      well revert <br>
      the clean ups.<br>
      <br>
    </blockquote>
    <br>
    BTW, backporting to an update release might request not to include
    the warnings cleanup.&nbsp; I'm not sure the putback approval
    requirements for 7u6.<br>
    <br>
    <blockquote cite="mid:4FBBEB98.50704@oracle.com" type="cite">
      <blockquote cite="mid:4FBAAFEA.6090305@oracle.com" type="cite">
        <pre><span class="new">
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).
</span></pre>
      </blockquote>
      I think one package private Message class for each package and
      separate <br>
      resource files for each package would be the best, but I didn't
      want <br>
      this CR to blow up, so I put the Message class where the other
      Java classes<br>
      related to resources were before.<br>
      <br>
      I can move it to sun.tools.jconsole, if you think that's better. <br>
      <br>
    </blockquote>
    <br>
    I think so so that sun.tools.jconsole.resources.* are resource
    files.<br>
    <br>
    <blockquote cite="mid:4FBBEB98.50704@oracle.com" type="cite">
      <blockquote cite="mid:4FBAAFEA.6090305@oracle.com" type="cite">
        <pre><span class="new">
</span><span class="new">  The initializeMessage</span> method uses the field name as the
  key and initializes its value to its localized message via
  reflection. Such approach seems strange.</pre>
      </blockquote>
      I like to enforce one-to-one mapping between the keys in the
      property file and<br>
      the keys used in the Java code. When going through the fields in
      the Message <br>
      class, using reflection, I can ensure that all fields have a
      corresponding<br>
      property value in the file, and vice versa.<br>
      <br>
      With this approrach it's not necessary click through all the GUI
      to verify that<br>
      all keys exists in the property file. It's also possible to detect
      if a value <br>
      in a property file is no longer in use.<br>
      <br>
      The code that does the one-to-one check was removed, but it should
      probably be <br>
      added back so similar problems can be catched automatically in the
      future.<br>
      <br>
      <blockquote cite="mid:4FBAAFEA.6090305@oracle.com" type="cite">
        <pre> 
<span class="new">  Have you considered about defining the constants with
  the key as the value (i.e. the variable name and its
  value are the same). </span></pre>
      </blockquote>
      The Java constants can't be the same as the property value</blockquote>
    <br>
    I meant the key value in messages.properties (not the property
    value). Essentially variable and the value is the same e.g.&nbsp; static
    final String ONE_DAY = "ONE_DAY";<br>
    <br>
    <blockquote cite="mid:4FBBEB98.50704@oracle.com" type="cite">
      <blockquote cite="mid:4FBAAFEA.6090305@oracle.com" type="cite">
        <pre><span class="new"> Instead of initializing each
  static field of the Messages class, you can build 
  a map of a key to the localized message + its </span><span class="new">mnemonic </span>
<span class="new">  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
 &nbsp;Resources.getText(String) method that returns the localized
 &nbsp;message, e.g.
      Resources.getText(</span><span class="changed">Messages.HELP_ABOUT_DIALOG_TITLE</span>)
<span class="new"></span><span class="new">
  I just don't see it's worth the complexity to initialize
  the static fields via reflection to get rid of a convenience
  method. </span></pre>
      </blockquote>
      <br>
      The synchronization is not really needed, if you always use the
      keys to <br>
      lookup the messages. The static initializer in the Message class
      should <br>
      ensure correct ordering.<br>
      <br>
      Looking up messages "dynamically" means you have to trigger all
      the <br>
      code in the GUI that needs a translated message to be sure you got
      things <br>
      correct. Since there are several hundred messages I think the <br>
      static-fields-reflection approach is better.<br>
      <br>
      <blockquote cite="mid:4FBAAFEA.6090305@oracle.com" type="cite">
        <pre><span class="new">

  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, 
  </span><span class="new">I'm okay with pushing this approach to 7u6 and push 
  a better fix to jdk8.</span>  Or I miss the benefit you were
  considering :)</pre>
      </blockquote>
      <br>
      You are missing it :) <br>
      <br>
      .. and it's probably because I removed the code that did the
      actual check :)&nbsp;&nbsp; <br>
      <br>
    </blockquote>
    <br>
    I agree that checking one-to-one mapping between the keys in the
    property file and<br>
    the keys used in the Java code is good.&nbsp; What you need is to compare
    the list of constants with the keys in messages.properties.&nbsp; Anyway,
    my suggestion was just to simplify such initialization.&nbsp; You can go
    with what you have.<br>
    <i></i><br>
    <blockquote cite="mid:4FBBEB98.50704@oracle.com" type="cite">
      <blockquote cite="mid:4FBAAFEA.6090305@oracle.com" type="cite">
        <pre> &nbsp;<span class="new">
  There are a few names with '_' suffix e.g. L93, 97, 104, 160
 &nbsp;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.</span></pre>
      </blockquote>
      The keys were generated programmatically.<br>
      <br>
    </blockquote>
    <br>
    Ah - that's what I guess.<br>
    <br>
    <blockquote cite="mid:4FBBEB98.50704@oracle.com" type="cite"> The
      'COLON' was needed so I could differentiate between message that
      looked <br>
      the same, except for the ':' at the end. The pattern was applied
      to all&nbsp; <br>
      messages. Some of those message were removed (since they were no
      longer in use)<br>
      <br>
      I can remove the 'COLON'&nbsp; suffix where it's not needed, same thing
      with spaces.<br>
      <br>
    </blockquote>
    <br>
    That'd be good since "COLON" and '_' is meaningless w.r.t. the key
    name.&nbsp; IDE refactoring feature should make this renaming effortless
    :)<br>
    <br>
    <blockquote cite="mid:4FBBEB98.50704@oracle.com" type="cite"> I will
      go over the message keys and clean up the names, but they were not
      that <br>
      clean before either :)<br>
    </blockquote>
    <br>
    Thanks.<br>
    Mandy<br>
  </body>
</html>