RFR : 8219313 : Support module specific stylesheets

Jonathan Gibbons jonathan.gibbons at oracle.com
Fri May 17 21:12:29 UTC 2019 

Priya,

Much better.

One tiny comment change, and you'll be good to go.

In Head.java, change

160 * The stylesheets are relative to root of the generated 
documentation hierarchy.

to

160 * The paths for the stylesheets must be relative to root of the 
generated documentation hierarchy.

No need for additional review.

-- Jon


On 05/14/2019 03:23 AM, Priya Lakshmi Muthuswamy wrote:
>
> Hi Jon,
>
> I have modified the code to set the stylesheets to be relative to root 
> of the documented hierarchy.
>
> updated webrev : 
> http://cr.openjdk.java.net/~pmuthuswamy/8219313/webrev.02/ 
> <http://cr.openjdk.java.net/%7Epmuthuswamy/8219313/webrev.02/>
>
> Thanks,
> Priya
>
> On 5/14/2019 12:36 AM, Jonathan Gibbons wrote:
>>
>> Priya,
>>
>> I do not think it is worth adding a 3rd kind of stylesheet to 
>> Head.java, to "paper over" the differences between local-relative and 
>> root-relative paths to stylesheets. It should be easy enough to use a 
>> single form/policy for all stylesheets, main and additional, and to 
>> cover all bases, it would seem that root-relative arguments for 
>> Head.java might make more sense, so that clients do not have to 
>> convert the root-relative values for the main stylesheet and 
>> additional stylesheets specified on the command line.  In addition, 
>> other DocPaths used in Head.java are also root-relative, so it might 
>> be good to make that a documented rule.
>>
>> That then reduces to the issue of how to accommodate the 
>> module-specific and package-specific stylesheets. In your webrev, it 
>> looks like you are computing local-relative paths, by the use of 
>> DocPaths.forRoot on this line in HtmlDocletWriter:
>>
>> 2233 DocPath basePath = DocPaths.forRoot(pkgElement);
>>
>> and is also affected by this code in DocFilesHandlerImpl
>>
>>   131                 if (srcFile.getName().endsWith(".css"))
>>   132                     stylesheets.add(DocPaths.DOC_FILES.resolve(srcFile.getName()));
>>
>> So, I think you could do one of the following:
>>
>>  1. Convert these two places to generate root-relative paths.
>>     For line 2233, you could use DocPaths.forPackage instead of
>>     DocPaths.forRoot.
>>     For the other, you would need something like
>>     DocPaths.forRoot(((PackageELement)
>>     element).resolve(DocPaths.DOC_FILES).resolve(srcFile.getName()
>>
>>  2. Or, continue to use local-relative paths for getLocalStyleSheets,
>>     and convert them to root-relative in HtmlDocletWriter:444-448.
>>
>>
>> Either way, you have to either change command-line stylesheets or 
>> local stylesheets to make them follow the same policy.
>>
>> -- Jon
>>
>> On 05/13/2019 01:24 AM, Priya Lakshmi Muthuswamy wrote:
>>>
>>> Hi Jon,
>>>
>>> I will change the name for method getLocalStylesheetContent.
>>>
>>> Regarding the additionalStylesheets:
>>> I am passing both the package/module local stylesheets present in 
>>> doc-files and additional stylesheet( specified via --add-stylesheet) 
>>> to Head.setStylesheets.
>>> Both main and additional stylesheets are present in root of the 
>>> generated doc hierarchy.
>>> For package/module local stylesheets, since they are used only by 
>>> the local package/module files, i feel relative path from current 
>>> file is better than relative path from root
>>> of the doc hierarchy.
>>> getLocalStylesheets should get path for module/package doc-files and 
>>> pathToRoot.resolve will inverse and add to 
>>> it(../../ma/doc-files/spanstyle.css).
>>>
>>> Another way would be to change Head.setStylesheets method to take 
>>> three arguments.
>>> mainStylesheet, additionalStylesheet, package/module local stylesheets.
>>> For main and additional stylesheets, relative path can be calculated 
>>> inside Head.java(pathToRoot.resolve) and package/module local 
>>> stylesheets paths can be included directly.
>>>
>>> Kindly let me know on this.
>>>
>>> Regards,
>>> Priya
>>>
>>> On 5/11/2019 5:29 AM, Jonathan Gibbons wrote:
>>>
>>>> The name getLocalStylesheetContent is now inappropriate since it no 
>>>> longer returns any Content.
>>>> I suggest using just getLocalStylesheets.
>>>>
>>>> I see you are changing the type for getAdditionalStyleSheets in 
>>>> HtmlConfiguration.java and Head.java.
>>>>
>>>> I think I can see why you are doing that, but it is inconsistent to 
>>>> be changing the "additional stylesheets"
>>>> from using DocFile to DocPath, but then not changing the "main 
>>>> stylesheet" as well. You should either
>>>> neither or change both.
>>>>
>>>> In addition, if you change from using DocFile to DocPath, for both 
>>>> the main and additional stylesheets,
>>>> since DocPath's are relative paths you should add a comment to 
>>>> Head.setStylesheets to specify
>>>> whether the DocPaths should be relative to the root of the 
>>>> generated doc hierarchy or whether they
>>>> should be relative to the current file being generated. I don't 
>>>> have a strong opinion which of the two it
>>>> should be, but you should document it, and then make sure the 
>>>> implementation follows that rule.
>>>>
>>>> (FWIW, my intuition says that the paths should be specified 
>>>> relative to the root of the document
>>>> hierarchy, meaning that you will need to use pathToRoot.resolve 
>>>> _inside_ Head .java and not _outside_.)
>>>>
>>>> -- Jon
>>>>
>>>>
>>>>
>>>> On 05/10/2019 04:32 AM, Priya Lakshmi Muthuswamy wrote:
>>>>>
>>>>> Hi Jon,
>>>>>
>>>>> Have modified the code to set the style sheets via 
>>>>> Head.setStylesheets. Method getLocalStyleSheetContent will return 
>>>>> List<DocPath>.
>>>>>
>>>>> updated webrev: 
>>>>> http://cr.openjdk.java.net/~pmuthuswamy/8219313/webrev.01/ 
>>>>> <http://cr.openjdk.java.net/%7Epmuthuswamy/8219313/webrev.01/>
>>>>>
>>>>> Thanks,
>>>>> Priya
>>>>>
>>>>> On 5/10/2019 6:34 AM, Jonathan Gibbons wrote:
>>>>>>
>>>>>> The underlying mechanism of getLocalStyleSheetContent is 
>>>>>> generally wrong.
>>>>>>
>>>>>> The code should not be creating content containing LINK elements, 
>>>>>> when the code to do that is in Head.java.  The code should be 
>>>>>> creating a List<DocPath> containing pointers to all the 
>>>>>> stylesheet files, and should then be using Head.setStylesheets, 
>>>>>> which allows a list of additional files to be passed in.
>>>>>>
>>>>>> Also, in the test, can you put a couple of packages in the 
>>>>>> module, with different depths (e.g. package "p" and package 
>>>>>> "p.q") to ensure that the relative links work correctly in more 
>>>>>> cases.
>>>>>>
>>>>>> -- Jon
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 04/29/2019 09:21 PM, Priya Lakshmi Muthuswamy wrote:
>>>>>>>
>>>>>>> Hi Jon,
>>>>>>>
>>>>>>> getModuleStylesheetContent is used to get the relative path of 
>>>>>>> style-sheets in doc-files directory of the corresponding module.
>>>>>>>
>>>>>>> In the case of packages/class files, we pass the PackageElement 
>>>>>>> to the getLocalStylesheetContent,
>>>>>>> This method checks for the style-sheet in both the module and 
>>>>>>> package doc-files directory.
>>>>>>> getModuleStylesheetContent method checks whether there is a 
>>>>>>> named module and returns the relative path of the 
>>>>>>> style-sheet.(Ex: ../doc-files/mod-stylesheet.css)
>>>>>>>
>>>>>>> In the case of module files, we pass the ModuleElement to the 
>>>>>>> getLocalStylesheetContent, which just needs to check for 
>>>>>>> stylesheet in the local doc-files directory(Ex: 
>>>>>>> doc-files/mod-stylesheet.css)
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Priya
>>>>>>>
>>>>>>> On 4/29/2019 9:15 PM, Jonathan Gibbons wrote:
>>>>>>>>
>>>>>>>> At first glance, this looks questionable.
>>>>>>>>
>>>>>>>>> 2193 if (element instanceof PackageElement) {
>>>>>>>>> 2194 
>>>>>>>>> stylesheetContent.add(getModuleStylesheetContent((PackageElement)element));
>>>>>>>>> 2195 }
>>>>>>>> Is this supposed to be using PackageElement and not ModuleElement?
>>>>>>>>
>>>>>>>> -- Jon
>>>>>>>>
>>>>>>>>
>>>>>>>> On 4/29/19 2:32 AM, Priya Lakshmi Muthuswamy wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Kindly review the changes for supporting module specific style 
>>>>>>>>> sheets.
>>>>>>>>>
>>>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8219313
>>>>>>>>> webrev: 
>>>>>>>>> http://cr.openjdk.java.net/~pmuthuswamy/8219313/webrev.00/
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Priya
>>>>>>>>>
>>>>>>
>>>>
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/javadoc-dev/attachments/20190517/1a786fb6/attachment.html>

More information about the javadoc-dev mailing list