Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[NETBEANS-1396] Missing code completion and Javadocs in maven projects with classifier #1548

Merged
merged 3 commits into from
Jan 30, 2020
Merged

Conversation

zimmi
Copy link
Contributor

@zimmi zimmi commented Oct 5, 2019

Edit: No need to rush this for 11.2.

This solves NETBEANS-1396 and NETBEANS-2197 with the goal of supporting JavaFX / OpenJFX Javadoc and Go to Declaration. I split the solution into two commits for easier review:

  1. Add support for the changed output of javadoc 9+. Don't be intimidated by the amount of code, it's mostly HTML test resources.
  2. OpenJFX uses Maven classifiers for platform dependent code, i.e. javafx-graphics-13.jar does not contain any code, but it depends through build profiles on javafx-graphics-13-linux.jar. There is however no javafx-graphics-13-linux-javadoc.jar, only a javafx-graphics-13-javadoc.jar, which NetBeans doesn't find. This PR lets NetBeans fall back to the main javadoc jar if there is none associated with the one with the classifier.
  3. (Part of the second commit) The contents of the javadoc jars of OpenJFX are prefixed by module name, which is different from the default output of the maven-javadoc-plugin. This PR does some bytecode reading to still find the correct .html file.

To link the discussions together: openjfx/openjfx-docs#44, openjfx/javadoc#6 and a stackoverflow question.

@zimmi zimmi changed the title [NETBEANS-1396] [WIP] Missing code completion and Javadocs in maven projects with classifier [NETBEANS-1396] Missing code completion and Javadocs in maven projects with classifier Oct 6, 2019
@neilcsmith-net neilcsmith-net added this to the 11.3 milestone Oct 7, 2019
@jgneff
Copy link
Contributor

jgneff commented Oct 26, 2019

I created NETBEANS-3296 to track all the errors related to using the JavaFX Javadoc and Sources in NetBeans. An identical copy of the issue description is on GitHub at jgneff/netbeans-doc-src.

@jgneff
Copy link
Contributor

jgneff commented Nov 7, 2019

Thomas asked me to test this pull request. Below are the results of my tests using the Non-Modular Maven project of the JavaFX samples repository. Once we resolve the issues with this first project, I can test the other sample projects described in NETBEANS-3296.

I ran the tests with OpenJDK 13.0.1 on Ubuntu 16.04.6 LTS. I did not install the nb-javac plug-in so as to avoid the exceptions it encounters. For the Java Platform Javadoc, I left the default entry created by NetBeans, which lists only the docs directory of the JDK.

platform-javadoc

Before downloading the JavaFX Javadoc and Sources, neither were available to NetBeans, as expected.

I right-clicked the Dependencies node in the Project window and selected Download Javadoc. The Javadoc artifacts were downloaded to the local Maven cache under the directory ~/.m2/repository/org/openjfx.

javafx-base/13.0.1/javafx-base-13.0.1-javadoc.jar
javafx-base/13.0.1/javafx-base-13.0.1-javadoc.jar.sha1
javafx-controls/13.0.1/javafx-controls-13.0.1-javadoc.jar
javafx-controls/13.0.1/javafx-controls-13.0.1-javadoc.jar.sha1
javafx-fxml/13.0.1/javafx-fxml-13.0.1-javadoc.jar
javafx-fxml/13.0.1/javafx-fxml-13.0.1-javadoc.jar.sha1
javafx-graphics/13.0.1/javafx-graphics-13.0.1-javadoc.jar
javafx-graphics/13.0.1/javafx-graphics-13.0.1-javadoc.jar.sha1

The Javadoc badges appeared on those dependencies without the -linux.jar platform classifier.

dependencies-javadoc

To my surprise, the Javadoc then displayed perfectly even before attaching the Sources! I had previously thought that NetBeans always got the Javadoc from the comments in the source code.

The Javadoc window showed the full description when selecting the setScene method.

javadoc-setScene

The full description was displayed when selecting the show method.

javadoc-show

