Please review a fix for a simple bug that somehow mushroomed into a major 
cleanup of the annotation member builder and writer code. I still think it is 
justified to do this as part of a bug fix since in my eyes it is the only 
reasonable way to fix the issue (lest we want to add more complex workarounds).

The problem is that currently we have two versions of each class used to 
generate annotation interface member documentation, one for required members 
and one for optional ones. However, only the summary tables are separated for 
these two kinds of members, while the details section uses one single list for 
both. This required coordination between the two builder classes to generate a 
single list without generating faulty (missing or duplicate) HTML. Obviously 
this workaround was flawed, since it avoided the duplicate headers but still 
generated duplicate lists and sections that should have been single elements. 

Even for the summary section, the dual class setup seemed like overkill, since 
the two summary lists differ only in label/text content, and the only thing the 
optional member writer could do extra was to generate the default value info.

So the core of this change unifies the dual annotation member builder and 
writer classes into single classes. For the writer, this simply involves adding 
a few switch expressions to retrieve the correct text values based on the value 
of a new nested enum class. The builder class is now simply instantiated once 
instead of twice to generate the member details list, and it does it for all 
annotation members.

Since the builder also uses the writer to generate the unified details list, 
the new enum has 3 values: `OPTIONAL`, `REQUIRED` and `ANY`. I'm not totally 
happy with this setup, but IMO it is still better than before and I have added 
a few comments to explain the reasons behind it.

To make retrieval of combined annotation members easier I added a new member 
kind to `VisualMemberTable` class called `ANNOTATION_TYPE_MEMBER`. This also 
allowed me to simplify the subnavigation code in `Navigation.java` which also 
contained some ugly workarounds for annotation interfaces. In the process I 
also reestablished the old order of annotation member subnavigation links to 
put the required members link first - this had been changed inadvertently in 
JDK 17. 

The change in return type to the `getVisibleMembers` methods in 
`VisualMemberTable` from `List<? extends Element>` to `List<Element>` is from 
when I did manual list merging with these return values. I left it in because I 
think it potentially makes other future uses of these methods easier. 

I did a recursive diff on the generated documentation before and after the fix. 
Obviously the reversed annotation member subnav links are changed in every 
annotation page. Apart from that, the only annotation interface that contains 
both required and optional members in the JDK (and therefore the only one that 
is affected and benefits from this fix) is 
`javax.annotation.processing.Generated` in the `java.compiler` module. 

As a sidenote: I also considered changing the nomenclature of the whole bunch 
of classes from the obsolete "annotation type" to "annotation interface", but I 
think that would have been an even bigger disruption in the code. If we want to 
do this I would prefer to do it as a separate task.

-------------

Commit messages:
 - JDK-8223358: Avoid unnecessary creation of list
 - JDK-8223358: Further cleanup
 - JDK-8223358: Add some explanatory comments
 - JDK-8223358: Update copyright headers
 - JDK-8223358: Incorrect HTML structure in annotation pages

Changes: https://git.openjdk.java.net/jdk/pull/5746/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5746&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8223358
  Stats: 1645 lines in 21 files changed: 707 ins; 883 del; 55 mod
  Patch: https://git.openjdk.java.net/jdk/pull/5746.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/5746/head:pull/5746

PR: https://git.openjdk.java.net/jdk/pull/5746

Reply via email to