RFR: 8013839: Enhance Logger API for handling of resource bundles

[Daniel Fuchs]

Hi Mandy,

Thanks for your comments!

On 9/26/13 10:18 PM, Mandy Chung wrote:
> Hi Daniel,
>
> This is a good solution for Logger subclasses or existing apps that rely
> on the previous stack walk search to lookup a resource bundle.I have
> some comments on the specification:
>
> Logger.setResourceBundle(ResourceBundle rb)
>     Should this method simply throw NPE if rb == null since we can't
> reset a non-null rb/rbname to null?

Excellent suggestion.

>
> Some suggestion to the javadoc:
>
>
>    * Sets the resource bundle for this logger to use for localization.
>    *
>    * @param bundle  The resource bundle that this logger shall use.
>    * @throws  IllegalArgumentException if the given bundle doesn't have a
>    *         {@linkplain ResourceBundle#getBaseBundleName base name},
>    *         or if this logger already has a resource bundle set but
>    *         the given bundle has a different base name.
>

Done.

> The getLogger(String name, String rbname) spec and the class specification
> should be updated about different ways in setting a resource bundle for
> a logger.
>
> For example in the class spec, this paragraph and text following it talks
> about resource bundle and name and also logrb method:
>
>      Each Logger may have a ResourceBundle name associated with it.
>      The named bundle will be used for localizing logging messages.
>      If a Logger does not have its own ResourceBundle name, then
>      it will inherit the ResourceBundle name from its parent,
>      recursively up the tree.
>
> Adding @see #setResourceBundle in the getLogger method would be helpful.

I have reworked the part of the class specification that talks about
ResourceBundles and ResourceBundle names.

> It'd be good to clarify the spec of getResourceBundle and
> getResourceBundleName
> methods that they return the ResourceBundle and its base name if set via
> setResourceBundle method.  Perhaps it might be good for the class spec
> to capture different ways to set the resource bundle and and have the
> methods
> to refer that specification.

I have updated the documentation of these two methods - and it
should be more clear now.

>
> The current implementation of the getLogger(String name, String rbname)
> method
> uses the caller's class loader to look up the resource bundle.  Do you mind
> taking this opportunity to update the spec to match the current
> implementation?
> This fix is a follow-up on that change.

OK - I have updated the paragraph that explains how bundle names are
mapped to ResourceBundle objects in the class description.
Since the mapping - if required - happens at logging time - I think
it's better to leave it at the class level.

> Maybe we should just use varargs (JDK-5001993) to replace the two
> logrb methods one taking a single parameter and the other taking an
> array of parameters:
>      public void logrb(Level level, String sourceClass, String
> sourceMethod,
>                        ResourceBundle bundle, String msg, Object... params)
>

Done.

> I can live with the inconsistency.
>
> ResourceBundle.getBaseBundleName - this is more a question to Naoto.
>
> The implementation note about this method may return a pathname
> but the spec states that it returns a fully qualified class name
> is a bit strange.  Can you elaborate what problem we have here?
> I am missing the context to connect how this is related to the
> older versions of JRE

I have simplified this part with Naoto's help.

The new webrev is here:
http://cr.openjdk.java.net/~dfuchs/webrev_8013839/webrev.07/

Thanks a lot for all the good feedbacks!

-- daniel

>
> thanks
> Mandy
>
>
> On 9/20/2013 5:33 AM, Daniel Fuchs wrote:
>> Hi,
>>
>> Please find below a patch for:
>>
>> 8013839: Enhance Logger API for handling of resource bundles, and
>> 4814565: (rb) add method to get basename from a ResourceBundle
>>
>> <http://cr.openjdk.java.net/~dfuchs/webrev_8013839/webrev.06/>
>>
>>    1. It adds a ResourceBundle.getBaseBundleName() method,
>>    2. It adds a Logger.setResourceBundle(ResourceBundle) method,
>>    3. It adds a series of Logger.logrb(...) that take a ResourceBundle
>>         object instead of a ResourceBundle name.
>>         The previous versions of Logger.logrb(...) that took a
>>         resource bundle name are therefore no longer needed,
>>         they offer no added value above the new methods, and
>>         are thus now deprecated.
>>
>> Logger.getLogger(name, rbname) will continue to work as before,
>>      and will also throw an exception if a RB with a different name
>>      has been set before, either through setResourceBundle or
>>      Logger.getLogger(name, rbname).
>>
>> Logger.setResourceBundle works similarly to
>>      Logger.getLogger(name, rbname) in the sense that it will not
>>      allow to replace an existing bundle, unless both have the same
>>      name. Like for Logger.getLogger(name, rbname) - it doesn't matter
>>      by which method the previous bundle has been set.
>>
>>      This is mostly for consistency of the API - if thread A has
>>      requested a logger with resource bundle name 'foo.Bundle'
>>      and thread B attempts to set the resource bundle of that
>>      logger to 'bar.Bundle' then thread B will get an IAE.
>>
>>      The resource bundle passed to Logger.setResourceBundle *must*
>>      have a base name. Note that ResourceBundle objects will have
>>      a base name by default if they have been loaded through
>>      one of the ResourceBundle.getBundle(...) methods;
>>      If ResourceBundle.getBaseBundleName() returns null an IAE
>>      will be thrown.
>>
>>      Logger.setResourceBundle requires the "control" permission.
>>
>>      When a resource bundle is set with Logger.setResourceBundle,
>>      then it's that resource bundle locale that will be used
>>      when logging (and not the default locale).
>>
>> I have written a set of unit tests which should cover all the points
>> listed above.
>>
>> best regards,
>>
>> -- daniel
>