RFR: 8214738 : javadoc should honor styles in doc-files

Jonathan Gibbons jonathan.gibbons at oracle.com
Mon Jan 7 23:09:42 UTC 2019 

OK.

In Head.java. consider simplifying 266-268 to

     extraContent.forEach(tree::addContent);

No need for another review.

-- Jon


On 01/07/2019 01:20 AM, Priya Lakshmi Muthuswamy wrote:
>
> 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/22ac1b2a/attachment.html>

More information about the javadoc-dev mailing list