Support Rename refactoring by svor · Pull Request #9636 · eclipse-che/che

svor · 2018-05-07T16:14:18Z

What does this PR do?

Make it possible to do Rename operation by using jdt.ls.
The result of this PR is here: https://youtu.be/HkCKpGSP05o

Depends on

What issues does this PR fix or reference?

tolusha · 2018-05-08T08:37:44Z

.../src/main/java/org/eclipse/che/ide/ext/java/client/refactoring/preview/PreviewPresenter.java

-            });
+    applyWorkspaceEditAction.applyWorkspaceEdit(edit);
+    view.close();
+    refactoringActionDelegate.closeWizard();


It invokes onCancelButtonClicked();
I think view.close() is enough.

view.close() closes just Preview window, but also we need to close the other one (Rename or Move) window which was opened previously

tsmaeder · 2018-05-09T11:27:31Z

...t/src/main/java/org/eclipse/che/ide/ext/java/client/diagnostics/PomDiagnosticsRequestor.java

          public void onEditorOpened(EditorOpenedEvent event) {
            String uri = buildHelper.getUri(event.getFile());
-            if (uri.endsWith(POM_FILE)) {
+            if (!uri.endsWith(POM_FILE)) {


Drive-by fix?

that's right

tsmaeder · 2018-05-09T11:36:13Z

.../src/main/java/org/eclipse/che/ide/ext/java/client/refactoring/preview/PreviewPresenter.java

+          if (!arg.isPresent()) {
+            return;
+          }
+          File existedFile = arg.get();


"existingFile"

tsmaeder · 2018-05-09T11:38:56Z

.../src/main/java/org/eclipse/che/ide/ext/java/client/refactoring/preview/PreviewPresenter.java

+    }
+    PreviewNode node = nodes.get(selectedNode.getUri());
+    List<TextEdit> edits = new ArrayList<>();
+    if (node.getId().equals(selectedNode.getId())) {


Not quite sure what we are doing here. Refactor so it's obvious or add comments.

refactored and added some comments

tsmaeder · 2018-05-09T11:41:29Z

.../src/main/java/org/eclipse/che/ide/ext/java/client/refactoring/preview/PreviewPresenter.java


-  private RefactorInfo refactorInfo;
-  private RefactoringSession session;
+  private Map<String, PreviewNode> nodes;


Are all nodes in this map? If not, this needs a different name. fileNodes?

renamed to fileNodes

tsmaeder · 2018-05-09T11:47:07Z

.../src/main/java/org/eclipse/che/ide/ext/java/client/refactoring/preview/PreviewPresenter.java

+    this.refactoringActionDelegate = refactoringActionDelegate;
+    nodes = new LinkedHashMap<>();
+
+    prepareNodes(workspaceEdit);


As I see it, prepareNodes just fills in the nodes map. why not do
nodes= prepareNodes(workspaceEdit);
If you make prepareNodes static, you can show that there are no side-effects in the method. Making stuff side-effect free simplifies understanding the code.

tsmaeder · 2018-05-09T11:59:56Z

.../main/java/org/eclipse/che/ide/ext/java/client/refactoring/rename/JavaRefactoringRename.java

+              for (ResourceChange resourceChange : edits.getResourceChanges()) {
+                edit.getResourceChanges().add(Either.forLeft(resourceChange));
              }
+              undoChanges();


Could you clarify please?
If you're thinking about why I undo changes here. It is because before applying all TextEdit changes we need to have initial state of the content where linked mode was activated.

yes, that's what I was thinking of.

tsmaeder · 2018-05-09T12:05:15Z

...main/java/org/eclipse/che/ide/ext/java/client/refactoring/rename/wizard/RenamePresenter.java

+    edit.setChanges(workspaceEdit.getChanges());
+    edit.setResourceChanges(new LinkedList<>());
+    for (ResourceChange resourceChange : workspaceEdit.getResourceChanges()) {
+      edit.getResourceChanges().add(Either.forLeft(resourceChange));


Out of curiosity...I thought sending WorkspaceChange over the wire did not work? What has changed?

Sending List<Either<ResourceChange, TextEdit>> which is described in org.eclipse.lsp4j.WorkspaceEdit doesn't work.
So here workspaceEdit is an instance of custom object (org.eclipse.che.jdt.ls.extension.api.dto.CheWorkspaceEdit) where I already have all actual changes and I take all changes from it and put them to the edit (org.eclipse.lsp4j.WorkspaceEdit) to sent it to class which applies org.eclipse.lsp4j.WorkspaceEdit (org.eclipse.che.plugin.languageserver.ide.editor.quickassist.ApplyWorkspaceEditAction#ApplyWorkspaceEditAction)

tsmaeder · 2018-05-09T12:11:48Z

...a/org/eclipse/che/plugin/languageserver/ide/editor/quickassist/ApplyWorkspaceEditAction.java

+      String newUri = change.getNewUri();
+      String current = change.getCurrent();
+
+      Path path = toPath(newUri).removeFirstSegments(1).makeAbsolute();


Transformation of file uri's to paths is usually done in WSMaster, so results from lsp servers contain either workspace paths of non-file URI's. See LanguageServiceUtils.removePrefixURI.

GWT doesn't support java.net.* so we can't use LanguageServiceUtils.java on the client. I've created utility class org.eclipse.che.api.languageserver.util.URIUtil which transforms uri to path.

Not what I mean, I've tried to explain in chat.

tsmaeder · 2018-05-09T12:13:43Z

...a/org/eclipse/che/plugin/languageserver/ide/editor/quickassist/ApplyWorkspaceEditAction.java

+        .getResource(path)
+        .then(
+            resource -> {
+              if (isNullOrEmpty(path.getFileExtension())) {


I dont' think this is a valid condition. It's perfectly valid to create files without extensions and folders with.

yes, but I don't see another way to recognize which type of resource is here. Is it a folder or is a file.

I think we need to fix this in CheWorkspaceEdit (make CheResourceChange.java) and have a "resourceType" enum to be used on create.

tsmaeder · 2018-05-09T12:14:30Z

...a/org/eclipse/che/plugin/languageserver/ide/editor/quickassist/ApplyWorkspaceEditAction.java

+    resource
+        .move(path)
+        .then(
+            arg -> {


arg is not a good name.

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

svor added kind/enhancement A feature request - must adhere to the feature request template. team/languages labels May 7, 2018

svor self-assigned this May 7, 2018

svor requested review from dkulieshov, evidolob and vparfonov as code owners May 7, 2018 16:14

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

svor changed the title ~~[WIP]Support Rename refactoring~~ Support Rename refactoring May 8, 2018

svor requested review from tolusha and tsmaeder May 8, 2018 07:29

tolusha reviewed May 8, 2018

View reviewed changes

vparfonov approved these changes May 8, 2018

View reviewed changes

tsmaeder requested changes May 9, 2018

View reviewed changes

tsmaeder added team/languages and removed team/languages labels May 11, 2018

tolusha force-pushed the 5730_java_ls_poc branch from 245c161 to 62ecb10 Compare May 12, 2018 19:47

svor requested review from ashumilova and benoitf as code owners May 12, 2018 21:37

svor requested a review from a user May 12, 2018 21:37

svor requested review from garagatyi, l0rd, riuvshin, skabashnyuk and sleshchenko as code owners May 12, 2018 21:37

svor force-pushed the rename branch from 31d6ffb to 81688be Compare May 12, 2018 21:49

svor removed request for ashumilova, benoitf, l0rd and skabashnyuk May 12, 2018 21:50

tsmaeder force-pushed the 5730_java_ls_poc branch 2 times, most recently from 6901019 to 518f5f6 Compare June 4, 2018 09:34

use jdt.ls to support Rename refactoring

73dca1a

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

svor force-pushed the rename branch from 6a742c4 to 73dca1a Compare June 4, 2018 14:19

tsmaeder mentioned this pull request Jun 5, 2018

Languages Sprint 150 #9804

Closed

22 tasks

svor merged commit 0558d31 into 5730_java_ls_poc Jun 5, 2018

svor deleted the rename branch June 5, 2018 12:36

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 Jun 5, 2018

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

use jdt.ls to support Rename refactoring (#9636)

d4c04a3

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

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

use jdt.ls to support Rename refactoring (#9636)

3dafece

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

tsmaeder mentioned this pull request Jun 18, 2018

Languages Sprint 151 #10075

Closed

26 tasks

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

use jdt.ls to support Rename refactoring (#9636)

8bde312

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

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

use jdt.ls to support Rename refactoring (#9636)

67f361c

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

tsmaeder pushed a commit that referenced this pull request Jul 27, 2018

use jdt.ls to support Rename refactoring (#9636)

50d54df

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

tolusha pushed a commit that referenced this pull request Aug 7, 2018

use jdt.ls to support Rename refactoring (#9636)

528a188

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

tsmaeder pushed a commit that referenced this pull request Aug 22, 2018

use jdt.ls to support Rename refactoring (#9636)

1003ccd

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

tsmaeder pushed a commit that referenced this pull request Aug 29, 2018

use jdt.ls to support Rename refactoring (#9636)

4042dbb

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

tolusha pushed a commit that referenced this pull request Sep 5, 2018

use jdt.ls to support Rename refactoring (#9636)

747143c

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

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

use jdt.ls to support Rename refactoring (#9636)

aac7989

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

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

use jdt.ls to support Rename refactoring (#9636)

b8ac5ef

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

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

use jdt.ls to support Rename refactoring (#9636)

cf83a3d

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

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

use jdt.ls to support Rename refactoring (#9636)

4088647

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

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

use jdt.ls to support Rename refactoring (#9636)

6d4a58b

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

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

use jdt.ls to support Rename refactoring (#9636)

9a98d20

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

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

use jdt.ls to support Rename refactoring (#9636)

87f175a

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

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

use jdt.ls to support Rename refactoring (#9636)

85004d5

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

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

use jdt.ls to support Rename refactoring (#9636)

559294e

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

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

use jdt.ls to support Rename refactoring (#9636)

a3adf16

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

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

use jdt.ls to support Rename refactoring (#9636)

068336b

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

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

use jdt.ls to support Rename refactoring (#9636)

4a6be0f

Signed-off-by: Valeriy Svydenko <vsvydenk@redhat.com>

Conversation

svor commented May 7, 2018

What does this PR do?

What issues does this PR fix or reference?

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants