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

Fix broken snippet completion after itemDefault implementation #2561

Merged
merged 1 commit into from
Mar 31, 2023

Conversation

JessicaJHee
Copy link
Contributor

Fixes #2560

@rgrunber
Copy link
Contributor

rgrunber commented Mar 28, 2023

So basically, we just forgot to set the new textEditText (required for the defaultItems feature) for snippets ?

What about JavadocCompletionProposal ? I don't see it here but definitely called at https://github.com/eclipse/eclipse.jdt.ls/blob/335ad830a64e04c8ef4d633203c72c777dea84e8/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/CompletionHandler.java#L228 . It doesn't set a text edit, but in the absence of one, the label itself is used.

@JessicaJHee
Copy link
Contributor Author

I believe because we set the textEdit here:

https://github.com/eclipse/eclipse.jdt.ls/blob/335ad830a64e04c8ef4d633203c72c777dea84e8/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/contentassist/JavadocCompletionProposal.java#L114

We don't need to handle this case because it will prioritize the use of textEdit and not insertText. (https://code.visualstudio.com/api/references/vscode-api#CompletionItem) The problem with the other cases is that we let the client side handle the completion with insertText since we don't set textEdit.

@rgrunber rgrunber requested a review from fbricon March 28, 2023 21:09
@rgrunber
Copy link
Contributor

Ok, and the snippets don't seem to ever set a text edit so we never have to worry about that. They just set insertText, which is fine, because that can co-exist with textEditText just like it does for non-snippet items.

@fbricon
Copy link
Contributor

fbricon commented Mar 29, 2023

Can we have some unit test showcasing the fix fixes the issue? Testing something like

public class Foo {
    void sysop(){}
    
    public static void main(Object[]...args) {
        new Foo().syso| //should return 2 completion items: sysop() and sysout postfix
    }
}

Signed-off-by: Jessica He <jhe@redhat.com>
@fbricon fbricon merged commit 637b127 into eclipse-jdtls:master Mar 31, 2023
@fbricon
Copy link
Contributor

fbricon commented Mar 31, 2023

Thanks @JessicaJHee!

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.

[regression] Postfix completion overwrites postfixed content
3 participants