Make it possible to report folder changes to jdt.ls (not only file ch… #755

vrubezhny · 2018-08-15T01:20:39Z

…anges)

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

snjeza · 2018-08-15T02:38:04Z

@vrubezhny Could you, please, add some unit tests?

vrubezhny · 2018-08-16T22:24:08Z

@snjeza Please don't push the PR until I update it and provide a test for it.
Unfortunately this fix is not full as it fails to detect if resource a folder or a file in case the given resource is not refreshed on workspace (See JDTUtils.isFolder(String uriString)).

…anges) Related Issue: eclipse-che/che#10115 Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

vrubezhny · 2018-08-17T16:03:08Z

@snjeza I've pushed the fixed code with junit test added (see org.eclipse.jdt.ls.core.internal.JDTUtilsTest.testIsFolder()).
Unfortunately, one test failed on Jenkins, however all tests successfully passed in my local environment:

[INFO] --- tycho-p2-plugin:1.2.0:p2-metadata (p2-metadata) @ org.eclipse.jdt.ls.product ---
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO] 
[INFO] JDT Language Server :: Parent ...................... SUCCESS [  0.106 s]
[INFO] JDT Language Server :: Target Platform ............. SUCCESS [  0.276 s]
[INFO] JDT Language Server :: Core ........................ SUCCESS [ 19.276 s]
[INFO] JDT Language Server :: Tests ....................... SUCCESS [02:53 min]
[INFO] JDT Language Server :: Product ..................... SUCCESS [ 16.899 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 06:56 min
[INFO] Finished at: 2018-08-17T18:00:35+02:00
[INFO] Final Memory: 127M/1195M
[INFO] ------------------------------------------------------------------------

snjeza · 2018-08-17T18:21:47Z

test this please

snjeza · 2018-08-17T20:31:06Z

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

@@ -601,10 +603,36 @@ private static boolean isJavaIdentifierOrPeriod(char ch) {
 		return Character.isJavaIdentifierPart(ch) || ch == '.';
 	}

+	public static boolean isFolder(String uriString) {


~~return findFolder(uriString).exists();~~

findFolder(uriString) always return a new IResource object of type IContainer (it just creates it) even if uriString represents a real file (not a folder) as well as not existing file/folder (exactly as well as findFile(ueiString) returns IFile even if uriString represents a real folder). So, return findFolder(uriString).exists() will save us from not existing folder, but still return true in case of synched files - which is wrong. Also it will return false in case of not-synched, but existing folders - which is also wrong.

My goal is to report to jdt.ls on every new just created folder (or a package) in order to make it go through ProjectsManager.fileChanged(String, CHANGE_TYPE) until it refreshes the specified IResource: https://github.com/eclipse/eclipse.jdt.ls/blob/master/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/managers/ProjectsManager.java#L275 (in case the resource is folder) which is impossible (or involves some other problems/errors) with current realization.
If I do not report folder changes (create/delete/update) to jdt.ls and as such those folders stay not-synchronized (they exist in file system, but not in workspace) then such folders/packages cannot be used as a destination target package of copy/move operations (they just stay invisible for the workspace).

I know that I do an extra refresh of resource in isFolder(), but otherwise I don't know a 100%-clear way to detect if a specified uri represents a folder or a file.

findFolder(uriString) always return a new IResource object of type IContainer (it just creates it)

findFolder(uriString) doesn't create it.

I know that I do an extra refresh of resource in isFolder(), but otherwise I don't know a 100%-clear way to detect if a specified uri represents a folder or a file.

resource.getType() == IResource.FOLDER && resource.exists()

You can try the following test:

@Test public void testIsFolder() throws Exception { IProject project = WorkspaceHelper.getProject(ProjectsManager.DEFAULT_PROJECT_NAME); IFolder org = project.getFolder("/src/org"); org.delete(true, null); org.create(true, true, null); IFolder iFolder = project.getFolder("/src/org/eclipse"); IResource resource = iFolder; assertFalse(resource.getType() == IResource.FOLDER && resource.exists()); iFolder.create(true, false, null); assertTrue(resource.getType() == IResource.FOLDER && resource.exists()); URI uriFolder = iFolder.getLocationURI(); IFile iFile = project.getFile("/src/org/eclipse/Test.java"); iFile.create(new ByteArrayInputStream("".getBytes()), true, null); project.refreshLocal(IResource.DEPTH_INFINITE, null); assertTrue(iFile.exists()); URI uriFile = iFile.getLocationURI(); assertTrue(JDTUtils.isFolder(uriFolder.toString())); assertFalse(JDTUtils.isFolder(uriFile.toString())); assertNotNull(JDTUtils.findFile(uriFile.toString())); assertNotNull(JDTUtils.findFolder(uriFolder.toString())); org.delete(true, null); }

findFolder(uriString) always return a new IResource object of type IContainer (it just creates it)

findFolder(uriString) doesn't create it.

JDTUtils.findFolder(...) creates a new resource of type IContainer as well as JDTUtils.findFile(...) creates a new resource object of type IFile, independently of real resource type represented by the given URI (either it points to a file or a folder), because the execution of both methods goes through org.eclipse.core.internal.resources.Workspace.newResource(IPath, int) where new resource object of specified type is created for the given path.

In your test case you're using 'IFile.create(..)andproject.refreshLocal(DEPTH_INFINITE)- the methods which create a file in workspace or even refresh the workspace project file structure. This make all the file system resources to became "visible" for workspace and synchronized before they are tested withisFolder(), findFile()andfindFolder()`.
In my case, the folders to be reported are not synchronized with workspace - I do actually reporting their paths to jdt.ls in order to synch them in workspace (from outside of jdt.ls).

And I don't actually care where exactly refresh()is called - in ProjectManager.fileChanged() or a bit earlier in JDTUtils.isFolder() method - both places suite my needs.

I'll look at your test more deeply later - maybe I'm missing something...

org.eclipse.core.internal.resources.Workspace.newResource(IPath, int) where new resource object of specified type is created for the given path.

See javadoc for org.eclipse.core.resources.IContainer.getFile(IPath) and org.eclipse.core.internal.resources.Container.getFile(IPath)

@Test public void testFindFileFindFolder() throws Exception { IProject project = WorkspaceHelper.getProject(ProjectsManager.DEFAULT_PROJECT_NAME); IFolder org = project.getFolder("/src/org"); org.delete(true, null); org.create(true, true, null); IFolder iFolder = project.getFolder("/src/org/eclipse"); URI uriFolder = iFolder.getLocationURI(); IFile file = JDTUtils.findFile(uriFolder.toString()); assertFalse(file.exists()); IContainer folder = JDTUtils.findFolder(uriFolder.toString()); assertFalse(folder.exists()); project.refreshLocal(IResource.DEPTH_INFINITE, null); // it doesn't help assertFalse(file.exists()); assertFalse(folder.exists()); iFolder.create(true, false, null); // create the folder assertTrue(folder.exists()); assertTrue(folder.getType() == IResource.FOLDER && folder.exists()); assertFalse(file.getType() == IResource.FOLDER && file.exists()); assertTrue(file.getType() == IResource.FILE); assertFalse(file.getType() == IResource.FILE && file.exists()); org.delete(true, null); }

And I don't actually care where exactly refresh()is called - in ProjectManager.fileChanged() or a bit earlier in JDTUtils.isFolder() method - both places suite my needs.

You are right. refresh() should be called in isFolder().

Two more notes:

Your test case doesn't follow my case workflow: I cannot change workspace resources (by calling methods like IFolder.create(), IFile.create(), IFolder.delete() and so on). Not acceptable for us.

It looks like my testcase still has a problem as it relies on folders that were created by some other test case. It's a bug that I must fix.

@snjeza I have fixed my testcase. In short the workflow is the following:

[from outside of jdt.ls] some folders (packages) are created by using java.io-API

[inside jdt.ls] URI of a changed folder is provided to jdt.ls, jdt.ls is to react by refreshing the folder in its workspace (by using org.eclipse.core.resources-API) in ProjectManager.fileChanged() method (here's the work of isFolder(), findFolder() and findFile()` methods is being tested)

…file changes)

