Extract embedded javadoc images #1138

NikolasKomonen · 2019-08-01T21:02:39Z

The purpose of this PR is to enable javadoc hover to work with html tag images that are embedded inside a jar. This does not currently work because the language server receives a relative path to the jar that it does not know how to resolve.

This PR will extract the image into the jdtls plugin folder and replace the html img tag 'src' attribute with the new path.

Image extraction works, regular paths work too.

No tests.

Need to figure out how to get path of source jar.

Fixes redhat-developer/vscode-java#1007

eclipse-ls-bot · 2019-08-01T21:02:42Z

Can one of the admins verify this patch?

fbricon · 2019-08-01T21:04:54Z

add to whitelist

NikolasKomonen · 2019-08-01T21:07:19Z

@fbricon
@snjeza I was told you'd be able to help. I'm trying to get access to the jar files.

This is how I did it for the javadoc jar https://github.com/eclipse/eclipse.jdt.ls/pull/1138/files#diff-b3a48af05d56183bee79e67754caeb7aR136

Am I on the right track, and how would I do it for the source jar? Thanks.

NikolasKomonen · 2019-08-01T21:09:44Z

@fbricon

add to whitelist

I removed it from the whitelist to allow for absolute local paths to work as well, but I realised that case won't happen. Will revert it.

Also still need to handle some URI/OS specific path things. You don't have to check those things yet.

fbricon · 2019-08-01T21:32:27Z

@NikolasKomonen "add to whitelist" is a command to add you to Jenkins' whitelist, so that your PRs are automatically built ;-)

snjeza · 2019-08-01T22:08:40Z

test this please

snjeza

I have tested https://github.com/reactor/reactor-core and get the following error:

[Error - 12:15:21 AM] Aug 2, 2019 12:15:21 AM Error computing hover
null
java.lang.NullPointerException
	at java.io.File.<init>(File.java:277)
	at java.util.jar.JarFile.<init>(JarFile.java:103)
	at org.eclipse.jdt.ls.core.internal.javadoc.JavaDocHTMLPathHandler.getValidatedHTMLSrcAttribute(JavaDocHTMLPathHandler.java:149)
	at org.eclipse.jdt.ls.core.internal.javadoc.JavadocContentAccess2.handleContentElements(JavadocContentAccess2.java:1465)
	at org.eclipse.jdt.ls.core.internal.javadoc.JavadocContentAccess2.handleContentElements(JavadocContentAccess2.java:1436)
	at org.eclipse.jdt.ls.core.internal.javadoc.JavadocContentAccess2.elementToHTML(JavadocContentAccess2.java:1095)
	at org.eclipse.jdt.ls.core.internal.javadoc.JavadocContentAccess2.toHTML(JavadocContentAccess2.java:934)
	at org.eclipse.jdt.ls.core.internal.javadoc.JavadocContentAccess2.javadoc2HTML(JavadocContentAccess2.java:845)
	at org.eclipse.jdt.ls.core.internal.javadoc.JavadocContentAccess2.getHTMLContentFromSource(JavadocContentAccess2.java:729)
	at org.eclipse.jdt.ls.core.internal.javadoc.JavadocContentAccess2.getHTMLContent(JavadocContentAccess2.java:587)
	at org.eclipse.jdt.ls.core.internal.javadoc.JavadocContentAccess2.getMarkdownContentReader(JavadocContentAccess2.java:2498)
	at org.eclipse.jdt.ls.core.internal.HoverInfoProvider.computeJavadoc(HoverInfoProvider.java:252)
	at org.eclipse.jdt.ls.core.internal.HoverInfoProvider.computeHover(HoverInfoProvider.java:133)
	at org.eclipse.jdt.ls.core.internal.handlers.HoverHandler.computeHover(HoverHandler.java:50)
	at org.eclipse.jdt.ls.core.internal.handlers.HoverHandler.hover(HoverHandler.java:39)
	at org.eclipse.jdt.ls.core.internal.handlers.JDTLanguageServer.lambda$6(JDTLanguageServer.java:514)
	at org.eclipse.jdt.ls.core.internal.handlers.JDTLanguageServer.lambda$41(JDTLanguageServer.java:909)
	at java.util.concurrent.CompletableFuture.uniApply(CompletableFuture.java:602)
	at java.util.concurrent.CompletableFuture$UniApply.tryFire(CompletableFuture.java:577)
	at java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:443)
	at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
	at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
	at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
	at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)

