-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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: snippet suggestion does not replace the input string #5931
Conversation
@yeweiasia can you provide comment on the PR template, mainly 'what it does' and 'how to test'? |
@vince-fugnitto comment updated |
You can have a look how VS Code does it here: https://github.com/TypeFox/vscode/blob/monaco/0.18.0/src/vs/workbench/contrib/snippets/browser/snippetCompletionProvider.ts#L80-L140 |
ok, I will investigate how vscode implemented in next few days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely fixes the buggy behavior described in the issue, but I am inclined we should do more sophisticated word-wise matching as VS Code does.
|
@yeweiasia Does it address #4208 as well? Could you update the description with |
I've updated a description with a link to the CQ. |
@akosyakov I think this can be implemented in my own way, I will rewrite and make another PR to resolve this bug |
I will test after rewriting |
What for? It will make it harder to keep in sync for future updates. We can wait for a CQ. |
@akosyakov I have implemented another version of this feature, #5963 |
@yeweiasia if you are sure that your implementation is better than let's go with it. We can reconsider later to use VS Code implementation if it will be required. Please close this PR then. |
@akosyakov after trying to provide another implementation, It seems better to sync with vscode logic and I have fully synced with vscode impl :) |
fix snippet suggestion does not replace the input string fix snippet suggestion always displayed all items Signed-off-by: yewei <yeweiasia@gmail.com>
@yeweiasia at least we've learned why it is implemented so 👍 @BroKun @vince-fugnitto Could you finish the review on this PR please? |
CQ was marked as ready for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works nicely, @yeweiasia thanks :)
What it does
fix #5926: snippet suggestion does not replace the input string
checkin
readyHow to test
1.theia 0.9.0 with redhat.java-0.46.0.vsix
2.open a maven project and edit a java file
3.type keyword like "if".
4.select "ifelse" or other snippet suggestion.
5.without this patch whole suggested item body will be append to "if" which is demonstrated in #5926
6.with this patch "if" will replaced by suggested item body.
Review checklist
Reminder for reviewers