New folders do not register in the jdt.ls workspace #10115 by vrubezhny · Pull Request #10893 · eclipse-che/che

vrubezhny · 2018-08-23T13:18:09Z

This adds the reporting creation/modification/deletion of source folders to jdt.ls

Signed-off-by: Victor Rubezhny vrubezhny@redhat.com

What does this PR do?

Fix adds a listener for changes inside source folders (Create/Update/Delete) in order to report them to jds.ls.

What issues does this PR fix or reference?

Fixes #10115
Depends on eclipse-che/che-ls-jdt#70
Depends on eclipse-jdtls/eclipse.jdt.ls#755

Release Notes

Docs PR

riuvshin · 2018-08-23T13:18:10Z

Can one of the admins verify this patch?

riuvshin · 2018-08-23T13:18:10Z

Can one of the admins verify this patch?

riuvshin · 2018-08-23T13:18:10Z

Can one of the admins verify this patch?

tsmaeder · 2018-08-27T11:52:32Z

plugins/plugin-java/che-plugin-java-plain/che-plugin-java-plain-server/pom.xml

            <groupId>commons-io</groupId>
            <artifactId>commons-io</artifactId>
        </dependency>
+				<dependency>


The formatting seems wrong here

Why do we need annotation API?

I don't have those tabs in my code... Looks like they were there for some time, but after I fixed them locally git ignores the fix. I'll try to fix

Probably because of:

+import javax.annotation.PostConstruct; +import javax.annotation.PreDestroy;

in PlainJavaProjectSourceFolderWatcher

tsmaeder · 2018-08-27T11:55:42Z

.../src/test/java/org/eclipse/che/selenium/refactor/move/MoveJavaFileInNewSourceFolderTest.java

    projectExplorer.waitAndSelectItem(PATH_TO_FILE);
+
+    // As FileTreeWalker's period is 10 secs - wait for the folders to be finally reported to jdt.ls
+    WaitUtils.sleepQuietly(11);


Isn't there a condition we can wait for?

Right answer is yes, we're waiting for a condition but, during the test, the content of 'Move'-dialog's tree is not updated. This sleepQuitely() call allows all the required changes to happen before the dialog is invoked.

tsmaeder · 2018-08-27T11:57:32Z

...main/java/org/eclipse/che/plugin/java/languageserver/JavaLanguageServerExtensionService.java

+   * @param projectPath project path
+   * @return source folder locations
+   */
+  public List<String> getAllSourceFoldersLocations(String projectPath) {


Source folders are package fragement roots, not package fragments. I dont' think the command you introduced does what you think it does.

Should read as "every location in source folder"...
If we have a project tree like:

Project +-- SourceFolder +-- org.pack.age +-- org.pack.house

With this method I'm going to get the list consisting of the following paths:

/projects/Project/SourceFolder/org/pack/age /projects/Project/SourceFolder/org/pack/house

in order to make them accepted by FileWatcherByPathMatcher.
If I don't do this, any file/folder that is not created during the working session (any existing path of a project) is never being reported as Updated or Deleted and, as such, will never be refreshed on jdt.ls workspace. As result, every such path (package) deleted by user, despite of its deletions, will appear in Move-dialog as an existing package.

tsmaeder · 2018-08-27T12:02:59Z

...main/java/org/eclipse/che/plugin/java/languageserver/JavaLanguageServerExtensionService.java

    return result.stream().map(LanguageServiceUtils::removePrefixUri).collect(Collectors.toList());
  }

