I haven't seen webrev.01, so I'll describe what I see in webrev.02.
Some of the affected tests have been made to check for <dl> elements, where they
previously didn't:
"<dl class=\"notes\">"
For the record, it's not that those <dl> elements were not in the HTML in the
first place, it's just the tests are now checking that those <dl>s have
a particular form.
<span class="..."> with the classes like "paramLabel", "returnLabel",
"seeLabel",
"simpleTagLabel", and "throwsLabel" have gone from the associated <dt>s.
I can see that not all of the affected tests have been updated with this issue
number in their @bug. I skimmed through all those tests and I can see that it
makes sense. The tests have been tidied up a bit too. Thanks!
I agree with Hannes, this whole change makes HTML output more economical and
cleaner. This new structure makes much more sense.
As for that CSS class name, "notes". I don't have a strong opinion. Hannes and
yourself know that area much better than I do. So I trust your judgment,
especially that you've mentioned you had something (related) in the works that
gives you a broader context for this particular choice of name.
Nits
====
1. Since we are in this area, could you please fix the preexisting bad grammar?
That is, all the occurrences of "overriden" in "TestExternalOverridenMethod"
and everywhere under the test/langtools/jdk/javadoc/doclet/testPrivateClasses
directory.
2. Please escape "@param" in the top-level comment of the ParamTaglet type
with {@code ...}.
3. There's a line in "TestOverrideMethods" whose indentation is a bit off.
Otherwise looks good.
-Pavel
> On 26 Feb 2020, at 21:41, Jonathan Gibbons <[email protected]>
> wrote:
>
> Updated webrev, with one additional change. The new CSS class name is
> changed from "blockTags" to "notes". This was done automagically in a single
> command, with one manual fixup to move the position of the renamed member in
> the list, which is still alphabetically sorted (for now).
>
> I'm suggesting this rename now, because upcoming work suggests there are more
> definition lists involved that need to be subject to cleanup, for which
> "blockTags" is not a good CSS class name. The proposed name "notes" is
> intended to cover the lists generated for block tags, the lists for supertype
> and subtype information for a class, and the supertype information for an
> overriding method.
>
> New webrev:
>
> Webrev: http://cr.openjdk.java.net/~jjg/8239804/webrev.02/
>
> -- Jon
>
>
> On 02/25/2020 02:32 PM, Jonathan Gibbons wrote:
>> Hannes,
>>
>> Thanks for all the feedback; comments inline.
>>
>> New webrev:
>>
>> Webrev: http://cr.openjdk.java.net/~jjg/8239804/webrev.01/
>>
>> -- Jon
>>
>>
>> On 02/25/2020 07:05 AM, Hannes Wallnöfer wrote:
>>> Hi Jon,
>>>
>>> I think this is a good change overall. Less HTML tags, less CSS classes
>>> while still providing the same result.
>>>
>>> A few minor issues:
>>>
>>> - The new imports in ParamTaglet.java and MethodWriterImpl.java are not
>>> used.
>> Fixed.
>>>
>>> - In line 297 of MethodWriterImpl.java there’s a use of writer.contents
>>> that should use the local contents variable (or get rid of local var and
>>> just use writer.contents)
>> I'll keep the local variable, since it's used in a few places, and fix line
>> 297.
>>>
>>> - In TagletWriter#getParamHeader the javadoc @param tag needs to be
>>> renamed/updated as well.
>> Oops, yes. Fixed.
>>>
>>> - Given its slightly broader usage I’m not fully convinced about the
>>> „blockTags“ name, but can’t think of anything better. I’m fine with keeping
>>> the name for now, maybe we'll find something better in the upcoming CSS
>>> cleanup.
>> I'm not wildly enthusiastic about the name either, and like you, I can't
>> think of anything better right now. But, the only anomaly is the use for
>> overriding methods, and it's hard to come up with a meaningful term for both
>> block tags and overriding info. There's words like "info", "more info"
>> which are very generic; "details" is too specific and already has a
>> different meaning on the most of the same pages.
>>
>> There's also the CSS name "block" being used, that means something else, but
>> that is a better candidate to be renamed!
>>
>>>
>>> - I got very confused when I saw one of the CSS styles you removed in the
>>> generated docs in one of the module-overview.html pages. I even thought the
>>> generated docs were built with some other JDK, until I realised some of the
>>> taglets reside outside the src directory. Below is a patch that updates
>>> these taglet classes to conform to the new output. I think it should be
>>> included with this patch for consistency.
>>
>> Yeah, I know about the other taglets, and was going to handle them
>> separately, but now they've come up here, I'll fix them here as well.
>>
>>>
>>> diff -r 4f902f017def
>>> make/jdk/src/classes/build/tools/taglet/ModuleGraph.java
>>> --- a/make/jdk/src/classes/build/tools/taglet/ModuleGraph.java Tue Feb 25
>>> 09:46:12 2020 +0100
>>> +++ b/make/jdk/src/classes/build/tools/taglet/ModuleGraph.java Tue Feb 25
>>> 15:32:53 2020 +0100
>>> @@ -74,7 +74,7 @@
>>> + "</span>";
>>> }
>>> return "<dt>"
>>> - + "<span class=\"simpleTagLabel\">Module Graph:</span>\n"
>>> + + "Module Graph:\n"
>>> + "</dt>"
>>> + "<dd>"
>>> + "<a class=moduleGraph href=\"" + imageFile + "\">"
>>> diff -r 4f902f017def make/jdk/src/classes/build/tools/taglet/ToolGuide.java
>>> --- a/make/jdk/src/classes/build/tools/taglet/ToolGuide.java Tue Feb 25
>>> 09:46:12 2020 +0100
>>> +++ b/make/jdk/src/classes/build/tools/taglet/ToolGuide.java Tue Feb 25
>>> 15:32:53 2020 +0100
>>> @@ -95,7 +95,7 @@
>>> return "";
>>> StringBuilder sb = new StringBuilder();
>>> - sb.append("<dt class=\"simpleTagLabel\">Tool Guides:</dt>\n")
>>> + sb.append("<dt>Tool Guides:</dt>\n")
>>> .append("<dd>");
>>> boolean needComma = false;
>>>
>>>
>>> —
>>> Hannes
>>>
>>>
>>>> Am 22.02.2020 um 01:57 schrieb Jonathan Gibbons
>>>> <[email protected]>:
>>>>
>>>> Please review a change to cleanup/simplify the HTML and CSS used to
>>>> present block tags, like @see, @since, etc.
>>>>
>>>> Currently, the information from these tags is presented in a definition
>>>> list (<dl>). The labels, in the <dt> nodes, are wrapped in <span> using an
>>>> inconsistent set of CSS class names. Sometimes the names are shared,
>>>> sometimes they are unique.
>>>>
>>>> With this change, a new CSS class "blockTags" is put on the enclosing <dl>
>>>> node, and the <span> nodes wrapping the individual labels removed. This
>>>> does mean that it is no longer possible to style some of the labels
>>>> individually, but the use of CSS class names that were sometimes shared,
>>>> sometime unique, makes it unlikely that anyone tried to do this. If (and
>>>> only if) there is a demand to style tags individually, that should be
>>>> handled as a new Enhancement request, that might involve putting
>>>> tag-specific CSS class names on the <dt> and <dd> nodes for each tag (i.e.
>>>> without an additional <span>).
>>>>
>>>> There is one anomaly. The definition list for the block tags for a method
>>>> is also used to present information when the method overrides or
>>>> implements a method from a supertype. That makes the new CSS class name of
>>>> "blockTags" slightly less than ideal. Suggestions for a better name are
>>>> welcome.
>>>>
>>>> The code to generate the overall list of tags for any element is not as
>>>> centralized as might be desired. This changeset does not attempt to fix
>>>> that.
>>>>
>>>> There should be no visible change to docs generated using the default
>>>> stylesheet when the docs are viewed in a browser.
>>>>
>>>> The src/ code changes are relatively simple, and just consist of adjusting
>>>> the HTML and CSS that is generated. In some cases, there is some
>>>> additional code cleanup. About 30 tests are affected, although the changes
>>>> are generally simple and localized. The bug number has been added to the
>>>> @bug list for a subset of the tests; there are no new additional tests.
>>>>
>>>> ---
>>>>
>>>> A separate exercise would be to identify other <dl> nodes generated by the
>>>> doclet, and to add a CSS class to those nodes to identify the kind of list.
>>>>
>>>> -- Jon
>>>>
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8239804
>>>> Webrev: http://cr.openjdk.java.net/~jjg/8239804/webrev.00/
>>>>
>>
>
