Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support change signature refactoring #2967

Merged
merged 13 commits into from
Mar 17, 2023
Merged

Support change signature refactoring #2967

merged 13 commits into from
Mar 17, 2023

Conversation

CsCherrYY
Copy link
Contributor

@CsCherrYY CsCherrYY commented Feb 27, 2023 •
edited
Loading

fix #2104

related to eclipse-jdtls/eclipse.jdt.ls#2497

a simple demo:

change.mp4

@CsCherrYY CsCherrYY mentioned this pull request Feb 27, 2023
Merged
Eskibear
Eskibear reviewed Feb 28, 2023
View reviewed changes
package.json Outdated Show resolved Hide resolved
@CsCherrYY
address comments
48037a8 
Signed-off-by: Shi Chen <[email protected]>
@CsCherrYY
1. improve parameter/exception table
f8a6982 
2. split refactor/preview buttons

Signed-off-by: Shi Chen <[email protected]>
@CsCherrYY
refine margin and enable debugging
5957344 
Signed-off-by: Shi Chen <[email protected]>
jdneo
jdneo reviewed Mar 1, 2023
View reviewed changes
src/refactoring/changeSignaturePanel.ts Outdated Show resolved Hide resolved
src/webview/utils.ts Outdated Show resolved Hide resolved
CsCherrYY added 2 commits March 1, 2023 16:10
@CsCherrYY
1. address comments
4705403 
2. align left margins

Signed-off-by: Shi Chen <[email protected]>
@CsCherrYY
1. add panel icon
4e83f55 
2. set text as preview cursor

Signed-off-by: Shi Chen <[email protected]>
@jdneo
Copy link
Collaborator

jdneo commented Mar 2, 2023

One more change request: Please add the description about this refactoring into https://github.com/redhat-developer/vscode-java/blob/master/document/_java.learnMoreAboutRefactorings.md.

And maybe other missing refactorings(e.g. extract interface)

You may encounter a problem that the document will not appear, a workaround for testing can be found at #2974

@CsCherrYY
Copy link
Contributor Author

description added in d419eab

And maybe other missing refactorings(e.g. extract interface)

Let me create another PR to add the missing refactoring descriptions.

@CsCherrYY
1. add reset button
4de426f 
2. layout improvement
3. tips for re-open the refactoring
4. remain unchecked in the preview

Signed-off-by: Shi Chen <[email protected]>
@jdneo
Copy link
Collaborator

jdneo commented Mar 10, 2023

Some feedback:

  1. If set the access modifier to package-private, the signature preview will show package-private. Should be without any keyword.
  2. Change the name of the parameter. After clicking reset, the name didn't reset to the original one.
  3. Click the column header will highlight it, is this by design?
    image
  4. I'm a little bit confused by the Default value, could be helpful if some tooltip can be provided.

@CsCherrYY
1. address comments
0722f74 
2. fix package private modifier
3. move backend functions from /webview/utils.ts to /webviewUtils.ts

Signed-off-by: Shi Chen <[email protected]>
@jdneo
Copy link
Collaborator

jdneo commented Mar 14, 2023

I cannot change the added parameter name now

@CsCherrYY
Copy link
Contributor Author

I cannot change the added parameter name now

My bad, just using same style for editing and non-editing parameter cell. 1cc199b fixes this.

@jdneo
Copy link
Collaborator

jdneo commented Mar 14, 2023

Click Keep original method as delegate to changed method, then click Reset. Nothing happens.

Looks like something wrong with the status handling.

@CsCherrYY
1. address comments
2818564 
2. use loadsh.cloneDeep() to keep state clean

Signed-off-by: Shi Chen <[email protected]>
jdneo
jdneo reviewed Mar 15, 2023
View reviewed changes
src/webview/changeSignature/App.tsx Outdated Show resolved Hide resolved
src/webview/changeSignature/App.tsx Outdated Show resolved Hide resolved
@CsCherrYY
address comments
35acd8e 
Signed-off-by: Shi Chen <[email protected]>
jdneo
jdneo approved these changes Mar 16, 2023
View reviewed changes
Copy link
Collaborator

@jdneo jdneo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. We can experiment it in the insider before the next month's stable release.

@jdneo jdneo merged commit 56b2516 into redhat-developer:master Mar 17, 2023
@jdneo jdneo added this to the Early April 2023 milestone Mar 20, 2023
This was referenced Mar 20, 2023
Merged
Closed
Closed
@xieu90
Copy link

xieu90 commented Apr 4, 2023

how can i try this feature? in vscode Ctrl shift P install extension: java pack?

@fbricon
Copy link
Collaborator

fbricon commented Apr 4, 2023

You can install the latest vscode-java pre-release

@xieu90
Copy link

xieu90 commented Apr 4, 2023

i dont really see vscode-java pre-release in extension search window

image

i tried to change the java pack to pre-release and dont see change signature refactoring either.

@fbricon
Copy link
Collaborator

fbricon commented Apr 4, 2023

"Language Support for Java(TM) by Red Hat" is the full name for vscode-java:

Screenshot 2023-04-04 at 11 25 58

@xieu90
Copy link

xieu90 commented Apr 4, 2023

it works now, i can see it, thank you ^^

@CsCherrYY CsCherrYY deleted the cs-changeSignature branch April 5, 2023 01:05
@rgrunber rgrunber removed this from the Early April 2023 milestone Apr 12, 2023
@gbroques gbroques mentioned this pull request Apr 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Eskibear Eskibear Eskibear left review comments

@jdneo jdneo jdneo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request] Method Signature Refactoring
6 participants
@CsCherrYY @jdneo @xieu90 @fbricon @Eskibear @rgrunber