RFR : 8219313 : Support module specific stylesheets

Jonathan Gibbons jonathan.gibbons at oracle.com
Fri May 17 22:45:51 UTC 2019 

Typo in my correction. The line should be:

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

-- Jon

On 5/17/19 2:12 PM, Jonathan Gibbons wrote:
>
> 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/2d5c4ae0/attachment-0001.html>

More information about the javadoc-dev mailing list