Jon,

1. The patch has become a tad bit outdated after to your recent push.
Just a heads-up.

2.

    145         if (locale == Locale.getDefault()) {

That looks very conservative, I would expect `equals()`. Since it's about
a one-off optimization, it is not a big deal. I just stumbled upon that.

Thanks for small cleanups along the way.
Looks good to me.

-Pavel

> On 6 Feb 2020, at 19:26, Jonathan Gibbons <jonathan.gibb...@oracle.com> wrote:
> 
> Please review a change to use separate locales for messages written to the 
> console and for content written in the generated HTML pages.
> 
> Background: although the command-line help is not very informative about how 
> the -locale option is used, the man page is clear that it is clear that the 
> option affects the generated documentation.
> 
> Command-line help:
> 
>     -locale <name>
>                   Locale to be used, e.g. en_US or en_US_WIN
> 
> Man page:
> 
> `-locale` *name*
> :   Specifies the locale that the `javadoc` tool uses when it generates
>     documentation. The argument is the name of the locale, as described in
>     `java.util.Locale` documentation, such as `en_US` (English, United States)
>     or `en_US_WIN` (Windows variant).
> 
> With the proposed change, the -locale option only affects the generated 
> documentation; it does not affect console output, which will use the default 
> locale, in line with other JDK tools.
> 
> 
> 
> The changes ...
> 
> The core of the change is in HtmlConfiguration, which creates the resource 
> bundles for the Messages object (used to generate console messages) and for 
> Configuration.getResources(), which is used to get the resources for content 
> in the generated output.  To clarify the distinction, 
> BaseConfiguration.getResources is renamed to 
> BaseConfiguration.getDocResources, to emphasise that it is to be used for 
> resources in the generated documentation. This rename percolates through most 
> of the other affected files.  The rename also highlighted some issues in 
> HtmlOptions, which were using the low-level reporter and resources API to 
> generate messaages, instead of using the Messages API.  These have now been 
> fixed.
> 
> The tests ...
> 
> There's a somewhat surprising test case in TestSearch, which was testing the 
> search feature when using Japanese and Chinese locales. The primary part of 
> these test cases seems to be that the locale setting does *not* affect the 
> generated search index files, but perhaps as a control to verify the locale 
> option is taking effect, the test cases also verify some of the output 
> generated by javadoc. It's not clear how useful the test cases are, but 
> rather than simply remove them, each one is split in two: one to set the 
> default locale, and one to use the locale option.
> 
> The primary test update is the recently-new TestLocaleOption.java, which is 
> updated to be able to test the use of the default locale and the locale 
> option. The test is refactored a bit to share more of the common code for the 
> different test cases.
> 
> 
> 
> Additional work ...
> 
> The command-line help and man page should be improved. The man page states 
> that the -locale option should be first on the command line, which is no 
> longer the case. 
> The change should probably have a release note.
> 
> 
> 
> -- Jon
> 
> 
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8238437
> Webrev: http://cr.openjdk.java.net/~jjg/8238437/webrev/
> 
> 
> 
> 
> 

Reply via email to