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.

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

interface/class snippets should not be available inside method bodies #689

Merged
merged 1 commit into from
Jun 12, 2018

Conversation

snjeza
Copy link
Contributor

@snjeza snjeza commented Jun 11, 2018

Fixes #681

I have removed the class/interface snippets from the following contexts:

  • javadoc
  • package declaration
  • imports
  • method body

The class snippets is sometimes allowed in the method body such as:

public class Test {
	void test() {
		class InnerTest {
		}
	}
}
  • static (the class snippet is allowed, interface is not allowed)

I have also changed the snippets to not generate duplicated public keywords, as in the following place:

package org.sample;
public |

BTW all the client snippets have the same issue.

Signed-off-by: Snjezana Peco [email protected]

@snjeza snjeza requested review from gorkem and fbricon June 11, 2018 15:12
@snjeza
Copy link
Contributor Author

snjeza commented Jun 11, 2018

test this please

@snjeza
Copy link
Contributor Author

snjeza commented Jun 12, 2018

test this please

//This check might need to be pushed back to the different get*Snippet() methods, depending on future features
if (UNSUPPORTED_RESOURCES.contains(cu.getResource().getName())) {
return Collections.emptyList();
}
if (!accept(cu, completionContext, true)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

L65-L70 need to be moved inside getClassSnippet.

if accept returns false, then you're not even trying interfaces next

if (classSnippet != null) {
res.add(classSnippet);
}
CompletionItem interfaceSnippet = getInterfaceSnippet(cu);
if (!accept(cu, completionContext, false)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

L80-L85 need to be moved to getInterfaceSnippet

@@ -109,6 +108,7 @@ public boolean isCanceled() {
try {
unit.codeComplete(offset, collector, subMonitor);
proposals.addAll(collector.getCompletionItems());
proposals.addAll(SnippetCompletionProposal.getSnippets(unit, collector.getContext(), subMonitor));
Copy link
Contributor

Choose a reason for hiding this comment

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

the problem here is the proposals ignore the currently typed text, i.e if you type m in the class body, you'd still get the class and interface snippets proposals. While vscode luckily filters them out, other clients won't necessarily do it.

@snjeza
Copy link
Contributor Author

snjeza commented Jun 12, 2018

@fbricon I have updated the PR.

@fbricon fbricon merged commit 8e16195 into eclipse-jdtls:master Jun 12, 2018
@fbricon
Copy link
Contributor

fbricon commented Jun 12, 2018

Thanks @snjeza!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants