Add commands to add/remove/list the project's source path #859

testforstephen · 2018-11-20T11:20:54Z

Signed-off-by: Jinbo Wang [email protected]

This is for feature redhat-developer/vscode-java#619, marking a folder as source root is helpful for the standalone java file support and simple eclipse project. Currently i disabled the operation on maven/gradle project because the user could manage this kind of source path through pom.xml and build.gradle.

Below is the basic feature:

Add Folder to Java Source Path (context menu)
Remove Folder from Java Source Path (context menu)
List all Java source paths (command palette)

See the peer PR redhat-developer/vscode-java#724

fbricon · 2018-11-30T06:25:38Z

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

+		return projects;
+	}
+
+	public static IProject createInvisibleProjectIfNotExist(IPath workspaceRoot) throws OperationCanceledException, CoreException {


so this partly depends on #843?

Share the same InvisibleProject concept for the workspace without project descriptor. But the code doesn't have dependency because this is just a common utility method.

fbricon · 2018-11-30T06:27:40Z

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

+		for (IClasspathEntry entry : existingEntries) {
+			if (entry.getEntryKind() == IClasspathEntry.CPE_SOURCE) {
+				if (entry.getPath().equals(sourcePath)) {
+					return;


should return false, as nothing was added

fbricon · 2018-11-30T06:27:55Z

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

@@ -83,4 +95,180 @@ public static String getJavaSourceLevel(IProject project) {
 	public static IProject[] getAllProjects() {
 		return ResourcesPlugin.getWorkspace().getRoot().getProjects();
 	}
+
+	public static void addSourcePath(IPath sourcePath, IJavaProject project) throws CoreException {


return boolean

fbricon · 2018-11-30T06:28:34Z

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

+		project.setRawClasspath(newEntries, project.getOutputLocation(), null);
+	}
+
+	public static void removeSourcePath(IPath sourcePath, IJavaProject project) throws CoreException {


return boolean. true if actually deleted, else false

testforstephen · 2018-11-30T08:15:19Z

test this please

fbricon · 2018-11-30T19:42:11Z

@tsmaeder @svor @tolusha before we merge that to jdt.ls, I'd like one of you guys to review it as it overlaps with some Che/jdt.ls functionality.

fbricon · 2018-11-30T19:48:53Z

Adding the same source multiple times keeps sending the "Successfully added" message

fbricon · 2018-11-30T19:58:15Z

create A,java file in source folder
create newfolder, add as source folder
create B.java file in new source folder
make A extend B
remove newfolder from source path

No compilation error is raised on A.java, until A.java is reopened. Compilation errors should appear as soon as the newsource folder has been removed

testforstephen · 2018-12-03T05:38:02Z

Cannot reproduce the scenario above. After the step remove newfolder from source path, WorkspaceDiagnistoicsHandler is able to detect the resource change event, and republish a diagnostic on the A.java.

Can reproduce the scenario under redhat-developer/vscode-java#724 (comment)

add source folder
add new class in new source folder
generate a compilation error (missing type), new diagnostic is added
remove source folder
diagnostic is not purged

The fix is to clear the diagnostics for the resource not on the classpath.

testforstephen · 2018-12-06T08:25:01Z

@fbricon After merge with the upstream master branch, i can reproduce scenario above. Looks like the new change 8ee5455 will skip the diagnostics republish for the A.java for build path change event.

Comment the if (!cu.isWorkingCopy()) statement under public boolean visit(IResourceDelta delta) throws CoreException, the diagnostic status can be reported correctly with add/remove build path.

tsmaeder · 2018-12-10T14:49:24Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/commands/BuildPathCommand.java

+	private static final String UNSUPPORTED_ON_MAVEN = "Unsupported operation. Please use pom.xml file to manage the source directories of maven project.";
+	private static final String UNSUPPORTED_ON_GRADLE = "Unsupported operation. Please use build.gradle file to manage the source directories of gradle project.";
+
+	public static CommandResult addToSourcePath(String sourceFolder) {


What is the format of "sourceFolder"? Why is this not a URI?

Yes, URI is better. Fixed.

tsmaeder · 2018-12-10T14:52:33Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/commands/BuildPathCommand.java

+	public static CommandResult addToSourcePath(String sourceFolder) {
+		IPath sourceFolderPath = ResourceUtils.filePathFromURI(Paths.get(sourceFolder).toUri().toString());
+		IProject targetProject = findBelongedProject(sourceFolderPath);
+		if (targetProject != null && (ProjectUtils.isMavenProject(targetProject.getProject()) || ProjectUtils.isGradleProject(targetProject.getProject()))) {


What we really want to know here: is this a simple Eclipse project? Can we add that method to ProjectUtils? If we ever decide to clean project support up (because importers are contributions, reall, no?) we can fix it in one place.

tsmaeder · 2018-12-10T15:26:56Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/commands/BuildPathCommand.java

+		return path;
+	}
+
+	public static class CommandResult {


If you're using a generic name, why not must "Result"?

tsmaeder · 2018-12-10T15:27:26Z

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/commands/BuildPathCommand.java

+			this.message = message;
+		}
+
+		CommandResult(boolean status, String message, Object data) {


If the result holds data of different types, we should have different result classes.

tsmaeder · 2018-12-10T15:29:53Z

...e.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/WorkspaceDiagnosticsHandler.java

@@ -127,6 +128,12 @@ public boolean visit(IResourceDelta delta) throws CoreException {
 		// Check if it is a Java ...
 		if (JavaCore.isJavaLikeFileName(file.getName())) {
 			ICompilationUnit cu = (ICompilationUnit) JavaCore.create(file);
+			// Clear the diagnostics for the resource not on the classpath


Wouldn't it make more sense go listen for classpath changes on the java model and remove diagnostics when a folder is removed from the classpath?

@tsmaeder From the classpath change event, we only know which project and which folder is affected. It's hard to infer which java files need refresh its diagnostics info. Adding or removing a source folder from classpath, it may cause compilation errors for java files under other source folders to be outdated too.

Here i just leverage the auto-build mechanism to tell us the problematic java files, and republish its diagnostics, because adding/removing source path operation will trigger the auto-build job so that the WorkspaceDiagnosticsHandler has a chance to republish the diagnostics.

I'm interested in the idea "listen for classpath changes on the java model" you mentioned, but not sure how to implement that, do you have more info? Thanks.

You can start from JavaCore#addElementChangedListener() to learn about Java model deltas.

@tsmaeder Thanks for the info. Have a try on JavaCore#addElementChangedListener().
Considering the scenario below:

create A,java file in source folder create newfolder, add as source folder create B.java file in new source folder make A extend B remove newfolder from source path

Below is the execution flow:

Invoke the API JavaProject.setRawClasspath() to update the project's classpath. -> IElementChangedListener receives .classpath Resource Delta (Remove From Classpath Event). -> AutoBuildJob is triggered automatically. -> IElementChangedListener receives the changed model. (Include A.java). -> WorkspaceDiagnosticsHandler (because it implements IResourceChangeListener) will also receive the changed resource. (Include A.java).

Both listeners do the same thing. Their handle logic are similar, adding a new listener will cause duplicated logic to handle the same work.

So i prefer to use WorkspaceDiagnosticsHandler to refresh diagnostics.

testforstephen · 2018-12-13T13:55:54Z

@tsmaeder @fbricon is it ok with the implementation about diagnostics refresh?

testforstephen · 2018-12-17T03:35:04Z

@fbricon is it possible to include this feature in Mid December release?

fbricon

We might find we need to iterate more over the design of this feature, but for now it does its job as advertised, AFAICS, so I'm ok to merge this as a provisional feature.

@testforstephen please squash and rebase your PR into 1 commit. Once this is done, @yaohaizh can merge it.

Signed-off-by: Jinbo Wang <[email protected]>

fbricon reviewed Nov 30, 2018

View reviewed changes

fbricon added this to the Mid December 2018 milestone Nov 30, 2018

tsmaeder reviewed Dec 10, 2018

View reviewed changes

fbricon approved these changes Dec 17, 2018

View reviewed changes

Add commands to add/remove/list the project's source path

d37d038

Signed-off-by: Jinbo Wang <[email protected]>

testforstephen force-pushed the jinbo_buildpath branch from 4d1154c to d37d038 Compare December 17, 2018 05:37

yaohaizh merged commit d91fdc4 into eclipse-jdtls:master Dec 17, 2018

testforstephen deleted the jinbo_buildpath branch December 17, 2018 05:51

Poytr1 mentioned this pull request Mar 19, 2019

java.project.addToSourcePath command should be allowed in gradle/maven project #964

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add commands to add/remove/list the project's source path #859

Add commands to add/remove/list the project's source path #859

testforstephen commented Nov 20, 2018 •

edited

Loading

fbricon Nov 30, 2018

testforstephen Nov 30, 2018

fbricon Nov 30, 2018

fbricon Nov 30, 2018

fbricon Nov 30, 2018

testforstephen commented Nov 30, 2018

fbricon commented Nov 30, 2018

fbricon commented Nov 30, 2018

fbricon commented Nov 30, 2018

testforstephen commented Dec 3, 2018

testforstephen commented Dec 6, 2018 •

edited

Loading

tsmaeder Dec 10, 2018

testforstephen Dec 11, 2018

tsmaeder Dec 10, 2018

tsmaeder Dec 10, 2018

tsmaeder Dec 10, 2018

tsmaeder Dec 10, 2018

testforstephen Dec 11, 2018

tsmaeder Dec 11, 2018

testforstephen Dec 12, 2018 •

edited

Loading

testforstephen commented Dec 13, 2018

testforstephen commented Dec 17, 2018

fbricon left a comment

Add commands to add/remove/list the project's source path #859

Add commands to add/remove/list the project's source path #859

Conversation

testforstephen commented Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testforstephen commented Nov 30, 2018

fbricon commented Nov 30, 2018

fbricon commented Nov 30, 2018

fbricon commented Nov 30, 2018

testforstephen commented Dec 3, 2018

testforstephen commented Dec 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testforstephen Dec 12, 2018 • edited Loading

Choose a reason for hiding this comment

testforstephen commented Dec 13, 2018

testforstephen commented Dec 17, 2018

fbricon left a comment

Choose a reason for hiding this comment

testforstephen commented Nov 20, 2018 •

edited

Loading

testforstephen commented Dec 6, 2018 •

edited

Loading

testforstephen Dec 12, 2018 •

edited

Loading