Clicking the toolbar button to show the documentation in an external Web browser opened my browser to the correct location.

browser-setScene

Furthermore, the Javadoc for the Java Platform was found with the default entry created by NetBeans. Previously, you had to edit the Platform Javadoc and add the specific Java module directories for the information to display correctly.

javadoc-getClass

Clicking through to the Sources showed only the Compiled Code, as expected.

compiled-code-setScene

Next, I right-clicked the Dependencies node in the Project window and selected Download Sources. The Sources artifacts were downloaded to the local Maven cache under the directory ~/.m2/repository/org/openjfx.

javafx-base/13.0.1/javafx-base-13.0.1-sources.jar
javafx-base/13.0.1/javafx-base-13.0.1-sources.jar.sha1
javafx-controls/13.0.1/javafx-controls-13.0.1-sources.jar
javafx-controls/13.0.1/javafx-controls-13.0.1-sources.jar.sha1
javafx-fxml/13.0.1/javafx-fxml-13.0.1-sources.jar
javafx-fxml/13.0.1/javafx-fxml-13.0.1-sources.jar.sha1
javafx-graphics/13.0.1/javafx-graphics-13.0.1-sources.jar
javafx-graphics/13.0.1/javafx-graphics-13.0.1-sources.jar.sha1

The Sources badges appeared on those dependencies without the -linux.jar platform qualifier.

dependencies-javadoc-sources

Here I started to see some unexpected behavior. Selecting the show() method in the App.java file no longer shows the method description as it did before.

javadoc-show-no-description

Selecting the setScene method still shows the full description in the Javadoc window, but Ctrl-clicking the method does not navigate to its location in the source file. It opens the source file with the cursor on the first line, even though it navigated to the correct location in the Compiled Code view previously.

source-setScene

To see what's happening, I deleted the Javadoc from the Maven cache and left just the Sources. I closed and reopened the project.

dependencies-sources

Now we see the same problems. The show method has no description.

javadoc-show-sources-only

The Ant projects in NetBeans 11.2, though, are able to show the Javadoc for this method using only the comments in the source code, as shown in the screenshot below. I expected the Maven project to provide the same information. Note that you can tell whether the Javadoc and Sources are available by looking at whether the corresponding toolbar buttons are enabled. In the images above and below, only the Sources button is enabled. No Javadoc is attached.

ant-javadoc-show

The setScene method displays no Javadoc at all, even though Javadoc comments are available in the source code.

javadoc-setScene-sources-only

Navigating to the show method's source works, but for setScene the file still just opens to the first line. The two methods in the source code are defined as follows.

    /**
     * Specify the scene to be used on this stage.
     */
    @Override final public void setScene(Scene value) {
        Toolkit.getToolkit().checkFxUserThread();
        super.setScene(value);
    }

    /**
     * {@inheritDoc}
     */
    @Override public final void show() {
        super.show();
    }

How should the Javadoc be displayed now? I'm guessing at the following algorithm based on the results of the all tests I've been doing here and for NETBEANS-3296.

  • If the Javadoc is attached and an entry exists for the selected item, use the HTML to display the Javadoc.
  • Otherwise, if the Sources are attached and there are Javadoc comments for the selected item, use those comments to display the Javadoc.
  • Otherwise, prompt the user to attach the Javadoc or Sources. In the Javadoc window and code completion, prompt the user to attach the Javadoc. In the Compiled Code view, prompt the user to attach the Sources.

@lkishalmi
Copy link
Contributor

The changes look sane. +1 for merging this one.

@jgneff
Copy link
Contributor

jgneff commented Nov 8, 2019

The changes look sane. +1 for merging this one.

I would prefer fixing the problems that occur after downloading the Sources: the Javadoc description disappears and the source navigation fails. I know, easy for me to say. 😃 We could, however, fix these in pieces and open new issues for those problems.