@NikolasKomonen You can explore org.eclipse.jdt.ui.StandardJavaElementContentProvider

NikolasKomonen · 2019-08-02T02:55:51Z

@fbricon

"add to whitelist" is a command to add you to Jenkins' whitelist

oh haha, guess theres more than one

@snjeza

Thanks for the info, I'll look into it.
Also this pr is not fully done yet but I will have tests in the actual PR

fbricon · 2019-08-09T16:20:27Z

@NikolasKomonen test failure in https://ci.eclipse.org/ls/job/jdt-ls-pr/1551/testReport/org.eclipse.jdt.ls.core.internal.javadoc/JavaDocImageExtractionTest/testImageExtraction/

launch/jdt.ls.remote.server.launch

snjeza · 2019-08-09T18:27:57Z

test failure in https://ci.eclipse.org/ls/job/jdt-ls-pr/1551/testReport/org.eclipse.jdt.ls.core.internal.javadoc/JavaDocImageExtractionTest/testImageExtraction/

@NikolasKomonen you can try the following:

diff --git a/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/javadoc/JavaDocImageExtractionTest.java b/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/javadoc/JavaDocImageExtractionTest.java
index 112275bc..735845b7 100644
--- a/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/javadoc/JavaDocImageExtractionTest.java
+++ b/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/javadoc/JavaDocImageExtractionTest.java
@@ -20,11 +20,18 @@ import org.eclipse.core.resources.IProject;
 import org.eclipse.core.runtime.IPath;
 import org.eclipse.core.runtime.NullProgressMonitor;
 import org.eclipse.core.runtime.Platform;
+import org.eclipse.jdt.core.IClassFile;
 import org.eclipse.jdt.core.ICompilationUnit;
 import org.eclipse.jdt.core.IJavaElement;
+import org.eclipse.jdt.core.IJavaProject;
+import org.eclipse.jdt.core.IType;
+import org.eclipse.jdt.core.JavaCore;
+import org.eclipse.jdt.internal.core.BinaryType;
 import org.eclipse.jdt.ls.core.internal.HoverInfoProvider;
 import org.eclipse.jdt.ls.core.internal.IConstants;
 import org.eclipse.jdt.ls.core.internal.JDTUtils;
+import org.eclipse.jdt.ls.core.internal.JobHelpers;
+import org.eclipse.jdt.ls.core.internal.SourceContentProvider;
 import org.eclipse.jdt.ls.core.internal.WorkspaceHelper;
 import org.eclipse.jdt.ls.core.internal.managers.AbstractMavenBasedTest;
 import org.eclipse.lsp4j.MarkedString;
