RFR : 8219313 : Support module specific stylesheets

[Jonathan Gibbons]

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/20190510/f514c670/attachment.html>