I just ran some more tests to make sure that this pull request doesn't break the technique I use to work on the JavaFX Graphics module in NetBeans. The work-around is to create an Ant-based project and add the individual JAR files to the Module or Compile Classpath, attach the appropriate Javadoc and Sources directories to each JAR file, remove the Javadoc and Sources from the Global Library, and uninstall the nb-javac plug-in. That still works as described in the Ant Non-Modular and Ant-Modular sections of NETBEANS-3296.

@lkishalmi
Copy link
Contributor

I'd say merge this in resolve the rest as separate issues.

@zimmi
Copy link
Contributor Author

zimmi commented Nov 8, 2019

Thank you so much for the detailed writeup @jgneff! Those are some good test cases, I will investigate.

I'm fine with merging this now, I'll open another PR for the disappearing javadoc after attaching sources. Is there already an issue for this?

I'm not sure I can fix the source navigation though, those parts of the code looked very compiler-y to me and I might need some help there.

@jgneff
Copy link
Contributor

jgneff commented Nov 8, 2019

I'm fine with merging this now, I'll open another PR for the disappearing javadoc after attaching sources. Is there already an issue for this?

Thanks, Thomas. I'm fine with merging this now, too. I will create a new sub-task of NETBEANS-3296 for the problems I found in my tests. I'm still trying to keep all of these issues together under the one main goal of attaching and using the Javadoc and Sources of a library.

I'm not sure I can fix the source navigation though, those parts of the code looked very compiler-y to me and I might need some help there.

It looks as if once you add the Sources, they take precedence over the HTML for displaying the Javadoc (and fail). Maybe that precedence could be swapped, using the Sources only when the Javadoc is not available for a selected item. I have not yet found any documentation on exactly how this is supposed to work. Have you?

Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing the modification in RepositoryForBinaryQueryImpl.java and reading

I would prefer fixing the problems that occur after downloading the Sources: the Javadoc description disappears and the source navigation fails.

I get a bad feeling. This query is used in other places - is there an explanation for the observations?

@@ -0,0 +1,360 @@
<!DOCTYPE HTML>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a license header into this file.

@@ -0,0 +1,367 @@
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a license header into this file.

@@ -0,0 +1,322 @@
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a license header into this file.

@@ -0,0 +1,406 @@
<!DOCTYPE HTML>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a license header into this file.

@jgneff
Copy link
Contributor

jgneff commented Nov 8, 2019

This query is used in other places - is there an explanation for the observations?

If that question is for me, just follow along in my tests to the point where, after adding the Sources, the Javadoc description disappears for the show method. That doesn't happen in my Ant-based projects.

@matthiasblaesing
Copy link
Contributor

@jgneff your comment was my trigger. The question is directed to the author of this PR, the modified query has a larger userbase, than just the javadoc access, so potential breakage needs to be considered harder, than a local regression.

@zimmi
Copy link
Contributor Author

zimmi commented Nov 8, 2019

@matthiasblaesing I am not sure yet, I need more time to investigate. Can I tag you when I find the underlying reason?

I'm trying hard not to change existing code, but the logic used to find the javadoc is spread out and very stateful.

@zimmi
Copy link
Contributor Author

zimmi commented Nov 9, 2019

After some digging, here are my findings. The algorithm to find the javadoc goes as follows:

  1. If there is source for the code element attached, try to use that first (this happens in ElementJavadoc::getElementDoc).

  2. Otherwise, if the source contains no javadoc for the requested element or finding the requested code element in the source fails, try to use the javadoc.

  3. Otherwise, if the code element is a method, recurse upward the type hierarchy.

  4. Otherwise, display the "Attach Javadoc..." popup.

I think the order is sensible, changing it would probably be too disruptive.

I traced the execution after requesting javadoc for stage.setScene(scene) to ElementHandle::resolveImpl, where the method signature the user clicked on ({ "javafx.stage.Stage", "setScene", "(Ljavafx/scene/Scene;)V" }) does not match the signature parsed from Stage.java ({ "javafx.stage.Stage", "setScene", "(LScene;)V" }). The reason the parameter from the JavaFX source is not fully qualified is because javac reports it as TypeKind.ERROR, see ClassFileUtil.
In fact, javac reports every reference type from Stage.java as TypeKind.ERROR, even types like String that are not part of the JavaFX source. I haven't checked yet, but I assume this is also the reason why source navigation fails (notice that only jumping to methods that take reference types fails, others work fine).

