RFR [15] 8239487: Better links generation for system properties found in HTML files

[Jonathan Gibbons]

    phase-1

*In AbstractIndexWriter*,
the use of "anyOf" with a singleton list seems a bit weird.

*In SearchIndexItem*:
The word "index" is heavily overloaded in javadoc. Would it be better in 
this case to use "INDEX_TAG"? (Just asking).
If nothing else, add doc-comments on the members of Category.

Folding "boolean systemProperty" into the Category seems like a good idea.

*In SearchIndexItems*:   ugh for having to put the apiNote after the 
scary comment. Nothing we can do, except maybe one day think about 
removing the scary comments, which have maybe outlived their usefulness. 
Maybe we could limit them to package-info.java fies.

120 * <p> The returned stream consists of all items {@code i} for which 
there's

It's often considered better to avoid contractions (like "there's") in 
formal writing.

120 * <p> The returned stream consists of all items {@code i} for which 
there's
121 * a category {@code c}, from the specified categories, such that
122 * {@code i.getCategory().equals(c)}. The stream may be empty, if 
there are
123 * no such items.

The commas are questionable around "from the specified categories". 
Don't use commas when the enclosed text is important to the 
understanding of the sentence.

`concat` seems overkill.  For an internal method, I'd simplify the 
anyOfCategories, and either use overloads or a single varargs and just 
verify the length is not zero.  By inspection, it seems to only ever use 
1 or 2 args. Use overloads!


    phase-2

AbstractIndexWriter
line 511,512: horrible non-standard formatting: was this suggested by an 
IDE?

keyCharacter: when is this method called with an empty string? It looks 
like it is related to the SearchIndexItem having en empty label ... 
perhaps we should check/fix the problem there.
Be careful of using Character.toUpperCase/toLowerCase ... they are 
locale-dependent.
That being said, maybe we do want the locale-dependent version here, 
which is unusual.
While it is reasonable to get the first character of a Java identifier, 
I wonder if it is reasonable
to get the first character of an arbitrary string, i.e. from an {@index} 
tag.

various: it's not wrong, but I personally don't like the style of 
importing static methods, like groupingBy and toList. Keep it if you 
want, but if I were writing the code, I would use Collectors.methodName


    phase-3

HtmlDocletWriter: this class is one instance per page ... do you really 
mean to put it here, as compared to Configuration (more global) or some 
more specific kind of page?  (I see it moved in phase 4!)

SystemPropertiesWriter:80
I don't know if this is the right or wrong place for the check, but for 
reference I would compare with other "optional" pages, like Constant 
Values and Serialized Form.

SystemPropertiesWriter:155
I like the intention, but I would have thought the link factory would 
have returned a link in code font. I guess I would check other places 
where links are generated.

TagletWriterImpl:457
The comment is unclear: does "those" refer back to DocFileELement?  I'm 
also curious why you think using identity equals and hashCode is 
inefficient.

Utils:  uuugh ... I keep trying to get stuff *out* of the Utils dumping 
ground, and here you are adding to it. This is OK for now, but there may 
be a different utility class waiting to be written for processing 
DocTrees. This is not the only method to be working on DocTrees 
(although other work may be elsewhere, down in jdk.compiler module.)

test: the new convention is to generate files on the fly ... which will 
be even easier when we have text blocks. The ToolBox library class has 
code to help write out smal files like these ... and using text strings 
avoids the heavy copyright overhead.


    phase-4

In SystemPropertiesWriter, there is no need to use a WeakHashMap 
compared to a standard HashMap. Elements created by javac, which is the 
dominant collection of elements, will be permanent until the end of the 
javadoc run. And moreover, SystemPropertiesWriter should have a short 
lifetime, and no elements will be garbage-collectible while it is alive.

Line 172: I see the comment moved here from TagletWriterImpl. Same 
comments apply that I made for phase-3.

In SearchIndexItem, if I understand it correctly, it seems strange to 
add a field of type DocletElement that is specific to certain types of 
search index item.  And, there is a cognizant disparity between the name 
of the type (DocletElement) and the name of the method (getElement). 
Generally, I would expect a method named `getElement` to return an 
Element. It seems like all other serach-index items are associated with 
n element anyway!



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/javadoc-dev/attachments/20200306/040c7e12/attachment.htm>