Fix CompleteDone #637
Merged
Fix CompleteDone #637
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
hrsh7th
commented
Dec 28, 2019
hrsh7th
commented
Dec 28, 2019
hrsh7th
force-pushed
the
fix-completion-edits
branch
2 times, most recently
from
de057cf
to
0fac572
Compare
|
Thanks for your advice. Could you tell me the #391's check points. |
hrsh7th
commented
Dec 28, 2019
| let s:user_data_filtertext_key = 'vim-lsp/filterText' | ||
| let s:user_data_server_name_key = 'vim-lsp/serverName' | ||
| let s:user_data_completion_item_key = 'vim-lsp/completionItem' | ||
|
|
There was a problem hiding this comment.
textEdit/additionalTextEdits/insertFormat are used by CompleteDone.
New implementation provides completionItem instead.
There was a problem hiding this comment.
Do we still we need s:user_data_insert_start_key and s:user_data_filtertext_key now? Can we not get it from vim-lsp/completionItem?
Less state we need to maintain or having just a single source of truth would be better.
There was a problem hiding this comment.
On purpose, I didn't change the omnifunc behavior in this PR.
But I think your advice is right. those keys can removed.
hrsh7th
commented
Dec 28, 2019
hrsh7th
commented
Dec 28, 2019
| return '' | ||
| endif | ||
|
|
||
| let l:completion_item = s:resolve_completion_item(l:completion_item, l:server_name) |
There was a problem hiding this comment.
It is an additional feature.
Run completionItem/resolve if it possible.
This enables auto importing when using typescript-language-server.
There was a problem hiding this comment.
This was one of the feature I was missing in vim-lsp. thanks for implementing this. 👏 👏 👏 . Tried this out and is awesome. Will save a lot of time.
There was a problem hiding this comment.
Hey! Is it possible to disable this feature somehow 😊? I'm using other tools to manage imports and such, and this is disrupting my current workflow a bit
There was a problem hiding this comment.
You can use let g:lsp_text_edit_enabled = v:false.
If you don't like this option, we need to implement a new feature.
|
I think it is ready to merge. |
|
Can we test with multibyte chars? |
|
OK, I try to test it. |
hrsh7th
commented
Dec 29, 2019
autoload/lsp/ui/vim/completion.vim
Outdated
| call s:expand_text_simply(v:completed_item.word) | ||
| elseif exists('g:lsp_snippet_expand') && len(g:lsp_snippet_expand) > 0 | ||
| " other snippet integartion point. | ||
| call g:lsp_snippet_expand[0](l:expand_text) |
There was a problem hiding this comment.
This is a new option.
I should create docs to it.
There was a problem hiding this comment.
yes. we should document this.
|
I try to create tests. |
|
By test I mean it can be manual adhoc verification. Just make sure to have some multi Ute characters and make sure autocomplete works as expected. |
|
I try to check the intelephense response on below buffer. /* あいうえお */ $test->#<C-x><C-o><Tab>So the cursor position will be the wrong place, I try to fix it. |
hrsh7th
force-pushed
the
fix-completion-edits
branch
from
5f1e77f
to
db6e8fc
Compare
|
Fixed the multi-byte problem. I checked below servers. (with enabled |
|
My checked case. # simple snippet supports with multibyte chars.
class Test {
public function set_value($value) {
// ...
}
}
$test = new Test();
/* あいうえお */ $test->#<C-x><C-o><C-n><Tab>
↓↓↓
/* あいうえお */ $test->set_value(#)# complex text_edit to correct indent-size.
<html>
<head>
</#<C-x><C-o><C-n><Tab>>
↓↓↓
</head#># the case of exists character after cursor.
<div class="あいうえお" aria-autocomp#<C-x><C-o><C-n><Tab>>
↓↓↓
<div class="あいうえお" aria-autocomplete="#"> |
|
I checked multibyte chars. |
|
I fixed the branch policies. Everything is green now. Will look at the PR. |
prabirshrestha
requested changes
Dec 29, 2019
There was a problem hiding this comment.
Thanks for this feature. I love that the auto import for typescript is working and that it also fixes a bunch of other bugs others have complained.
I have left some comments. Most of them now seems minor comments so we should be ok.
Given that we don't have full integration test can you make sure to add some manual test cases as comments in the test folder. In the future I would like to have full tests going so we can prevent regressions. Also include test cases for multibyte chars as comments.
I did file an issue in vim for allowing us to store any object in user_data. vim/vim#5412 Let us see how this goes.
| let s:user_data_filtertext_key = 'vim-lsp/filterText' | ||
| let s:user_data_server_name_key = 'vim-lsp/serverName' | ||
| let s:user_data_completion_item_key = 'vim-lsp/completionItem' | ||
|
|
There was a problem hiding this comment.
Do we still we need s:user_data_insert_start_key and s:user_data_filtertext_key now? Can we not get it from vim-lsp/completionItem?
Less state we need to maintain or having just a single source of truth would be better.
autoload/lsp/ui/vim/completion.vim
Outdated
| \ 'on_notification': function(l:ctx.callback, [], l:ctx) | ||
| \ }) | ||
|
|
||
| if empty(l:ctx.response) |
There was a problem hiding this comment.
if empty(l:ctx['response'])
autoload/lsp/ui/vim/completion.vim
Outdated
| endfunction | ||
|
|
||
| " | ||
| " Remove inserted text duratin completion. |
autoload/lsp/ui/vim/completion.vim
Outdated
| if has_key(a:completion_item, 'textEdit') | ||
| let l:range.start.character = min([ | ||
| \ l:range.start.character, | ||
| \ a:completion_item.textEdit.range.start.character |
There was a problem hiding this comment.
a:completion_items['textEdit']['range']['start']['character']
autoload/lsp/ui/vim/completion.vim
Outdated
| \ ]) | ||
| endif | ||
|
|
||
| " Remove. |
There was a problem hiding this comment.
can we be more explicit in the comment on what do we actually remove?
|
I didn't implement commit chars for now. I don't know how to implement it. |
|
new integration well worked. |
|
Bram suggested using key at vim/vim#5412 (comment) instead of Here is how I think we can do but my issue is what if there are multiple lang servers for the same buffer. let s:user_data = {} // { 'vim-lsp/1': { 'serverName': 'serverName', 'completeItem': 'completeItem' } }
let s:user_data_index = 0
function! s:add_user_data(data) abort
let s:user_data_index += 1
let s:user_data[s:user_data_index'] = a:data
return 'vim-lsp/' + s:user_data_index " return the key
endfunction
// reset s:user_data and s:user_data_index when CompleteDone and popup menu is set. |
- Rename expand_text_simply -> simple_expand_text - to_col -> _lsp_to_vim - Rename user_data key
hrsh7th
force-pushed
the
fix-completion-edits
branch
from
4d9a358
to
1800e56
Compare
| End | ||
|
|
||
| It should expand simple snippet with multibyte chars | ||
| Skip This test needs asynchronous process and snippetSupport=true |
There was a problem hiding this comment.
Should we create a wait(timeout) function? Something like how we do in asyncwait. https://github.com/prabirshrestha/async.vim/blob/master/autoload/async/job.vim#L240-L253
then we can actually run the test in ci.
If we do want to fix vim-themis I pasted some notes at thinca/vim-themis#61
There was a problem hiding this comment.
while-sleep is not interrupted other any process in vim e.g. feedkeys/timer etc.
So we want to done feature like mocha, I think.
It example test
let ctx = {}
let ctx.done = themis#async()
let ctx.changedtick = getbufvar('%', '&changedtick')
function! ctx.callback(timer)
if self.changedtick == getbufvar('%', '&changedtick')
return
endif
" some tests...
call self.done()
endfunction
call some#async#job()
call timer_start(100, { timer -> ctx.callback(timer) }, { 'repeat': -1 })
EndThere was a problem hiding this comment.
It is good that you still have the tests that is skipped so at least we can manually verify in case we change anything in completion engine in future
autoload/lsp/ui/vim/completion.vim
Outdated
| let l:ctx = {} | ||
| let l:ctx['response'] = {} | ||
| function! l:ctx['callback'](data) abort | ||
| let l:self['response'] = a:data.response |
autoload/lsp/ui/vim/completion.vim
Outdated
| @@ -134,13 +134,14 @@ function! s:resolve_completion_item(completion_item, server_name) abort | |||
| let l:ctx = {} | |||
| let l:ctx['response'] = {} | |||
| function! l:ctx['callback'](data) abort | |||
| let l:self['response'] = a:data.response | |||
| let l:self['response'] = a:data['response'] | |||
| endfunction | |||
|
|
|||
| call lsp#send_request(a:server_name, { | |||
There was a problem hiding this comment.
should we have a try..catch here?
There was a problem hiding this comment.
Oh! Thanks for your review.
Yes, we should add try-catch here.
hrsh7th
force-pushed
the
fix-completion-edits
branch
from
b70cfbe
to
fd78bdd
Compare
|
@ hrsh7th This is looking really good. I tried it and seems to work well on Mac for I do seems issues on Windows though and this seems like it is due to line endings. Mac doesn't have this issue.
This is for both mac and windows:
logAlso let me know if there is anything you are planning to add to this PR or in future PRs. |
This was referenced Jan 2, 2020
|
Signature help problem can be solved in #638 maybe. I have no plan adding anything to this PR.
|
prabirshrestha
approved these changes
Jan 2, 2020
|
I'm merging this, since it seems great and is working. Thanks for the contribution and patience with the PR. Looking forward to see more PRs from you. Let me know if there is any reason why vim-lsp isn't enough and you are working on another client. Or if there is something we should do here to improve. |
kg8m
added a commit
to kg8m/dotfiles
that referenced
this pull request
Jan 2, 2020
It has been fixed by prabirshrestha/vim-lsp#637.
|
@hrsh7th Seems like on windows we are not getting additionalTextEdits. Windows[
"<---",
1,
"typescript-language-server",
{
"response": {
"id": 6,
"jsonrpc": "2.0",
"result": {
"label": "createLspConnection",
"commitCharacters": [
".",
",",
"("
],
"data": {
"file": "d:\\tmp\\github\\typescript-language-server\\server\\src\\cli.ts",
"entryNames": [
{
"name": "createLspConnection",
"source": "d:/tmp/github/typescript-language-server/server/src/lsp-connection"
}
],
"offset": 2,
"line": 13
},
"sortText": "�5",
"kind": 3,
"detail": "Auto import from './lsp-connection'\nfunction createLspConnection(options: IServerOptions): lsp.IConnection",
"command": {
"arguments": [
"d:\\tmp\\github\\typescript-language-server\\server\\src\\cli.ts",
[
{
"description": "Import 'createLspConnection' from module \"./lsp-connection\"",
"changes": [
{
"fileName": "d:/tmp/github/typescript-language-server/server/src/cli.ts",
"textChanges": [
{
"end": {
"offset": 1,
"line": 12
},
"newText": "import { createLspConnection } from './lsp-connection';\r\n",
"start": {
"offset": 1,
"line": 12
}
}
]
}
]
}
]
],
"title": "",
"command": "_typescript.applyCompletionCodeAction"
},
"insertTextFormat": 2
}
},
"request": {
"id": 6,
"jsonrpc": "2.0",
"method": "completionItem/resolve",
"params": {
"label": "createLspConnection",
"commitCharacters": [
".",
",",
"("
],
"data": {
"file": "d:\\tmp\\github\\typescript-language-server\\server\\src\\cli.ts",
"entryNames": [
{
"name": "createLspConnection",
"source": "d:/tmp/github/typescript-language-server/server/src/lsp-connection"
}
],
"offset": 2,
"line": 13
},
"sortText": "�5",
"kind": 3,
"insertTextFormat": 2
}
}
}
]Mac[
"<---",
1,
"typescript-language-server",
{
"response": {
"id": 6,
"jsonrpc": "2.0",
"result": {
"label": "createLspConnection",
"commitCharacters": [
".",
",",
"("
],
"data": {
"file": "/Users/prabir/tmp/typescript-language-server/server/src/cli.ts",
"entryNames": [
{
"name": "createLspConnection",
"source": "/Users/prabir/tmp/typescript-language-server/server/src/lsp-connection"
}
],
"offset": 2,
"line": 12
},
"additionalTextEdits": [
{
"range": {
"end": {
"character": 0,
"line": 11
},
"start": {
"character": 0,
"line": 11
}
},
"newText": "import { createLspConnection } from './lsp-connection';\n"
}
],
"sortText": "�0",
"kind": 3,
"detail": "Auto import from './lsp-connection'\nfunction createLspConnection(options: IServerOptions): lsp.IConnection",
"insertTextFormat": 2
}
},
"request": {
"id": 6,
"jsonrpc": "2.0",
"method": "completionItem/resolve",
"params": {
"label": "createLspConnection",
"commitCharacters": [
".",
",",
"("
],
"data": {
"file": "/Users/prabir/tmp/typescript-language-server/server/src/cli.ts",
"entryNames": [
{
"name": "createLspConnection",
"source": "/Users/prabir/tmp/typescript-language-server/server/src/lsp-connection"
}
],
"offset": 2,
"line": 12
},
"sortText": "�0",
"kind": 3,
"insertTextFormat": 2
}
}
}
] |
|
Seems like path issue on windows on the typescript-language-server. More details at typescript-language-server/typescript-language-server#135 so this isn't vim-lsp bug. |
|
Thanks for your deep investigate! |
|
Seems like we still have one bug in apply text edits but not related to this PR as it has always been here. For this file even in windows the format is Unix but language server seems to return \r\n because it assumes we are on windows and the text edit works but now all the edited line has ^M because it is Unix.
|
hrsh7th
added a commit
to hrsh7th/vim-lsp
that referenced
this pull request
Jan 4, 2020
* Move some codes to completion.vim that related to handling CompleteDone. * Add abort * Fix test * Add l: prefix to self vars * Disable unused vars * Restore position * Support snippet simple case and vim-lsp-snippets * Support g:lsp_text_edit_enabled * Fix for v:null results * Fix multi-byte chars * Add document * Fix for vim-lsp-snippets for now * Prepare to future improvements * More clalify line comment * User get_user_data instead of extract_user_data * Always use [''] pattern for accessing dictionary property * Fix misspelling * More strict check for completionProvider.resolveProvider * Fix obj.key to obj['key'] * Store completed_item id to the user_data * Fix omni tests * Apply for the review - Rename expand_text_simply -> simple_expand_text - to_col -> _lsp_to_vim - Rename user_data key * Add example tests * Add timeout to completionItem/resolve in completion.vim * Fix timeout feature * Add timeout log * Fix documents
williamboman
added a commit
to williamboman/vim-lsp
that referenced
this pull request
Jan 24, 2020
* upstream/master: (37 commits) Treat severity=1 when not specified (prabirshrestha#686) Use prefix filter when does not specified (prabirshrestha#678) Check capability is dict (prabirshrestha#675) Check textEdit is not null Add comment Avoid clearing managed user_data unexpectedly Fix completion for clangd (prabirshrestha#665) Add missing function (prabirshrestha#667) Improve code action (prabirshrestha#663) Ignore cmdwin (prabirshrestha#664) Cache semantic tokens (prabirshrestha#635) Fixup lsp_location_item_to_vim (prabirshrestha#657) disable semantic highlight by default (prabirshrestha#656) Improve signature help with lexima.vim (prabirshrestha#638) show long of starting server Fix prabirshrestha#651 (prabirshrestha#652) do not echo Retrieving signature help Improve user_data format (prabirshrestha#649) Fix CompleteDone (prabirshrestha#637) add lsp#_new_command() and lsp#_last_command() (prabirshrestha#648) ...
itchyny
reviewed
Jan 28, 2020
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR aims to fix
textEditrelated problems.TODO
1st step(no breaking change).
CompleteDonehandling from omni.vimvim-lsp-snippetsg:lsp_text_edit_enabled = 0[x] Supportg:lsp_insert_text_enabled = 0// The PR can be merged at this timing.
2nd step(has breaking change).