Updated webrev. Only minor changes:
1. Improved comments on {Base,Html}Configuration
2. Changed locale in test to en_GB_ALLCAPS
3. Filed JDK-8238503 for ToolOption cleanup.
-- Jon
JBS: https://bugs.openjdk.java.net/browse/JDK-8222793
Webrev: http://cr.openjdk.java.net/~jjg/8222793/webrev.01/
On 02/04/2020 09:37 AM, Pavel Rappo wrote:
1. Javadoc comments for constructors of the HtmlConfiguration and
BaseConfiguration
classes could be updated. The comment for the HtmlConfiguration ctor could be
deleted, as there's not much value in it. The comment for the BaseConfiguration
ctor should better be updated to reflect the change in the signature.
2. The test case is impressive! I wonder if we could create a completely
contrived name for that new locale. Perhaps en_ALL_CAPITALS, or the like. The
reason is that the en_UK locale could be considered as a mistyped en_GB, which
is the real locale. It's not a big deal though.
Otherwise looks good.
On 2 Feb 2020, at 22:37, Jonathan Gibbons <[email protected]> wrote:
A couple of minor follow-ups.
1. ToolOptions has a ToolOption object for `-locale`, as it should (so that the
option shows up in command-line help) but it sets a field `docLocale` with a
corresponding accessor, which IIRC are unused. They could be removed either in
this changeset, or soon, perhaps with some improved comments in ToolOptions.
If you choose to remove those is some other changeset, you could put a TODO or
a FIXME comment somewhere near that field and its getter, so it would be clearly
seen. Otherwise, we'll likely forget about it.
-Pavel
2. Another test case, that would more closely mirror the reported conditions
for the error, would be to set a locale other than en_US as the default locale
for javadoc, and then verify that using the `-locale en_US` option sets the
locale correctly.
-- Jon
On 1/31/20 4:48 PM, Jonathan Gibbons wrote:
Please review a medium-small fix for a regression in the new doclet.
The problem is that the -locale option is not being handled correctly, and is
not taking effect as it should.
[Note: for a while, I was confusing the issue with a related related of
possibly using different locales for the console messages and the generated
docs. This is not that one.]
The root cause is that some doclet objects are being initialized either too
early or in the wrong order. The fix is to address the order of initialization,
which allowed some unexpected additional cleanup.
The locale option is picked up in the first pass over the options in Start,
which determines the doclet and locale. After that first pass, the doclet is
created and its init method called, passing in the locale. But, inside the
doclet, the configuration object and some of its contents were created in the
doclet's constructor, before the init method is called with the locale to use.
They end up seeing a null locale, which translates to the default locale.
The fix is to delay initializing the configuration until the init method, which
has the downside of making it not final, but the unexpected upside is that
inside the configuration, a couple of lazy init methods can be removed, and the
corresponding fields themselves made final. (So, net increase in final fields;
yay!)
The test is a bit tricky. We need to make sure that setting the -locale option correctly
affects the various forms of generated output. But by default, the only locales we
provide are asian and working with those Unicode characters can be challenging for some
of us. The solution is to generate a new set of resource files with
similar-but-different content, and to install them in a different "test"
locale. Any easy conversion is to convert the values in the property files to upper
case, and then to use another English locale, such as en_UK. The easiest way to install
new resource files is to use the --patch-module VM option, which means running javadoc in
a child VM, which means we can't use the standard JavadocTester framework, which always
uses same-vm mode. But, it's easy enough to use the toolbox library classes instead. The
test runs javadoc a few times, with and without the -locale option, and verifies the
generated text is coming from either the default resource bundle, or the uppercased
resource bundle, as appropriate.
The changeset has been tested on all standard platforms.
-- Jon
JBS: https://bugs.openjdk.java.net/browse/JDK-8222793
Webrev: http://cr.openjdk.java.net/~jjg/8222793/webrev/