About stage.show(): No reference type parameters, so it should work fine. It uses {@inheritDoc} though, and javac fails to resolve the superclass Window inside ElementJavadoc::getInherited (type.getSuperclass().getKind() returns TypeKind.ERROR). There is no other content in the javadoc of the method, so only the method signature is displayed.

That's where I stopped, the next step would probably be to see what's so special about Stage.java that javac can't parse its type references.

@matthiasblaesing
The javac errors are not caused by this PR: Renaming the source artifact in .m2 to javafx-graphics-13-linux-sources.jar (so NetBeans finds it) leads to the same failures in stock NetBeans 11.2: source navigation jumps to the top of the file and javadoc is only displayed for methods without reference type parameters. With this PR there is at least a fallback to javadoc jars when javac can't resolve type references.

I did not check why the Ant project works, but fixing the javac errors has to be done anyway.

While looking through the source I also found two other places that deal with javadoc that need to be adapted to javadoc 9+ output (namely HTMLJavadocParser::parseClass and JavadocHelper.TextStream). I'd like to consolidate the logic a bit, I think I have a way that doesn't break any public API. But leaving those places in for now shouldn't have a huge negative impact. I'm for merging this PR and I'll do a follow up PR when time permits.

@jgneff
Copy link
Contributor

jgneff commented Nov 10, 2019

Thank you for the great research, Thomas. Thanks especially for documenting how this is supposed to work according to the code. So it's the opposite of my guess—Sources first, then Javadoc. That makes sense, as many of the Javadoc comments are not published in their Javadoc HTML.

I also tried to figure out what's supposed to happen, but by comparing NetBeans with IntelliJ IDEA when they both appear to be working. I posted my comparison as Apache NetBeans & IntelliJ IDEA. I found the comparison interesting, but I don't think you'll see anything you don't already know.

@zimmi
Copy link
Contributor Author

zimmi commented Nov 10, 2019

The point about attached Javadoc substituting missing comments in the attached Source is subtle, great find! I revised the algorithm in my comment to reflect this. I verified that this is supposed to be happening with stage.setMinWidth(0) (no reference type parameter, so it's not accidental).

@jgneff
Copy link
Contributor

jgneff commented Nov 10, 2019

Thanks, Thomas. That's a good update to the algorithm description. There are quite a few subtleties in all of this. I added one more test to demonstrate the precedence for both IDEs and put the results in a new section called Precedence. That test finally shows an important difference between the two, and I prefer how NetBeans does it. I also added a section at the end called JavaFX library to show how I set up both IDEs to use the Javadoc and Sources.

The new sections are added to my Apache NetBeans & IntelliJ IDEA comparison.

@zimmi
Copy link
Contributor Author

zimmi commented Nov 15, 2019

Just for future reference, the algorithm doesn't work for the new @extLink taglet (the sources don't contain the hyperlinks, they are added to the Javadoc by the build process). To reproduce, open the Javadoc page of java.io:
java_io_javadoc

@jgneff
Copy link
Contributor

jgneff commented Nov 15, 2019

Also for future reference, the new @systemProperty tag in JDK 12 for system properties documentation is not recognized.

systemProperty-code

systemProperty-completion

Notice that all the keys are missing in the table below for System.getProperties().

getProperties

…ependencies with classifiers

OpenJFX uses Maven classifiers for platform dependent code, i.e. javafx-graphics-13.jar does not contain any code, but it depends through build profiles on javafx-graphics-13-linux.jar. There is however no javafx-graphics-13-linux-javadoc.jar, only a javafx-graphics-13-javadoc.jar, which NetBeans doesn't find. This patch lets NetBeans fall back to the main javadoc jar if there is none associated with the one with the classifier.

