Fix adding preview features to a visible project #1863

snjeza · 2021-08-30T19:30:55Z

Fixes redhat-developer/vscode-java#1971

Signed-off-by: Snjezana Peco snjezana.peco@redhat.com

rgrunber · 2021-09-13T19:53:05Z

The .vsix in the issue works for me, but not this PR. Specifically, if I monitor the .settings/org.eclipse.jdt.core.prefs in the real project (not the invisible link), it never reverts away from enabling+ignoring the preview features.

One thing I noted was that being able to reproduce the issue depended on me having the Java source file opened while the build was happening (since this triggers the invisible project logic on the syntax server).

snjeza · 2021-09-13T20:34:26Z

The .vsix in the issue works for me, but not this PR.

You have to create .vsix.

snjeza · 2021-09-13T21:20:32Z

test this please

rgrunber · 2021-09-14T01:02:08Z

I'm still observing the same behaviour. https://github.com/snjeza/vscode-test/raw/master/java-0.82.4.vsix fixes the issue but If I build this PR with npm run build-server + vsce package with JDT-LS set on this change, the preview settings never get updated.

snjeza · 2021-09-14T14:46:33Z

but If I build this PR with npm run build-server + vsce package with JDT-LS set on this change, the preview settings never get updated.

@rgrunber Could you, please, try now.

rgrunber · 2021-09-14T14:47:03Z

I'll try this out again. I was also trying the non-rebased version (8180a65).

rgrunber · 2021-09-14T15:55:46Z

So If I'm understanding the logic correctly :

For every project treated as an invisible project (seems to happen on the syntax server side), we save the original enable+report options under the org.eclipse.jdt.ls.core node (not the org.eclipse.jdt.core one). We overwrite the project options as before because we simply don't know if/when we switch to standard mode.
Once the standard server takes over, we look at every non-invisible project (they should have been imported by now) and try to access those original org.eclipse.jdt.ls.core options. If they exist, we know we must have overridden the original options, and so we need to restore them.
The org.eclipse.jdt.ls.core node gets cleared so the same approach can happen again for the next time the project is treated as an invisible project.

snjeza · 2021-09-14T16:08:09Z

@rgrunber You are right.

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/ProjectsManager.java

rgrunber · 2021-09-14T19:15:03Z

Last thing I can currently think of : Is there any way to avoid forcing enable+report preview settings for the lightweight mode ? It doesn't seem like anything useful happens with those settings in that case. I was thinking that might be simpler than what we're doing here.

Update : If not, I don't see any other way currently to prevent this issue other than what @snjeza is proposing here.

snjeza · 2021-09-14T19:43:36Z

Last thing I can currently think of : Is there any way to avoid forcing enable+report preview settings for the lightweight mode ?

cc @fbricon

fbricon · 2021-09-21T07:54:52Z

I think we're following the wrong approach here. Basically, the source of the issue is: invisible projects are not properly identified, so the wrong projects are mutated. Current solution is way too complex for my taste..
What I think would work much better (and simpler) is this:

add a invisible project nature to the actual invisible projects
detect invisible projects by their nature.
that's it. No need to change the existing logic of JVM changes

snjeza · 2021-09-21T16:58:14Z

The issue happens when Java LS starts in the following way:

starts the Syntax server
the Syntax server creates an invisible project X
the Syntax server links the .settings directory and updates the preview property. See 733e4e1
the Syntax server exits
Java LS starts the Standard server
the Standard server import the X project with the updated preview property

The PR saves changes made by the Syntax server and restores them when starting the Standard server.

add a invisible project nature to the actual invisible projects
detect invisible projects by their nature.

Is there a way to know if the Syntax server has changed the preview property?

fbricon · 2021-09-21T17:28:06Z

instead of linking .settings/ in syntax mode, can we instead create a physical copy (overwrite if necessary) of the original files into the actual invisible project location?
It might no be possible, I don't remember the details of the latest implem.

snjeza · 2021-09-21T17:56:18Z

instead of linking .settings/ in syntax mode, can we instead create a physical copy (overwrite if necessary) of the original files into the actual invisible project location?
It might no be possible, I don't remember the details of the latest implem.

@fbricon you may want to take a look at redhat-developer/vscode-java#1657 (comment)

fbricon · 2021-09-21T21:23:22Z

ok so how about: if invisible project and ./settings is linked, then do not update the JVM settings

snjeza · 2021-09-21T21:54:42Z

ok so how about: if invisible project and ./settings is linked, then do not update the JVM settings

