RFR: 8214738 : javadoc should honor styles in doc-files
Priya Lakshmi Muthuswamy
priya.lakshmi.muthuswamy at oracle.com
Mon Jan 7 09:20:32 UTC 2019
Hi Jon,
Updated webrev : http://cr.openjdk.java.net/~pmuthuswamy/8214738/webrev.02/
Thanks,
Priya
On 1/5/2019 4:16 AM, Jonathan Gibbons wrote:
>
> DocFilesHandlerImpl.java:198
> Why is the `if` statement necessary? Why would the call on 201 behave
> differently to the call on 199 when localTagsContent is empty? As I
> aid in an earlier review, I think you can replace 198-202 with just 199.
>
> HtmlDocletWriter: 431
> Use Collections.emptyList instead of null, to simplify 458
>
> HtmlDocletWriter:442
> Please don't use the word "header" in this context. I suggest you
> follow the convention in Head.java and call the parameter "extraContent".
>
> HtmlDocletWriter: 458
> Remove null check, and ensure that null is never passed in on 431.
>
> Head.java
> OK, although the old arrangement was deliberate/intentional, looking
> down the road at better support of stylesheets, I guess the revised
> version is more logical.
>
> -- Jon
>
>
> On 12/25/2018 08:05 PM, Priya Lakshmi Muthuswamy wrote:
>>
>> Hi Jon,
>>
>> Javadoc styles for header/navbar will appear as it is and will not
>> get disturbed.
>>
>
> You're assuming that the javadoc styles for the header/navbar will not
> be affected by the user styles ;-)
>
>> I meant for any html content in doc-files, if there is any
>> user-defined style, then those should override the javadoc styles.
>> In html head element, default(javadoc) styles will be placed first
>> and then those defined in doc-file.
>>
>> Thanks,
>> Priya
>>
>> On 12/21/2018 9:24 PM, Jonathan Gibbons wrote:
>>
>>> I've not yet read the full webrev, but I do have one immediate
>>> question, given below.
>>>
>>> -- Jon
>>>
>>> On 12/21/18 2:13 AM, Priya Lakshmi Muthuswamy wrote:
>>>>
>>>> Hi Jon,
>>>>
>>>> Thanks for the review.
>>>>
>>>> I have made the following changes:
>>>> 1)Added style tag in HtmlTag.java, before we were using style
>>>> attribute for comparison.
>>>> 2)utils.getPreamble(element) ignores new lines after html tags, so
>>>> I can included them while fetching the localHeadTags.
>>>> 3)In Head.java, used addContent() itself instead of new methods.
>>>> I had seen the addContent(), but it was adding the content before
>>>> the scripts/styles tags.
>>>> The doc-files script/style needed to added at the end of the other
>>>> styles it that it overrides them.
>>>>
>>> This is a red flag. Why is it important that the user styles
>>> override the javadoc styles? Given that we're adding javadoc styles
>>> for the javadoc header/navbar, isn't it important that those should
>>> appear as intended?
>>>
>>>
>>>> When I checked the usage of addContent, i see that its been used
>>>> only by IndexRedirectWriter to include script/noscript tags.
>>>> I think that can be added at the end of existing tags as well.
>>>>
>>>> updated webrev:
>>>> http://cr.openjdk.java.net/~pmuthuswamy/8214738/webrev.01/
>>>> api: http://cr.openjdk.java.net/~pmuthuswamy/8214738/api/
>>>>
>>>> some of the doc-file with user specific styles.
>>>> api/jdk.jdi/com/sun/jdi/doc-files/signature.html
>>>> api/java.base/java/lang/doc-files/threadPrimitiveDeprecation.html
>>>> api/java.desktop/java/awt/doc-files/Modality.html
>>>>
>>>> Thanks,
>>>> Priya
>>>>
>>>> On 12/21/2018 3:16 AM, Jonathan Gibbons wrote:
>>>>>
>>>>> Priya,
>>>>>
>>>>> In DocFilesHandlerImpl, you have a boolean flag titleOrMetaTags.
>>>>> Since <meta> elements never have content, and so never have end
>>>>> elements, it seems misleading/incorrect to set the flag to true.
>>>>> Also, you omit to handle ENTITY in the <title> element, so I added
>>>>> that in as well. You could also write this code as a visitor if
>>>>> you wanted, but the switch form is not too bad.
>>>>>
>>>>> I suggest the code should look something like this:
>>>>>
>>>>> 205 List<DocTree> localTags = new ArrayList<>(); 206 boolean
>>>>> inHead = false;
>>>>> 207 boolean inTitle = false; loop: 208 for (DocTree dt : dtrees) {
>>>>> 209 switch (dt.getKind()) {
>>>>> 210 case START_ELEMENT:
>>>>> 211 StartElementTree startElem = (StartElementTree) dt; switch
>>>>> (HtmlTag.get(startElem)) { case HEAD: inHead = true; break;case
>>>>> META: break; case TITLE: inTitle=true; break; default: if (inHead)
>>>>> { localTags.add(startElem); } }
>>>>> 223 break; 224 case END_ELEMENT: 211 EndElementTree endElem =
>>>>> (EndElementTree) dt; switch (HtmlTag.get(endElem)) { case HEAD:
>>>>> break loop; case TITLE:
>>>>> inTitle = false;
>>>>> break loop;default: if (inHead) { localTags.add(startElem); } }
>>>>> 235 break; case ENTITY: 236 case TEXT:
>>>>> 237 if (inHead && !inTitle) {
>>>>> 238 localTags.add(dt);
>>>>> 239 }
>>>>> 240 break;
>>>>> 241 }
>>>>> -- Jon
>>>>>
>>>>> On 12/20/2018 11:31 AM, Jonathan Gibbons wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 12/19/2018 08:38 PM, Priya Lakshmi Muthuswamy wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Kindly review the fix for
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8214738
>>>>>>> webrev : http://cr.openjdk.java.net/~pmuthuswamy/8214738/webrev.00/
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Priya
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> I don't think you need to add anything (userHeaderTags etc) to Head.
>>>>>> Head already has the requisite feature, "extraContent" and
>>>>>> "addContent"
>>>>>> which can be used to add content into the <head> element.
>>>>>>
>>>>>> Re:
>>>>>> Utils.toLowerCase(nodeEnd.getName().toString()).equals(HtmlTag.HEAD.getText())
>>>>>> Don't use long-winded String comparison when you have type-safe
>>>>>> comparison.
>>>>>> In this case, you should be able to do something like
>>>>>> HtmlTag.get(nodeEnd.getName()) == HtmlTag.HEAD
>>>>>>
>>>>>> getLocalHeaderTags needlessly walks through all the <body>
>>>>>> element. You can
>>>>>> break out of the loop once you hit </head>.
>>>>>>
>>>>>> DocFilesHandlerImpl:196.
>>>>>> The "if" statement is a waste of time and effort. The code will
>>>>>> work perfectly
>>>>>> well when localTagsContent is an empty list, so you can simplify
>>>>>> 196-199 to just 197.
>>>>>>
>>>>>> TestCopyFiles.java
>>>>>> (style) re: indenting the continuation of multi-line strings
>>>>>> The general Java style for breaking long expressions is to break
>>>>>> before a binary operator, not after.
>>>>>> The style in other javadoc tests is to not indent the
>>>>>> continuation expressions, so that it is easier
>>>>>> to see how the first line and subsequent lines are aligned.
>>>>>>
>>>>>> -- Jon
>>>>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/javadoc-dev/attachments/20190107/2f317571/attachment.html>
More information about the javadoc-dev
mailing list