On Wed, 29 Sep 2021 06:09:53 GMT, Hannes Wallnöfer <hann...@openjdk.org> wrote:
> 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 `VisibleMemberTable` 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 > `VisibleMemberTable` 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. This pull request has now been integrated. Changeset: 055de6f5 Author: Hannes Wallnöfer <hann...@openjdk.org> URL: https://git.openjdk.java.net/jdk/commit/055de6f566208b168818be1dc3ad29cdb9caa1cf Stats: 1646 lines in 21 files changed: 708 ins; 883 del; 55 mod 8223358: Incorrect HTML structure in annotation pages Reviewed-by: jjg ------------- PR: https://git.openjdk.java.net/jdk/pull/5746