snjeza · 2018-08-18T21:44:46Z

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

+			if (!parent.isSynchronized(DEPTH_ONE)) {
+				parent.refreshLocal(DEPTH_ONE, null);
+			}
+			for (IResource member : parent.members()) {


return parent.findMember(name) instanceof IFolder;

agreed. that's a way better. fixed

snjeza · 2018-08-18T21:56:50Z

https://github.com/eclipse/eclipse.jdt.ls/blob/master/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/AbstractWorkspaceTest.java#L22

@gorkem @fbricon @vrubezhny We probable need to use After/Before instead of AfterClass/BeforeClass

…t only file changes) Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

vrubezhny · 2018-08-20T13:41:17Z

https://github.com/eclipse/eclipse.jdt.ls/blob/master/org.eclipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/AbstractWorkspaceTest.java#L22

@gorkem @fbricon @vrubezhny We probable need to use After/Before instead of AfterClass/BeforeClass

@snjeza If you do so, initWorkspace() will be invoked before each testcase in a test, as well as cleanWorkspace() will be invoked after each testcase in a test. Yes, you'll have a clean just initialized workspace before a testcase is executed, which isolates a testcase from the results of preceding testcase execution, but for tests with quite big amount of workspace initialization work the execution time will be slightly increased.
On the other hand, if you share the initialization and have some tests started to fail after some testcase is added or changed - it's a strong reason to review such testcases for possible problems (like init/cleanup of tetcase itself or any algorithmic problems) when working with the common/shared data.
Not sure if we need this change.

