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: snippet suggestion does not replace the input string #5931

Merged
merged 1 commit into from
Aug 23, 2019

Conversation

yeweiasia
Copy link
Contributor

@yeweiasia yeweiasia commented Aug 13, 2019

What it does

fix #5926: snippet suggestion does not replace the input string

How 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".
image
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
autocompletion1
6.with this patch "if" will replaced by suggested item body.
autocompletion2

Review checklist

Reminder for reviewers

@yeweiasia
Copy link
Contributor Author

#5926

@vince-fugnitto
Copy link
Member

@yeweiasia can you provide comment on the PR template, mainly 'what it does' and 'how to test'?

@vince-fugnitto vince-fugnitto added editor issues related to the editor monaco issues related to monaco labels Aug 13, 2019
@yeweiasia
Copy link
Contributor Author

@vince-fugnitto comment updated

@akosyakov akosyakov requested review from vince-fugnitto and a team and removed request for vince-fugnitto August 14, 2019 06:59
@akosyakov
Copy link
Member

@akosyakov akosyakov changed the title fix #5926 fix: snippet suggestion does not replace the input string Aug 14, 2019
@yeweiasia
Copy link
Contributor Author

ok, I will investigate how vscode implemented in next few days.

Copy link
Contributor

@JanKoehnlein JanKoehnlein left a 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
Copy link
Contributor Author

autocompletion3
@akosyakov PR updated.

@akosyakov
Copy link
Member

@yeweiasia Does it address #4208 as well? Could you update the description with fix if so.

@akosyakov
Copy link
Member

I've updated a description with a link to the CQ.

@yeweiasia
Copy link
Contributor Author

@akosyakov I think this can be implemented in my own way, I will rewrite and make another PR to resolve this bug

@yeweiasia
Copy link
Contributor Author

@yeweiasia Does it address #4208 as well? Could you update the description with fix if so.

I will test after rewriting

@akosyakov
Copy link
Member

@akosyakov I think this can be implemented in my own way, I will rewrite and make another PR to resolve this bug

What for? It will make it harder to keep in sync for future updates. We can wait for a CQ.

@yeweiasia
Copy link
Contributor Author

yeweiasia commented Aug 16, 2019

@akosyakov I have implemented another version of this feature, #5963
that version is more concise when dealing with pattern, I just find out the word at the cursor position rather than find out all offsets of the word in whole line. I think find out all word in whole line make no sense in case of snippet code suggestion.
for #4208, I have test the latest version of groovy plugin 0.1.2, I am not able to reproduce the problem mentioned in the issue. typing "thr" will not trigger auto completion , by searching the repository of code-groovy, no snippet named "thread" can be found.

@akosyakov
Copy link
Member

@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.

@yeweiasia
Copy link
Contributor Author

yeweiasia commented Aug 20, 2019

@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>
@akosyakov
Copy link
Member

akosyakov commented Aug 20, 2019

@yeweiasia at least we've learned why it is implemented so 👍

@BroKun @vince-fugnitto Could you finish the review on this PR please?

@akosyakov
Copy link
Member

CQ was marked as ready for checkin, @BroKun @vince-fugnitto please merge if you are happy after testing

Copy link
Member

@BroKun BroKun left a 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 :)

@BroKun BroKun merged commit 5339bef into eclipse-theia:master Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
editor issues related to the editor monaco issues related to monaco
Projects
None yet
Development

Successfully merging this pull request may close these issues.

snippet suggestion does not replace the input string
5 participants