RFR : 8213354 : Support package-specific stylesheets

Priya Lakshmi Muthuswamy priya.lakshmi.muthuswamy at oracle.com
Mon Feb 18 06:21:15 UTC 2019 

ok Jon.

updated webrev : http://cr.openjdk.java.net/~pmuthuswamy/8213354/webrev.03/

Thanks,
Priya

On 2/16/2019 2:01 AM, Jonathan Gibbons wrote:
> Priya,
>
> This is a mostly just a heads-up with some additional comments, that 
> only now came to mind.
>
> The Head class provides API to set stylesheets, but you are 
> "tunneling" the package-specific stylesheets into the <head> node via 
> the "extraContent" parameter. That is less than ideal, since it means 
> that the client code that is calling printHtmlDocument is creating the 
> <link> nodes, when it would  be better to leverage the code already in 
> Head.java.
>
> I apologize for not suggesting this before, but it only came to me 
> when I was reviewing other code, and noticed the code in 
> printHtmlDocument, and that it was calling Head.setStylesheets.
>
> But, even given all that, I'm inclined to stay with your current 
> strategy for the short term, because another item I've been 
> considering is to replace the "printHtmlDocument" call with a builder 
> and print call.  In other words, instead of having overloads of 
> printHtmlDocument with lots of arguments, it would be better to create 
> and use a different HtmlDocument class so that
>
>     printHtmlDocument(arg1, arg2, arg3);
>
> is replaced by
>
>     new HtmlDocument()
>         .setArg1(arg1)
>         .setArg2(arg2)
>         .setArg3(arg3)
>         .write(file);
>
> If we create this new class, it would be easy to design in a new method,
>
>     setLocalStylesheets(List<DocPath> files)
>
> which would mean we could eliminate the code in your current patch to 
> create the <link> nodes and to pass them in through the extraContent arg.
>
> -- Jon
>
>
> On 02/15/2019 04:08 AM, Priya Lakshmi Muthuswamy wrote:
>> Hi Jon,
>>
>> I have updated the fix based on the comments.
>>
>> Regarding throwing broader DocletException in getStylesheets, 
>> printDocument in various writer classes throw DocFileIOException,
>> If we have to change, then we might make changes in all these methods.
>>
>> I will log a separate bug for supporting module specific stylesheets.
>>
>> webrev : http://cr.openjdk.java.net/~pmuthuswamy/8213354/webrev.02/
>>
>> Thanks,
>> Priya
>>
>> On 2/15/2019 7:28 AM, Jonathan Gibbons wrote:
>>> It's looking good but could still be improved.
>>>
>>> 1. You haven't followed the typical coding patter for managing a 
>>> cache -- in this case the cache of stylesheets.
>>>
>>> The pattern is typically:
>>>
>>>     Foo foo = fooCache.get(key);
>>>     if (foo == null) {
>>>         foo = // compute foo
>>>         fooCache.put(key, foo);
>>>    }
>>>     // use foo
>>>
>>> In your case, you have most of that pattern in HtmlDocletWriter, but 
>>> the call to update the cache is buried inside DocFilesHandlerImpl.
>>>
>>> 2. I still don't really like the reference to stylesheets in 
>>> DocFilesHandler, but I can live with it, but you should follow the 
>>> style of the other method and declare that you throw DocletException 
>>> instead of the more specific DocFileIOException. That saves needing 
>>> the more specific import, and allows any future subtypes or upgrades 
>>> to use other subtypes of DocletException if desired. Note that you 
>>> can change the exception here without changing the throws on the 
>>> implementation method.
>>>
>>> 3. You are very close to supporting module-specifc stylesheets as 
>>> well as package-specific ones. I'm not suggesting you go all the way 
>>> for this, unless you want to, but I think you should at least 
>>> generalize the cache in HtmlConfiguration to be a 
>>> Map<Element,List<DocPath>> and change/generalize some of the method 
>>> names as well (e.g. by removing the Package word), so that they can 
>>> be reused if we choose to support module-specific stylesheets.
>>>
>>> It would be pretty easy to have what is currently 
>>> getPackageLocalStylesheetContent(PackageElement) automatically check 
>>> if the package has an enclosing module and look up the style sheets 
>>> for the enclosing module as well as the package.
>>>
>>> -- Jon
>>>
>>> On 02/12/2019 12:40 AM, Priya Lakshmi Muthuswamy wrote:
>>>> Hi Jon,
>>>>
>>>> Thanks for the review.
>>>>
>>>> I have modified the DocFilesHandlerImpl to get all the .css files, 
>>>> so that user is not restricted to one name.
>>>> Also included the Map in HtmlConfiguration.
>>>>
>>>> updated webrev : 
>>>> http://cr.openjdk.java.net/~pmuthuswamy/8213354/webrev.01/
>>>>
>>>> Thanks,
>>>> Priya
>>>>
>>>> On 2/9/2019 6:24 AM, Jonathan Gibbons wrote:
>>>>> Hi Priya,
>>>>>
>>>>> There are two significant issues that need to be addressed.
>>>>>
>>>>> 1.
>>>>>
>>>>> In DocFilesHandlerImpl you randomly take the first .css file you 
>>>>> find when listing the directory. If there should only be one, we 
>>>>> should specify the name (e.g. stylesheet.css). Otherwise, I think 
>>>>> you should honor all files found. Note that the order of iteration 
>>>>> of a directory is undefined, and may depend on the operating 
>>>>> system, and may vary over time. That is a very strong reason not 
>>>>> to just take the first file found when listing the directory.
>>>>>
>>>>> In terms of naming, "check..." is not a good name for this method. 
>>>>> I'd suggest either getStylesheet (if you only want to handle one) 
>>>>> or getStyleSheets() returning a List<DocPath> if you support more 
>>>>> than one.  Note that using a list allows you to easily handle no 
>>>>> files found (i.e. an empty list) as well as 1 or more files found.
>>>>>
>>>>> Minor nit: please ensure a space between a keyword and an open 
>>>>> parenthesis. You should be able to configure your IDE for that 
>>>>> style; it is the standard style for JDK code.
>>>>>
>>>>> 2.
>>>>>
>>>>> You should not be modifying anything for this feature in the 
>>>>> jdk/javadoc/internal/doclets/toolkit/ directory. The use of 
>>>>> stylesheets and CSS is specific to the HTML format, and any 
>>>>> changes should be limited to the directory 
>>>>> jdk/javadoc/internal/doclets/formats/html/ (and any subdirectories.)
>>>>>
>>>>> I realize you need to get information into the individual 
>>>>> *WriterImpl classes.  In general, the way we do that is to build 
>>>>> and use a table in the HtmlConfiguration object. Some class like 
>>>>> HtmlDoclet can stash information in that table; other Html classes 
>>>>> can access it.  The table can either be a 
>>>>> Map<PackageElement,DocPath> or a Map<PackageElement,List<DocPath>> 
>>>>> depending on whether you support at most one stylesheet per 
>>>>> package or many.
>>>>>
>>>>> If you keep a Map in HtmlConfiguration, you could also compute 
>>>>> entries lazily, and not bother with editing HtmlDoclet. For 
>>>>> example, no entry in the cache means it has not been computed yet, 
>>>>> an empty list in the cache means no package-specific stylesheets, 
>>>>> a non-empty list means you have local stylesheets. This 
>>>>> computation can all be done in DocFilesHandlerImpl, with just the 
>>>>> cache object itself being retained in HtmlConfiguration.
>>>>>
>>>>> -- Jon
>>>>>
>>>>>
>>>>> On 01/30/2019 04:18 AM, Priya Lakshmi Muthuswamy wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Kindly review the fix for 
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8213354
>>>>>> Package specific stylesheets needs to placed under doc-files 
>>>>>> directory of a package.
>>>>>> DocFilesHandler looks for any css file and adds them to the 
>>>>>> generated html pages.
>>>>>>
>>>>>> webrev : http://cr.openjdk.java.net/~pmuthuswamy/8213354/webrev.00/
>>>>>>
>>>>>> Thanks,
>>>>>> Priya
>>>>>>
>>>>>
>>>
>

More information about the javadoc-dev mailing list