+  /**


JavaLanguageServerExtensionService is made for communication with the browser. That's one reason why the methods are synchronous. This method here is NOT for that purpose, so should be somewhere else.

tsmaeder · 2018-08-27T12:04:14Z

...ava/org/eclipse/che/plugin/java/plain/server/inject/PlainJavaProjectSourceFolderWatcher.java

+  private void setInitialPathsToWatch(String projectUri) {
+    List<String> sourceFolderLocations;
+    try {
+      sourceFolderLocations = extensionService.getAllSourceFoldersLocations(projectUri);


We should not use a synchronous call here, rather use the promise we get from "WorkspaceService.executeCommand()". Then we can get rid of the executor service.

Re-implemented

tsmaeder · 2018-08-27T12:06:51Z

...ava/org/eclipse/che/plugin/java/plain/server/inject/PlainJavaProjectSourceFolderWatcher.java

+    watcherIds.stream().forEach(id -> manager.unRegisterByMatcher(id));
+  }
+
+  private void onServerInitialized(LanguageServerInitializedEvent event) {


No need to collect all language servers here. We only want to notify the java language server, and we can find that one by id.

We don't need to listen to language server start events. There is supposed to be a way to just ask for a language server by id. No need to remember it here.

It's done exactly the same way here:
https://github.com/eclipse/che/blob/master/wsagent/che-core-api-languageserver/src/main/java/org/eclipse/che/api/languageserver/LanguageServerFileWatcher.java#L82

If I won't wait for LS start, the obtaining of Java LS will be failed - not sure if it's a good trying to do anything with LS before it's started...
I don't know... Maybe I can use someow org.eclipse.che.api.languageserver.FindServer.byId(String) - but again - it could return null (if it is allowed t use it from che-plugin-java-plain-server)

tsmaeder · 2018-08-27T12:07:46Z

...ava/org/eclipse/che/plugin/java/plain/server/inject/PlainJavaProjectSourceFolderWatcher.java

+  }
+
+  private void send(LanguageServer server, String path, FileChangeType changeType) {
+    RegisteredProject project = projectManager.getClosestOrNull(path);


Why is this check necessary?

Not cleaned up code. sorry.

tsmaeder · 2018-08-27T12:13:16Z

...ava/org/eclipse/che/plugin/java/plain/server/inject/PlainJavaProjectSourceFolderWatcher.java

+  }
+
+  private PathMatcher folderMatcher() {
+    return it -> isSourceFolder(it);


This is causing us to communicate with jdt.ls for every folder change in the file system. I think it would be more efficient to send every folder change to jdt.ls and let jdt.ls do the filtering.
Also, are we interested in every change or only in create/delete?

Yes, you're right, I think we can reduce this check to just isDirectory(it) call. And we don't need to send Update as jdt.ls just ignores this event for folders.

vrubezhny · 2018-08-28T02:30:08Z

@tsmaeder I've re-implemented the fix based on your comments.
As result MoveJavaFileInNewSourceFolderTest is also looks fixed. (However I still in doubt if I should have leaved that sleepQuitely() line in the test as the default timeout is equal to the File Tree Walker period and, in theory, test might fail by timeout under some circumstances)

Please review.

...ava/org/eclipse/che/plugin/java/plain/server/inject/PlainJavaProjectSourceFolderWatcher.java

tsmaeder · 2018-08-29T06:55:45Z

...ava/org/eclipse/che/plugin/java/plain/server/inject/PlainJavaProjectSourceFolderWatcher.java

+    watcherIds.stream().forEach(id -> manager.unRegisterByMatcher(id));
+  }
+
+  private void onServerInitialized(LanguageServerInitializedEvent event) {


We don't need to listen to language server start events. There is supposed to be a way to just ask for a language server by id. No need to remember it here.

...ava/org/eclipse/che/plugin/java/plain/server/inject/PlainJavaProjectSourceFolderWatcher.java

This adds the reporting creation/modification/deletion of source folders to jdt.ls Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

vrubezhny · 2018-09-05T19:08:17Z

@tsmaeder I've rebased and modified this PR - now it tries to find Java LS when it's needed by its ID (JavaModule.LS_ID)

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

vrubezhny requested review from Ohrimenko1988, dmytro-ndp, musienko-maxim and vparfonov as code owners August 23, 2018 13:18

tsmaeder added kind/bug Outline of a bug - must adhere to the bug report template. team/languages labels Aug 24, 2018

tsmaeder requested changes Aug 27, 2018

View reviewed changes

vrubezhny force-pushed the ls10115-part2 branch 2 times, most recently from ade6988 to b6379dc Compare August 28, 2018 02:22

benoitf added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. target/branch Indicates that a PR will be merged into a branch other than master. labels Aug 28, 2018

svor requested a review from tolusha August 29, 2018 06:29

tsmaeder force-pushed the 5730_java_ls_poc branch from b3033ac to 9d7bee0 Compare August 29, 2018 11:18

vrubezhny force-pushed the ls10115-part2 branch from b6379dc to 71d86a7 Compare August 30, 2018 15:59

tolusha reviewed Aug 31, 2018

View reviewed changes

...ava/org/eclipse/che/plugin/java/plain/server/inject/PlainJavaProjectSourceFolderWatcher.java Show resolved Hide resolved

tolusha approved these changes Sep 5, 2018

View reviewed changes

tsmaeder requested changes Sep 5, 2018

View reviewed changes

tolusha force-pushed the 5730_java_ls_poc branch from 549b240 to 6c2d0b9 Compare September 5, 2018 14:32

New folders do not register in the jdt.ls workspace eclipse-che#10115

99bf160

This adds the reporting creation/modification/deletion of source folders to jdt.ls Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

vrubezhny force-pushed the ls10115-part2 branch from 71d86a7 to 99bf160 Compare September 5, 2018 18:34

tsmaeder approved these changes Sep 5, 2018

View reviewed changes

tsmaeder merged commit 9d271dd into eclipse-che:5730_java_ls_poc Sep 5, 2018

benoitf removed the status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. label Sep 5, 2018

tsmaeder pushed a commit that referenced this pull request Sep 13, 2018

New folders do not register in the jdt.ls workspace #10115 (#10893)

2dc2202

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

tsmaeder pushed a commit that referenced this pull request Sep 20, 2018

New folders do not register in the jdt.ls workspace #10115 (#10893)

b121a59

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

tsmaeder pushed a commit that referenced this pull request Sep 26, 2018

New folders do not register in the jdt.ls workspace #10115 (#10893)

2720a18

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

tsmaeder pushed a commit that referenced this pull request Oct 1, 2018

New folders do not register in the jdt.ls workspace #10115 (#10893)

00f3eb4

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

tsmaeder pushed a commit that referenced this pull request Oct 5, 2018

New folders do not register in the jdt.ls workspace #10115 (#10893)

13ca79f

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

tsmaeder pushed a commit that referenced this pull request Oct 12, 2018

New folders do not register in the jdt.ls workspace #10115 (#10893)

8485b5c

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

tsmaeder pushed a commit that referenced this pull request Oct 16, 2018

New folders do not register in the jdt.ls workspace #10115 (#10893)

c8257e6

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

tsmaeder pushed a commit that referenced this pull request Oct 17, 2018

New folders do not register in the jdt.ls workspace #10115 (#10893)

bde0a49

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

tsmaeder pushed a commit that referenced this pull request Oct 17, 2018

New folders do not register in the jdt.ls workspace #10115 (#10893)

39f970f

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

tsmaeder pushed a commit that referenced this pull request Oct 17, 2018

New folders do not register in the jdt.ls workspace #10115 (#10893)

5a152be

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

tsmaeder pushed a commit that referenced this pull request Oct 17, 2018

New folders do not register in the jdt.ls workspace #10115 (#10893)

2488c8a

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

tsmaeder pushed a commit that referenced this pull request Oct 17, 2018

New folders do not register in the jdt.ls workspace #10115 (#10893)

46582b2

Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

Conversation

vrubezhny commented Aug 23, 2018

What does this PR do?

What issues does this PR fix or reference?

Release Notes

Docs PR

Uh oh!

riuvshin commented Aug 23, 2018

Uh oh!

riuvshin commented Aug 23, 2018

Uh oh!

riuvshin commented Aug 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrubezhny Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrubezhny Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrubezhny commented Aug 28, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vrubezhny commented Sep 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vrubezhny Aug 27, 2018 •

edited

Loading

vrubezhny Sep 5, 2018 •

edited

Loading