@@ -56,6 +63,14 @@ public class JavaDocImageExtractionTest extends AbstractMavenBasedTest {
        public void setup() throws Exception {
                importProjects("maven/javadoctest");
                project = WorkspaceHelper.getProject("javadoctest");
+               IJavaProject javaProject = JavaCore.create(project);
+               IType type = javaProject.findType("reactor.core.publisher.Mono");
+               IClassFile classFile = ((BinaryType) type).getClassFile();
+               String source = new SourceContentProvider().getSource(classFile, new NullProgressMonitor());
+               if (source == null) {
+                       JobHelpers.waitForDownloadSourcesJobs(JobHelpers.MAX_TIME_MILLIS);
+                       source = new SourceContentProvider().getSource(classFile, new NullProgressMonitor());
+               }
        }
 
        @Test

See https://github.com/eclipse/eclipse.jdt.ls/blob/master/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/managers/MavenBuildSupportTest.java#L162

snjeza · 2019-08-09T18:33:57Z

@NikolasKomonen you should also test the source package fragments:

git clone git@github.com:reactor/reactor-core.git
cd reactor-core
code .
# open Mono.java
# hover over Mono.error(Throwable error)

You will get an exception.

NikolasKomonen · 2019-08-15T13:37:05Z

@snjeza I've fixed the issues with this PR. Would you be able to check this when you have time?

...eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/javadoc/JavaDocHTMLPathHandler.java

NikolasKomonen · 2019-08-23T20:41:41Z

@fbricon Updated and tested on Windows as well.

fbricon · 2019-09-09T15:10:47Z

Still doesn't work, testing io.projectreactor:reactor-core:3.2.10.RELEASE or the latest one. It seems the parsing is thrown off by a leading <p> before the <img` tag.

fbricon · 2019-09-10T17:09:25Z

still doesn't work on my mac. It used to work, in your previous patches. Might be some change in the way javadoc is computed/sent to the new code, which breaks it

fbricon · 2019-09-11T11:03:20Z

doesn't work on my linux box either. Tested with a bunch of reactor-core versions.
@snjeza does it work for you?

snjeza · 2019-09-11T11:49:45Z

@snjeza does it work for you?

No, it doesn't.

fbricon · 2019-09-11T18:06:50Z

@NikolasKomonen so it works when hovering over Mono.error for instance, but not when hovering over Mono. As I've shown on the previous image, this is caused by the javadoc elements being split differently. eg. If the node under consideration starts with <img, it works, else if you have <p><img, it doesn't.

...eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/javadoc/JavaDocHTMLPathHandler.java

snjeza · 2019-09-12T16:04:28Z

I have faced the following issue:

Not allowed to load local resource: file:///home/snjeza/runtime-JDT-LS/.metadata/.plugins/org.eclipse.jdt.ls.core/extracted-jar-images/reactor-core-3.2.11.RELEASE/mono.svg

See microsoft/vscode#45260 (comment)
"Disable: Allow all content and script execution. Not Recommended" doesn't help.

NikolasKomonen · 2019-09-16T15:12:42Z

@snjeza I'm not able to re-create your issue. Was it always happening or did it appear randomly?

Also, are you running the most recent (August) Vscode version?

snjeza · 2019-09-16T16:12:07Z

I'm not able to re-create your issue. Was it always happening or did it appear randomly?

NikolasKomonen · 2019-09-16T16:57:23Z

@snjeza Ok I see the issue and can recreate it, but are you having the same issue with loading the image on hover? It could be that the permissions are different for language server hovers compared to using the mardown preview feature.

fbricon · 2019-09-16T17:18:42Z

I have faced the following issue:

Not allowed to load local resource: file:///home/snjeza/runtime-JDT-LS/.metadata/.plugins/org.eclipse.jdt.ls.core/extracted-jar-images/reactor-core-3.2.11.RELEASE/mono.svg

I can't reproduce that, I'm using the latest VS Code insiders.

I still see the problem of the missing image when hovering on Mono though

fbricon · 2019-09-16T17:22:41Z

oh but I can reproduce it in regular vscode.

snjeza · 2019-09-16T17:26:26Z

Ok I see the issue and can recreate it, but are you having the same issue with loading the image on hover?

~~Yes, I am.~~

It works now.

snjeza · 2019-09-16T17:43:20Z

I can't reproduce that, I'm using the latest VS Code insiders.

...eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/javadoc/JavaDocHTMLPathHandler.java

This PR solves the issue when the image linked in the javadoc points to a location inside the jar. Fixes eclipse-jdtls#1007 Signed-off-by: Nikolas Komonen <nikolaskomonen@gmail.com>

fbricon · 2019-09-17T07:53:35Z

Thanks @NikolasKomonen !

fbricon requested a review from snjeza August 1, 2019 21:07

NikolasKomonen force-pushed the javadocImageFix branch from 1a04501 to de255b3 Compare August 1, 2019 21:19

snjeza requested changes Aug 1, 2019

View reviewed changes

NikolasKomonen force-pushed the javadocImageFix branch 2 times, most recently from d832f45 to b36522e Compare August 8, 2019 20:29

fbricon reviewed Aug 9, 2019

View reviewed changes

launch/jdt.ls.remote.server.launch Outdated Show resolved Hide resolved

NikolasKomonen force-pushed the javadocImageFix branch 5 times, most recently from 08cf8ce to 7d6b2ab Compare August 14, 2019 19:21

NikolasKomonen marked this pull request as ready for review August 14, 2019 19:44

snjeza approved these changes Aug 15, 2019

View reviewed changes

fbricon reviewed Aug 20, 2019

View reviewed changes