The contents of the javadoc jars of OpenJFX are prefixed by module name, which is different from the default output of the maven-javadoc-plugin. This patch does some bytecode reading to still find the correct .html file.

Fix RepositoryForBinaryQueryImplTest::testResultChanging which was broken because of a missing test resource.
@zimmi
Copy link
Contributor Author

zimmi commented Nov 17, 2019

@jgneff Would you mind testing it again? I rebased onto the master branch and now source navigation and @inheritDoc work for me. I'm using nb-javac, sources and Javadoc attached (although sources would probably be enough). Seems like a recent commit on the master branch fixed it.

@jgneff
Copy link
Contributor

jgneff commented Nov 18, 2019

@zimmi, sure! I should have time to test it early this week.

Next time, though, could you add incremental commits instead of a forced push? That would save time for anyone testing the pull request. See the JavaFX CONTRIBUTING guidelines for an example of a good general policy:

Please adhere to the general guideline that you should never force push to a publicly shared branch. Once you have opened your pull request, you should consider your branch publicly shared. Instead of force pushing you can just add incremental commits; this is generally easier on your reviewers. If you need to pick up changes from master, you can merge master into your branch.

@zimmi
Copy link
Contributor Author

zimmi commented Nov 18, 2019

You are right, sorry for that! I'll merge master in the future.

@neilcsmith-net
Copy link
Member

@jgneff that's actually different to NetBeans' own contribution guidelines! Such a change may not be a bad thing, but maybe something to discuss on dev@ rather than in a PR?!

@zimmi
Copy link
Contributor Author

zimmi commented Nov 20, 2019

I forgot to switch the Java Platform for the project from JDK 8 to JDK 13. As soon as I do that, it's broken again. Sorry for the false hope. I might have some time this week to look into the nb-javac error, maybe I can come up with something. This PR is still good to go as is from my perspective though, the errors existed previously as well.

@jgneff
Copy link
Contributor

jgneff commented Nov 20, 2019

Thanks for the update, Thomas. I'm all set up to test this morning, anyway, so I'll check for any changes without the nb-javac plug-in on JDK 13.0.1 and the early-access build of JavaFX 14-ea+2.

@jgneff
Copy link
Contributor

jgneff commented Nov 21, 2019

For these tests, I repeated my previous tests and the Apache NetBeans & IntelliJ IDEA comparison tests using an early-access build of JavaFX 14. I found two differences.

Previous tests

The first difference is due to using the early-access build of JavaFX 14 instead of the released version 13.0.1. With JavaFX 13.0.1, showing the documentation in an external Web browser opens the Javadoc page at the following location:

http://127.0.0.1:8082/resource/jar%3Afile%3A/home/john/.m2/repository/org/openjfx/javafx-graphics/13.0.1/javafx-graphics-13.0.1-javadoc.jar%21/javafx.graphics/javafx/stage/Stage.html#show%28%29

With the early-access build of JavaFX 14-ea+2, showing the documentation in an external Web browser opens an error page at the following location:

http://127.0.0.1:8082/resource/jar%3Afile%3A/home/john/.m2/repository/org/openjfx/javafx-graphics/14-ea%2B2/javafx-graphics-14-ea%2B2-javadoc.jar%21/javafx.graphics/javafx/stage/Stage.html#show%28%29

The error page contains:

Error: 500

Location: /resource/jar%3Afile%3A/home/john/.m2/repository/org/openjfx/javafx-graphics/14-ea%2B2/javafx-graphics-14-ea%2B2-javadoc.jar%21/javafx.graphics/javafx/stage/Stage.html

No detailed message

It seems to be a simple URL-encoding problem, but I couldn't find a different encoding of the plus sign that worked. I would suggest there's a problem in the JavaFX version string, except that it's part of the Semantic Versioning customs:

Build metadata MAY be denoted by appending a plus sign and a series of dot separated identifiers immediately following the patch or pre-release version.

Comparison tests

