RFR : 8219313 : Support module specific stylesheets

Jonathan Gibbons jonathan.gibbons at oracle.com
Mon May 13 19:06:13 UTC 2019 

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/20190513/12856ea3/attachment-0001.html>

More information about the javadoc-dev mailing list