vrubezhny force-pushed the ls10115-part2 branch from 6daf588 to 5ccfb9d Compare August 15, 2018 01:39

snjeza self-requested a review August 15, 2018 02:27

Make it possible to report folder changes to jdt.ls (not only file ch…

de9b178

…anges) Related Issue: eclipse-che/che#10115 Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

vrubezhny force-pushed the ls10115-part2 branch from 5ccfb9d to de9b178 Compare August 17, 2018 15:13

snjeza requested changes Aug 17, 2018

View reviewed changes

fixup! Make it possible to report folder changes to jdt.ls (not only …

3a38b03

…file changes)

snjeza requested changes Aug 18, 2018

View reviewed changes

fixup! fixup! Make it possible to report folder changes to jdt.ls (no…

05d5515

…t only file changes) Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

fbricon added this to the End August 2018 milestone Aug 20, 2018

fbricon added the enhancement label Aug 20, 2018

fbricon assigned vrubezhny Aug 20, 2018

snjeza approved these changes Aug 20, 2018

View reviewed changes

fbricon merged commit b49b83a into eclipse-jdtls:master Aug 20, 2018

vrubezhny mentioned this pull request Aug 23, 2018

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make it possible to report folder changes to jdt.ls (not only file ch… #755

Make it possible to report folder changes to jdt.ls (not only file ch… #755

vrubezhny commented Aug 15, 2018

snjeza commented Aug 15, 2018

vrubezhny commented Aug 16, 2018

vrubezhny commented Aug 17, 2018

snjeza commented Aug 17, 2018

snjeza Aug 17, 2018 •

edited

vrubezhny Aug 17, 2018

snjeza Aug 17, 2018

vrubezhny Aug 17, 2018 •

edited

snjeza Aug 18, 2018

vrubezhny Aug 18, 2018

vrubezhny Aug 18, 2018

snjeza Aug 18, 2018

vrubezhny Aug 20, 2018

snjeza commented Aug 18, 2018

vrubezhny commented Aug 20, 2018

Make it possible to report folder changes to jdt.ls (not only file ch… #755

Make it possible to report folder changes to jdt.ls (not only file ch… #755

Conversation

vrubezhny commented Aug 15, 2018

snjeza commented Aug 15, 2018

vrubezhny commented Aug 16, 2018

vrubezhny commented Aug 17, 2018

snjeza commented Aug 17, 2018

snjeza Aug 17, 2018 • edited

Choose a reason for hiding this comment

vrubezhny Aug 17, 2018

Choose a reason for hiding this comment

snjeza Aug 17, 2018

Choose a reason for hiding this comment

vrubezhny Aug 17, 2018 • edited

Choose a reason for hiding this comment

snjeza Aug 18, 2018

Choose a reason for hiding this comment

vrubezhny Aug 18, 2018

Choose a reason for hiding this comment

vrubezhny Aug 18, 2018

Choose a reason for hiding this comment

snjeza Aug 18, 2018

Choose a reason for hiding this comment

vrubezhny Aug 20, 2018

Choose a reason for hiding this comment

snjeza commented Aug 18, 2018

vrubezhny commented Aug 20, 2018

snjeza Aug 17, 2018 •

edited

vrubezhny Aug 17, 2018 •

edited