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/
