RFR : 8213354 : Support package-specific stylesheets

Jonathan Gibbons jonathan.gibbons at oracle.com
Fri Feb 15 19:17:50 UTC 2019 

> 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. 

As I said in my previous email, you would NOT need to change the 
overriding methods in the subtypes.

Try playing with this example:

-------

import java.io.*;

interface Super {
     void m() throws IOException, ReflectiveOperationException;
}

class Suub implements Super {
     @Override
     public void m() throws FileNotFoundException { }
}

-------

Note how Super.m throws two exceptions and Sub.m just throws 1.
Note how Super.m throws IOException, but Sub.m throws 
FileNotFOundException, which is a subtype of IOException.

See:
https://docs.oracle.com/javase/specs/jls/se11/html/jls-8.html#jls-8.4.6
https://docs.oracle.com/javase/specs/jls/se11/html/jls-8.html#jls-8.4.8.3

  *

    For every checked exception type listed in the |throws| clause of
    |m_2 |, that same exception class or one of its supertypes must
    occur in the erasure (ยง4.6
    <https://docs.oracle.com/javase/specs/jls/se11/html/jls-4.html#jls-4.6>)
    of the |throws| clause of |m_1 |; otherwise, a compile-time error
    occurs.



-- 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
>>>>>
>>>>
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/javadoc-dev/attachments/20190215/9f9566d5/attachment-0001.html>

More information about the javadoc-dev mailing list