The second difference is compared to the working Ant-based tests. In Ant-based projects set up to work properly, the Javadoc navigation is the same whether you're selecting a method where it's used in an application, or you're selecting the same method where it's defined in a library.

In this Non-Modular Maven project, the toolbar button to open the Javadoc in an external Web browser is enabled when you select a method where it's used, but the button becomes disabled when you select the same method where it's defined. This occurs for both JavaFX 13.0.1 and JavaFX 14-ea+2.

Javadoc where used
javadoc-from-app

Javadoc where defined
javadoc-from-lib

This is not a new problem—just something I had not noticed in my earlier tests.

I'm fine with this pull request being merged. We can open new issues for the problems I found.

@zimmi, for what it matters, I'm fine now if you rewrite commits. I finally understand that NetBeans merges the pull request history directly into master as-is, so it really does matter what's there. My limited experience is with OpenJDK projects, which forbid forced pushes, discard the pull request history, and add only a single fast-forward commit to master. I assumed that all projects worked in a similar manner, so I couldn't understand why an author would even bother to tidy up the pull request history. I think I understand better now, and I'm sorry for my ignorant assumptions.

@zimmi
Copy link
Contributor Author

zimmi commented Nov 21, 2019

Thank you for testing it again, John! There are many small things to improve, it's great to have them all written down. I also have two smaller things regarding the toolbar button:

Spending a bit more time in the debugger, I found that replacing JavaSource js = JavaSource.forFileObject(fo) with JavaSource js = JavaSource.create(cpInfo, fo) in ElementJavadoc::getElementDocFromSource allows nb-javac to find the parent class of Stage and correctly render the @inheritDoc for stage.show(). cpInfo seems to represent the classpath of the current project, whereas JavaSource::forFileObject creates a more targeted classpath here. Doing this breaks the Javadoc for JDK classes though, so some more investigation of the differences is needed.

@jgneff
Copy link
Contributor

jgneff commented Nov 22, 2019

cpInfo seems to represent the classpath of the current project, whereas JavaSource::forFileObject creates a more targeted classpath here.

Could that be the reason {@inheritDoc} works so well when I list individual JAR files and their Sources directly on the Classpath in an Ant project?

compile-time-libraries javadoc-stage-show

@ebarboni
Copy link
Contributor

Hi, @neilcsmith-net and @matthiasblaesing. I'm not sure of the status for this PR, is this mergeable ? need feedback or still work in progress ?

@neilcsmith-net
Copy link
Member

@ebarboni personally don't know - although looks like @lkishalmi has tested and recommends merging? Good to get in in principle - it's affecting me on a project at the moment!

@lkishalmi
Copy link
Contributor

I've only checked this PR at the early stage, looked good and in fact it is not being merged blocks a few of my outstanding fixes for Gradle vs JavaFx documentation. I'm kind of +0 merging it at this phase of the release.
I see JavaFx is getting popular again with NetBeans, and if we add a bit of emphasis testing the javadoc functionality, I think we can eventually merge this into 11.3beta2

Copy link
Contributor

@lkishalmi lkishalmi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+0 as of the timing.

@jgneff
Copy link
Contributor

jgneff commented Jan 30, 2020

I didn't do a code review, but I did test the fix and document the results rather thoroughly two times—on Nov 7, 2019 and again on Nov 21, 2019. Based on the test results, I recommended that it be merged at the time.

It's easy to follow along and repeat my tests, if anyone wants to test it on the latest NetBeans. I also created a repository comparing NetBeans with the competition on this feature. NetBeans wins the comparison, in my opinion, but only if these bugs can get out of the way.

So for me, the sooner this is merged, the better! That will make it one down, ten more to go.

@ebarboni
Copy link
Contributor

going to merge.

@ebarboni ebarboni merged commit 6fbdb7c into apache:master Jan 30, 2020
@zimmi
Copy link
Contributor Author

zimmi commented Jan 30, 2020

Thank you for merging! I got a new job and didn't have much time to dedicate to this. I might come back to it when time permits, there are still some improvements / fixes left for full JavaFX support with Maven.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants