7989-refactor-preview: Support for Refactor Preview functionality

[danarad05]

Signed-off-by: Dan Arad [email protected]

What it does

Resolves #7989 - Adds Refactor Preview functionality.

Remarks:
These features existing in vscode were not implemented currently in this PR:

1. Checkboxes for selecting which changes to implement
2. Upon right clicking on text line in Refactor Preview pane - context menu is not displayed like in vscode.

CQ: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=23016

How to test

1. Open ts file, click F2 for renaming a function
2. Enter new name and press Shift+Enter
3. Refactor Preview pane should open in bottom with changes
4. Press V button on pane toolbar to accept and apply changes
5. Press X button to discard changes

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[vince-fugnitto]

@danarad05 CI is failing since the newly added extension does not include a test file such as:

https://github.com/eclipse-theia/theia/blob/master/packages/getting-started/src/package.spec.ts

[danarad05]

@danarad05 CI is failing since the newly added extension does not include a test file such as:

https://github.com/eclipse-theia/theia/blob/master/packages/getting-started/src/package.spec.ts

Thanks Vince, just saw it and in process of adding spec file. Thanks

[vince-fugnitto]

I have a few general comments, I have not yet tested the functionality.

We should also add the @theia/bulk-edit extension to both examples (browser and electron) since we explicitly list all the available extensions in both examples for others to reference.

[amiramw]

@danarad05 you should usually keep single commit in your PR

[danarad05]

@vince-fugnitto

What does this error mean?

[vince-fugnitto]

@vince-fugnitto

What does this error mean?

I believe it's unrelated, I'll restart the build for macOS.

[idoprz]

Hi Dan,

I tried to run one of our scenarios that uses it, and it didn't opened the preview.
Can you take a look at this:
https://github.com/SAP/code-snippet

I can help you run it if necessary.

Thanks,
Ido.

[danarad05]

@vince-fugnitto
Finished with current PR. please review.
Thanks
Dan

[vince-fugnitto]

@vince-fugnitto
Finished with current PR. please review.

@danarad05 I'd be happy to take a look. I have a few other reviews in my backlog along with EclipseCon this week so it may take some time before I am able to review it thoroughly.

[vince-fugnitto]

I have a few general comments already regarding the extension (I have not looked in depth yet for the source-code):

• when the pull-request is built, it produces unstaged changes to .travis.yml:

evinfug@elx7849163z[±|danarad05-7989-refactor-preview U:1 ✗]:~/workspaces/theia $ git diff
diff --git a/.travis.yml b/.travis.yml
index 7e1f979e1..34b4cc8aa 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -19,7 +19,6 @@ cache:
     - examples/browser/node_modules
     - examples/electron/node_modules
     - node_modules
-    - packages/bulk-edit/node_modules
     - packages/callhierarchy/node_modules
     - packages/console/node_modules
     - packages/core/node_modules

• I believe the styling of the refactor (in the editor) should be adjusted (ex: black font), and the message is poorly formatted:

theia:

vscode:

[vince-fugnitto]

when the pull-request is built, it produces unstaged changes to .travis.yml:
Not sure what you mean exactly - travis.yml is in file changed in PR. Please elaborate.

Sure, I did the following:

1. checkout the branch using the command line instructions from GitHub:

git checkout -b danarad05-7989-refactor-preview master
git pull https://github.com/danarad05/theia.git 7989-refactor-preview

2. perform a git clean -ffdx
3. perform a fresh build with yarn
4. when the build is completed, unstaged changes related to travis.yml are produced

Same thing happens when viewing the pull-request on Gitpod:

In regard to the rename label - it is displayed correctly for me, I'm not sure why it is displayed like so for you.
Seems like the keybindings are not populated for you? (or they contain CRLF) Do you have any idea why?

I'm not sure why unfortunately, looking at the keyboard shortcuts I see that the keybinding is properly set.
For additional information I'm on Linux:

Edit: I see the issue is present on the electron version and not the browser (for browser it is rendered properly like your screenshot) - For Linux.

[RomanNikitenko]

Tested the changes after today's update.
I see some differences from VS Code behavior, could you take a look:

Tab of an editor preview doesn't have title and the corresponding icon:

VS Code:

[RomanNikitenko]

At pressing Discard Refactoring button VS Code closes Refactor Preview panel and preview editor as well.

Theia behavior - preview editor is open

[RomanNikitenko]

Displaying of preview editor takes about 4-5 seconds for me.
Do you know why there is this delay and if it could be improved.

[RomanNikitenko]

At clicking an item in the Refactor Preview panel incorrect line number is selected in a refactor preview editor.
Is it related to your remark 2 in the PR description?

[RomanNikitenko]

I see that in VS Code double click on items in Refactor Preview panel selects the corresponding line number in refactor preview editor.
Looks like Theia opens refactor preview editor + an editor.

VS Code

Theia

[amiramw]

Sometimes it take a lot of time for the refactoring to calculate. For example try to rename EditorManager. It took me ~30 seconds.
Please check if there isn't a bug causing it to take so much time. Otherwise, it would be nice to have a progress indication on such long operations.

[danarad05]

When I test rename with F2 I see strikeout also on the added text.

Fixed

[danarad05]

Sometimes it take a lot of time for the refactoring to calculate. For example try to rename EditorManager. It took me ~30 seconds.
Please check if there isn't a bug causing it to take so much time. Otherwise, it would be nice to have a progress indication on such long operations.

1. You are referring to just running Rename refactor and not opening a file from refactor preview (which is a know issue Performance issue when displaying file in preview editor #8768), correct?
2. Did you get 30 seconds locally or on gitpod?
3. I tried renaming EditorManager class and it took around 3 seconds which resulted in 62 files changed in Refactor Preview. In those 3 seconds, most of the time was reading contents of files changed for display in Refactor Preview - i.e. getFileContentsMap function in bulk-edit-tree-widget.tsx

I did an improvement there which improved time by second+.
Before improvement:

After improvement:

4. I didn't manage to reproduce 30 seconds. If you can elaborate how to reproduce it I could check it again.

[amiramw]

1. You are referring to just running Rename refactor and not opening a file from refactor preview (which is a know issue Performance issue when displaying file in preview editor #8768), correct?

Yes. Calculating list of changes to be displayed in the new view.

2. Did you get 30 seconds locally or on gitpod?

Gitpod.

3. I tried renaming EditorManager class and it took around 3 seconds which resulted in 62 files changed in Refactor Preview. In those 3 seconds, most of the time was reading contents of files changed for display in Refactor Preview - i.e. getFileContentsMap function in bulk-edit-tree-widget.tsx

I did an improvement there which improved time by second+.
Before improvement:

After improvement:

1. I didn't manage to reproduce 30 seconds. If you can elaborate how to reproduce it I could check it again.

Now it takes 10 seconds in gitpod. I guess it is good enough for big refactoring.

[danarad05]

@amiramw @azatsarynnyy Please review again - if issue in conversation is satisfactory please resolve conversation.

[amiramw]

LGTM, just rebase and fix versions to 1.10.0

[danarad05]

LGTM, just rebase and fix versions to 1.10.0

@amiramw Done

[amiramw]

@akosyakov this PR can't be merged due to your requested changes. I believe they were addressed by @danarad05 , can you take a look?

dismissed due to changes

[amiramw]

@vince-fugnitto do you think we can merge this?

[vince-fugnitto]

@vince-fugnitto do you think we can merge this?

@amiramw I'll leave it up to your discretion, if the pull-request required CQ (since we copy or are inspired by the bulk-edit implementation of vscode), can you confirm it's been approved. Generally a link in the pull-request description is added for other committers to track.

[amiramw]

@amiramw I'll leave it up to your discretion, if the pull-request required CQ (since we copy or are inspired by the bulk-edit implementation of vscode), can you confirm it's been approved. Generally a link in the pull-request description is added for other committers to track.

Good catch. Created now CQ https://dev.eclipse.org/ipzilla/show_bug.cgi?id=23016

[amiramw]

According the CQ:

We have completed a high level triage of the attachment and you may now check
the content into your project’s source code repository. If a repository has not
yet been allocated, please connect with [email protected] to either create
a new repository or move your existing repository to the Eclipse Foundation.
We will perform a comprehensive analysis at a later date based on IP work
queue.

@vince-fugnitto this means that we can merge this PR, right?

[vince-fugnitto]

@vince-fugnitto this means that we can merge this PR, right?

@amiramw correct :)

[amiramw]

Merged. Thanks everyone for reviewing.