It is doable.

snjeza · 2021-09-22T01:26:45Z

@fbricon @rgrunber I have updated the PR.

fbricon · 2021-09-22T08:40:12Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/ProjectUtils.java

+						// reset the preview feature property - https://github.com/eclipse/eclipse.jdt.ls/pull/1863#issuecomment-924395431
+						Hashtable<String, String> defaultOptions = JavaCore.getDefaultOptions();
+						String defaultPreview = defaultOptions.get(JavaCore.COMPILER_PB_ENABLE_PREVIEW_FEATURES);
+						String defaultReport = defaultOptions.get(JavaCore.COMPILER_PB_REPORT_PREVIEW_FEATURES);
+						IJavaProject javaProject = JavaCore.create(invisibleProject);
+						javaProject.setOption(JavaCore.COMPILER_PB_ENABLE_PREVIEW_FEATURES, defaultPreview);
+						javaProject.setOption(JavaCore.COMPILER_PB_REPORT_PREVIEW_FEATURES, defaultReport);


I don't understand why is this necessary here?

The Syntax server calls JVMConfigurator.configureJVMSettings before configuring linked settings. See https://github.com/eclipse/eclipse.jdt.ls/blob/master/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/ProjectsManager.java#L319
You can try to run the InvisibleProjectImporter.testPreviewFeaturesSettingsDisabled test without it.

...jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/managers/InvisibleProjectImporterTest.java

fbricon · 2021-09-24T14:34:24Z

...jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/managers/InvisibleProjectImporterTest.java

@@ -217,7 +250,7 @@ public void testPreviewFeaturesDisabledForNotLatestJDK() throws Exception {
 			assertTrue(invisibleProject.exists());
 			assertNoErrors(invisibleProject);
 			IJavaProject javaProject = JavaCore.create(invisibleProject);
-			assertEquals(JavaCore.DISABLED, javaProject.getOption(JavaCore.COMPILER_PB_ENABLE_PREVIEW_FEATURES, false));
+			assertEquals(JavaCore.DISABLED, javaProject.getOption(JavaCore.COMPILER_PB_ENABLE_PREVIEW_FEATURES, true));


why switching from false to true?

It actually does fail with false because the result will be null. Interestingly, javaProject.getOptions(false).get(JavaCore.COMPILER_PB_ENABLE_PREVIEW_FEATURES) does return disabled though.

Weird.. The Javadoc mentions that getOption(String, boolean) is a Helper method for returning one option value only. Equivalent to (String)this.getOptions(inheritJavaCoreOptions).get(optionName) :\

Update : The inconsistency happens when the project .settings folder gets linked to the invisible one. If you insert a breakpoint at settingsLinkFolder.createLink(..) in createInvisibleProjectIfNotExist you can actually see the settings we just applied for the preview feature/reporting go to null.

Do you have any suggestions?

Actually, this behaviour only happens in the testcase, and not in the welcome-app example, so maybe it's related to how we set up the tests.

Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>

snjeza · 2021-09-25T18:34:44Z

@fbricon @rgrunber I have updated the PR.

rgrunber

I think we should merge this as it solves the issue.

We're basically setting the preview options to enable/report only when we don't detect that a project has its own .settings folder, and we undo the one case where enable/report get set prior to having set up the .settings folder.

rgrunber · 2021-10-19T01:35:26Z

...jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/managers/InvisibleProjectImporterTest.java

@@ -217,7 +250,7 @@ public void testPreviewFeaturesDisabledForNotLatestJDK() throws Exception {
 			assertTrue(invisibleProject.exists());
 			assertNoErrors(invisibleProject);
 			IJavaProject javaProject = JavaCore.create(invisibleProject);
-			assertEquals(JavaCore.DISABLED, javaProject.getOption(JavaCore.COMPILER_PB_ENABLE_PREVIEW_FEATURES, false));
+			assertEquals(JavaCore.DISABLED, javaProject.getOption(JavaCore.COMPILER_PB_ENABLE_PREVIEW_FEATURES, true));


Actually, this behaviour only happens in the testcase, and not in the welcome-app example, so maybe it's related to how we set up the tests.

snjeza requested review from gorkem, fbricon, rgrunber and testforstephen August 30, 2021 19:30

snjeza force-pushed the issue-1971 branch from 8180a65 to eeda05c Compare September 13, 2021 20:56

snjeza added the in progress label Sep 14, 2021

snjeza force-pushed the issue-1971 branch from eeda05c to ed2539f Compare September 14, 2021 14:39

rgrunber reviewed Sep 14, 2021

View reviewed changes

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/ProjectsManager.java Outdated Show resolved Hide resolved

rgrunber modified the milestone: Mid September 2021 Sep 15, 2021

snjeza force-pushed the issue-1971 branch from ed2539f to 51ec010 Compare September 22, 2021 01:14

fbricon reviewed Sep 22, 2021

View reviewed changes

snjeza removed the in progress label Sep 24, 2021

fbricon reviewed Sep 24, 2021

View reviewed changes

...jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/managers/InvisibleProjectImporterTest.java Outdated Show resolved Hide resolved

fbricon reviewed Sep 24, 2021

View reviewed changes

Fix adding preview features to a visible project

333e04a

Signed-off-by: Snjezana Peco <snjezana.peco@redhat.com>

snjeza force-pushed the issue-1971 branch from 51ec010 to 333e04a Compare September 25, 2021 16:51

rgrunber approved these changes Oct 19, 2021

View reviewed changes

rgrunber merged commit 5eff580 into eclipse-jdtls:master Oct 19, 2021

rgrunber added this to the Mid October 2021 milestone Oct 19, 2021

testforstephen mentioned this pull request Oct 19, 2021

Add Changelog for 1.5.0 #1914

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix adding preview features to a visible project #1863

Fix adding preview features to a visible project #1863

snjeza commented Aug 30, 2021

rgrunber commented Sep 13, 2021 •

edited

Loading

snjeza commented Sep 13, 2021

snjeza commented Sep 13, 2021

rgrunber commented Sep 14, 2021

snjeza commented Sep 14, 2021

rgrunber commented Sep 14, 2021

rgrunber commented Sep 14, 2021 •

edited

Loading

snjeza commented Sep 14, 2021

rgrunber commented Sep 14, 2021 •

edited

Loading

snjeza commented Sep 14, 2021

fbricon commented Sep 21, 2021

snjeza commented Sep 21, 2021

fbricon commented Sep 21, 2021

snjeza commented Sep 21, 2021 •

edited

Loading

fbricon commented Sep 21, 2021

snjeza commented Sep 21, 2021 •

edited

Loading

snjeza commented Sep 22, 2021

fbricon Sep 22, 2021

snjeza Sep 22, 2021

fbricon Sep 24, 2021

rgrunber Sep 25, 2021 •

edited

Loading

snjeza Sep 25, 2021

rgrunber Oct 19, 2021

snjeza commented Sep 25, 2021

rgrunber left a comment

rgrunber Oct 19, 2021

Fix adding preview features to a visible project #1863

Fix adding preview features to a visible project #1863

Conversation

snjeza commented Aug 30, 2021

rgrunber commented Sep 13, 2021 • edited Loading

snjeza commented Sep 13, 2021

snjeza commented Sep 13, 2021

rgrunber commented Sep 14, 2021

snjeza commented Sep 14, 2021

rgrunber commented Sep 14, 2021

rgrunber commented Sep 14, 2021 • edited Loading

snjeza commented Sep 14, 2021

rgrunber commented Sep 14, 2021 • edited Loading

snjeza commented Sep 14, 2021

fbricon commented Sep 21, 2021

snjeza commented Sep 21, 2021

fbricon commented Sep 21, 2021

snjeza commented Sep 21, 2021 • edited Loading

fbricon commented Sep 21, 2021

snjeza commented Sep 21, 2021 • edited Loading

snjeza commented Sep 22, 2021

fbricon Sep 22, 2021

Choose a reason for hiding this comment

snjeza Sep 22, 2021

Choose a reason for hiding this comment

fbricon Sep 24, 2021

Choose a reason for hiding this comment

rgrunber Sep 25, 2021 • edited Loading

Choose a reason for hiding this comment

snjeza Sep 25, 2021

Choose a reason for hiding this comment

rgrunber Oct 19, 2021

Choose a reason for hiding this comment

snjeza commented Sep 25, 2021

rgrunber left a comment

Choose a reason for hiding this comment

rgrunber Oct 19, 2021

Choose a reason for hiding this comment

rgrunber commented Sep 13, 2021 •

edited

Loading

rgrunber commented Sep 14, 2021 •

edited

Loading

rgrunber commented Sep 14, 2021 •

edited

Loading

snjeza commented Sep 21, 2021 •

edited

Loading

snjeza commented Sep 21, 2021 •

edited

Loading

rgrunber Sep 25, 2021 •

edited

Loading