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
