RFR : 8213354 : Support package-specific stylesheets

Jonathan Gibbons jonathan.gibbons at oracle.com
Wed Feb 20 21:49:15 UTC 2019 

In DocFilesHandler.java, put the import for java.util.List first.

Otherwise OK.

No need for additional review.

-- Jon


On 02/17/2019 10:21 PM, Priya Lakshmi Muthuswamy wrote:
> 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