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

Add source CodeAction support for organize imports. #856

Merged
merged 3 commits into from
Nov 21, 2018
Merged

Add source CodeAction support for organize imports. #856

merged 3 commits into from
Nov 21, 2018

Conversation

yaohaizh
Copy link
Contributor

@yaohaizh yaohaizh commented Nov 19, 2018 •
edited by fbricon
Loading

@fbricon The original organize imports delegate commands is not deleted yet since it is used by other client and provide organize in folders/workspaces functionalities.

Fixes #845

@yaohaizh yaohaizh force-pushed the yaohai_source_action branch from 4bb07a3 to 532fd6d Compare November 19, 2018 07:03
fbricon
fbricon reviewed Nov 19, 2018
View reviewed changes
org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/corrections/QuickFixProcessor.java Outdated
@@ -60,10 +60,10 @@ private static int moveBack(int offset, int start, String ignoreCharacters, ICom
return start;
}

public CUCorrectionProposal[] getCorrections(IInvocationContext context, IProblemLocationCore[] locations)
public ArrayList<CUCorrectionProposal> getCorrections(IInvocationContext context, IProblemLocationCore[] locations)
Copy link
Contributor

Choose a reason for hiding this comment

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

public List

fbricon
fbricon reviewed Nov 19, 2018
View reviewed changes
org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/corrections/QuickFixProcessor.java Outdated
throws CoreException {
if (locations == null || locations.length == 0) {
return new CUCorrectionProposal[0];
return new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

return Collections.emptyList();

fbricon
fbricon reviewed Nov 19, 2018
View reviewed changes
org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CodeActionHandler.java Outdated
$.add(getCodeActionFromProposal(proposal, params.getContext()));
}
ArrayList<CUCorrectionProposal> corrections = this.quickFixProcessor.getCorrections(context, locations);
corrections.sort(new CUCorrectionProposalComparator());
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to sort it now, it'll be sorted again L101

fbricon
fbricon reviewed Nov 19, 2018
View reviewed changes
org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CodeActionHandler.java Outdated

try {
ArrayList<CUCorrectionProposal> corrections = this.quickAssistProcessor.getAssists(context, locations);
corrections.sort(new CUCorrectionProposalComparator());
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to sort it now, it'll be sorted again L101

fbricon
fbricon reviewed Nov 19, 2018
View reviewed changes
org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CodeActionHandler.java Outdated
}

ArrayList<CUCorrectionProposal> corrections = this.sourceAssistProcessor.getAssists(context, locations);
corrections.sort(new CUCorrectionProposalComparator());
Copy link
Contributor

Choose a reason for hiding this comment

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

sort candidates instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would keep the group by kind before sort.

Copy link
Contributor

Choose a reason for hiding this comment

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

then just make the comparator use the kind 1st

fbricon
fbricon reviewed Nov 19, 2018
View reviewed changes
...e.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/text/correction/QuickAssistProcessor.java Outdated
@@ -141,7 +141,7 @@
public QuickAssistProcessor() {
}

public CUCorrectionProposal[] getAssists(IInvocationContext context, IProblemLocationCore[] locations) throws CoreException {
public ArrayList<CUCorrectionProposal> getAssists(IInvocationContext context, IProblemLocationCore[] locations) throws CoreException {
Copy link
Contributor

Choose a reason for hiding this comment

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

public List

fbricon
fbricon reviewed Nov 19, 2018
View reviewed changes
...e.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/text/correction/QuickAssistProcessor.java Outdated
}
return new CUCorrectionProposal[0];
return new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Collections.emptyList

fbricon
fbricon reviewed Nov 19, 2018
View reviewed changes
....jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/text/correction/SourceAssistProcessor.java Outdated

public class SourceAssistProcessor {

public SourceAssistProcessor() {
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary constructor

fbricon
fbricon reviewed Nov 19, 2018
View reviewed changes
....jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/text/correction/SourceAssistProcessor.java Outdated
@@ -0,0 +1,61 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

remove extra blank line

fbricon
fbricon reviewed Nov 19, 2018
View reviewed changes
...lipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/correction/AbstractQuickFixTest.java Outdated
@@ -179,6 +190,22 @@ protected void setIgnoredCommands(List<String> ignoredCommands) {
parms.setContext(context);

List<Either<Command, CodeAction>> codeActions = new CodeActionHandler(this.preferenceManager).getCodeActionCommands(parms, new NullProgressMonitor());

if (this.ignoredKinds != null) {
List<Either<Command, CodeAction>> filteredList = new ArrayList<>();
Copy link
Contributor

Choose a reason for hiding this comment

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

filteredList = codeActions.stream().filter(Either::isRight).filter(codeAction -> {
for (String str : this.ignoredKinds) {
if (codeAction.getRight().getKind().matches(str)) {
return false;
}
}
return true;
}).collect(Collectors.toList())

fbricon
fbricon reviewed Nov 19, 2018
View reviewed changes
...lipse.jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/correction/AbstractQuickFixTest.java Outdated
@@ -161,6 +168,10 @@ protected void setIgnoredCommands(List<String> ignoredCommands) {
this.ignoredCommands = ignoredCommands;
}

protected void setIgnoredKind(List<String> ignoredKind) {
Copy link
Contributor

Choose a reason for hiding this comment

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

String...ignoredKind makes it easier to use

fbricon
fbricon reviewed Nov 19, 2018
View reviewed changes
....jdt.ls.tests/src/org/eclipse/jdt/ls/core/internal/correction/OrganizeImportsActionTest.java Outdated
fJProject1.setOptions(options);
fSourceFolder = fJProject1.getPackageFragmentRoot(fJProject1.getProject().getFolder("src"));

this.setIgnoredKind(Arrays.asList("quickfix.*", "refactor.*"));
Copy link
Contributor

Choose a reason for hiding this comment

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

this.setIgnoredKind("quickfix.", "refactor."); when you have setIgnoredKind(String...ignoredKind)

@yaohaizh yaohaizh mentioned this pull request Nov 19, 2018
Merged
fbricon
fbricon requested changes Nov 19, 2018
View reviewed changes
Copy link
Contributor

@fbricon fbricon left a comment

Choose a reason for hiding this comment

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

A few minor things to change, but overall, this works as expected. I'm looking forward to merging it

@yaohaizh yaohaizh force-pushed the yaohai_source_action branch from 532fd6d to 2f7ad52 Compare November 20, 2018 06:20
@yaohaizh
Copy link
Contributor Author

@fbricon Updated PR. Just keep the group by kind behavior.

@fbricon
Copy link
Contributor

fbricon commented Nov 21, 2018

test this please

@fbricon
Copy link
Contributor

fbricon commented Nov 21, 2018

@yaohaizh please fix the build (390 failures because of new, unexpected, code action)

@yaohaizh yaohaizh force-pushed the yaohai_source_action branch from cbf60d8 to a2cae96 Compare November 21, 2018 08:29
@yaohaizh
Copy link
Contributor Author

@fbricon Updated the PR

@yaohaizh
Copy link
Contributor Author

test this please

@fbricon fbricon merged commit da21494 into master Nov 21, 2018
@fbricon fbricon deleted the yaohai_source_action branch November 21, 2018 15:18
@JulioJu JulioJu mentioned this pull request Apr 12, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fbricon fbricon fbricon requested changes

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

Successfully merging this pull request may close these issues.
2 participants
@yaohaizh @fbricon