Typo in my correction. The line should be:
160 * The paths for the stylesheets must be relative to the root of the
generated documentation hierarchy.
-- Jon
On 5/17/19 2:12 PM, Jonathan Gibbons wrote:
Priya,
Much better.
One tiny comment change, and you'll be good to go.
In Head.java, change
160 * The stylesheets are relative to root of the generated
documentation hierarchy.
to
160 * The paths for the stylesheets must be relative to root of the
generated documentation hierarchy.
No need for additional review.
-- Jon
On 05/14/2019 03:23 AM, Priya Lakshmi Muthuswamy wrote:
Hi Jon,
I have modified the code to set the stylesheets to be relative to
root of the documented hierarchy.
updated webrev :
http://cr.openjdk.java.net/~pmuthuswamy/8219313/webrev.02/
<http://cr.openjdk.java.net/%7Epmuthuswamy/8219313/webrev.02/>
Thanks,
Priya
On 5/14/2019 12:36 AM, Jonathan Gibbons wrote:
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
