RFR : 8219958 : Automatically load taglets from a jar file

[Jonathan Gibbons]

On 03/06/2019 02:56 AM, Priya Lakshmi Muthuswamy wrote:
> Hi,
>
> Kindly review the changes for automatically loading taglets from a 
> service provider jar.
> JBS: https://bugs.openjdk.java.net/browse/JDK-8219958
> webrev: http://cr.openjdk.java.net/~pmuthuswamy/8219958/webrev.00/
> CSR: https://bugs.openjdk.java.net/browse/JDK-8219959
>
> Thanks,
> Priya
>
>

Priya,

*BaseConfiguration:*

You've moved the filemanager setup from TagletManager:216-226 into 
BaseConfiguration.
I agree it needs to be moved from its current position, but I think it 
should stay in TagletManager.
I suggest that you create a new method in TagletManager to init the 
taglet path.

The beginning of BaseConfiguration.initTagletManager would then be 
something like:

  837         tagletManager = tagletManager == null ?
  838                 new TagletManager(nosince, showversion, showauthor, javafx, this) :
  839                 tagletManager;
              tagletManager.initTagletPath(tagletPath);
  840         for (List<String> args : customTagStrs) {
  841             if (args.get(0).equals("-taglet")) {
842 tagletManager.addCustomTag(args.get(1), getFileManager(), tagletpath);
  843                 continue;
  844             }


As part of the suggestion to move stuff into BaseConfiguration, you had 
suggested using
`try ... catch (Exception e)`.   That would have been overly broad; if 
we were to continue
to do that, it would have been better to narrow the caught exception 
class to IOException.

*doclets.properties:*

The new error message needs improving. Think of how it would appear to 
an end-user:

/Error: could not set location for /some/path/input/by/the/user/

What is that going to mean to a user?  You also don't give any 
additional diagnostic information
that might be of assistance to remedy the situation. I don't like having 
to resort to using `e.toString()`
but when that's the only option, it's all we can do, and better than 
nothing.

I suggest the message should be something like:
/
//Error: could not set the taglet path: {0}/

where the provided value is the exception message.
*
**TagletManager.java:*

I've already suggested that you should move the init of the taglet path 
back into TagletManager.
The init code on lines 216-226 is somewhat curious, in that it gives 
precedence to the TAGLET_PATH
in the file manager if it has been set ... that may be because the code 
was executed repeatedly,
because addCustomTag is called in a loop.

We should refactor the init, to honor the taglet path option if it is 
set, and the TAGLET_PATH if it
is set when calling javadoc through the javax.tools API.

Put that all together, and the initTagletPath code should look something 
like the following (in pseudo-code)

     void initTagletManager(String tagletPath) {
         if (fileManager instanceof StandardJavaFileManager) {
             Standard JavaFileManager sfm = (StandardJavaFileManager) 
fileMaanger;
             if (tagletPath != null) {
                 sfm.setLocation(TAGLET_PATH,
tagletPath.splitAsStream(File.pathSeparator).collect(Collectors.toList()));
             } else if (!sfm.hasLocation(TAGLET_PATH)) {
                 sfm.setLocation(TAGLET_PATH, Collections.emptyList());
             }
         } else if (tagletPath != null) {
             messages.error("cannot set taglet path; the file manager is 
not a StandardJavaFileManager");
         }
     }

In addCustomTag, what exceptions are you catching? It may be that you 
can just catch
ReflectiveOperationException instead of Exception. If there are other 
exceptions being thrown,
consider using a multi-catch.

In loadTagletsJar ... the method name is weird as well as the doc comments.
I suggest ...

  * calling the method "loadTaglets" or "loadRegisteredTaglets" or
    something like that.
  * the javadoc should be "Loads taglets from the taglet path using
    ServiceLoader."

Remove the parameter "tagletPath" and remove the "if (tagletPath != 
null)" surrounding the
use of ServiceLoader. There is no need for the user to use -tagletpath 
... the user could be
calling javadoc through the javax.tools API with a preconfigured 
filemanager that already
has the  TAGLET_PATH set up.  Also, it would be better to split lines 
234,235 after the '='
instead of before the '(' to avoid splitting the method call.

234 ServiceLoader<jdk.javadoc.doclet.Taglet> serviceLoader =
235 fileManager.getServiceLoader(TAGLET_PATH, 
jdk.javadoc.doclet.Taglet.class);


In registerTaglet, you should not need to pass in the class name, since 
it is just the class name
of the instance parameter, isn't it?

Also in registerTaglet, although you haven't modified it, the body seems 
excessively verbose.
How about this:

245 /**
246 * Registers the Taglet.
247 * @param instance the Taglet instance
249 */
250 private void registerTaglet(jdk.javadoc.doclet.Taglet instance) {
  251         instance.init(docEnv, doclet);
  252         Taglet newLegacy = new UserTaglet(instance);
  258         allTaglets.put(newLegacy.getName(), newLegacy);
259 messages.notice("doclet.Notice_taglet_registered", 
instance.getClass().getName());
  260     }

It's pretty icky having two classes with simple name Taglet, forcing the 
use of fully-qualified
names. At some point (not necessarily in this review), we should rename 
the internal one
to something else. (We can't change the name of the one in the public API.)

-- Jon


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/javadoc-dev/attachments/20190308/8caa88f8/attachment.html>