Implement linkedEditingRange (experimental) #1022

hrsh7th · 2021-01-05T10:27:07Z

This PR aims to support textDocument/linkedEditingRange.

This implementations uses nvim_buf_set_extmarks in neovim and prop_add in vim (this is polyfilled by vim-vital-vs).

Notable changes

Replace `TextEdit` implementation to `vim-vital-vs`'s one.

This change is needed to preserve extmarks or text-props because the current implementation always applies line-wise changes.
The extmarks and text-props are shifted to follow other text changes automatically so we should apply TextEdit as char-wise changes.

~~Currently, vim-vital-vs will support char-wise implementation only when available nvim_buf_set_text.~~
~~I sent an issue to vim for supporting it... vim/vim#7617~~

Now, vim-vital-vs supports vim/nvim both.

Use `vim-vital-vs`'s `TextMark` implementation.

~~It is polyfill for nvim_buf_set_extmarks and prop_add and it respects the Range defined in LSP.~~

Now, vim-vital-vs use vim's position instead of Range.

How to test this?

Use latest neovim (it has `nvim_buf_set_text).
npm install -g vscode-langservers-extracted
Add vscode-html-language-server by lsp#register.
Open foo.html and edit tag name.

    call lsp#register_server({
    \   'name': 'vscode-html-language-server',
    \   'cmd': { -> ['vscode-html-language-server', '--stdio'] },
    \   'allowlist': ['html', 'typescriptreact', 'javascriptreact'],
    \   'initialization_options': {
    \     'embeddedLanguages': {
    \       'css': v:true,
    \       'html': v:true,
    \       'javascript': v:true,
    \     }
    \   }
    \ })

TODO

Provide mapping for d and c normal-mode commands
Add documents
Add API checks (nvim_buf_set_extmarks etc...)
Fix CI

DEMO

Kapture.2021-01-05.at.19.30.50.mp4

autoload/lsp/internal/linked_editing_range.vim

hrsh7th · 2021-01-05T10:40:43Z

@prabirshrestha If we can't accept external vital module dependencies, feel free to close this PR.

autoload/lsp/internal/linked_editing_range.vim

hrsh7th · 2021-01-05T18:02:36Z

Hm... CI failed by vital files...

prabirshrestha · 2021-01-06T00:10:44Z

@hrsh7th I don't mind external dependencies as long it ships with vim-lsp so that for a consumer there is only one plugin. There are few requirements.

License should be compatible.
Update https://github.com/prabirshrestha/vim-lsp/blob/master/LICENSE-THIRD-PARTY
should be lightweight

vim-lsp was one of my first plugins and was the plugin I learnt to built vim plugins. So I'm all for better refactors as the code was written long time back as long we don't break others. We should avoid breaking changes if possible.

I do like how vim-lamp organizes so we could slowly migrate to that style but I'm not sure what your plans are for vim-lamp. One big difference is I do want to use callbag instead of promises as they are more powerful to expose async apis.

I have been trying to add popup from vitial.vim too. vim-jp/vital.vim#748

One open question would be is where is the source of truth for lsp vital.vim be?
this might also help with CI or not sure if that is the vital standard. another option is to have different lint rules for different folder and run review dog twice with different configs. what about tests?

by the way these are awesome changes! I will be looking at the PR over the next few days.

hrsh7th · 2021-01-06T02:16:28Z

Thank you for your comments.

I don't mind external dependencies as long it ships with vim-lsp so that for a consumer there is only one plugin. There are few requirements.

Thank you. I will update the license.

I do like how vim-lamp organizes so we could slowly migrate to that style but I'm not sure what your plans are for vim-lamp. One big difference is I do want to use callbag instead of promises as they are more powerful to expose async apis.

The reasons why I wrote vim-lamp is that the below.

I don't like callback style async API

But currently, vim-lsp has callbag style API so It already solved for now.

I want to have sandbox environment for consideration with try-and-error to finding better implementations.

In fact, I have ported some improvements to vim-lsp if I found it in try-and-error.
Now, I'm wondering to dispose of vim-lamp or not because I've got the general pattern to implement LSP plugins

One open question would be is where is the source of truth for lsp vital.vim be?
this might also help with CI or not sure if that is the vital standard. another option is to have different lint rules for different folder and run review dog twice with different configs. what about tests?

I agree with you. To ship LSP related features with original vital.vim will be an ideal solution.
But it will be hard work so I still keep it as an external vital module, I think.

Another point.
In my thoughts, it would be very good if we split LSP and Editor features.

For example, the linkedEditingRange implementation already exists in this PR.
It is a very common implementation because it needs only Range[] but now, it depends lsp#get_allowed_servers() etc.

If we provide an API as below, it would be very flexible.

let s:provider = {}

function! s:provider.capable(context) abort
  return index(['typescript'], a:context.filetype) && has('nvim')
endfunction

function! s:provider.do(context, params) abort
  " we get the ranges from nvim's `treesitter` API for renaming local variable identifiers.
endfunction

call lsp#features#linked_editing_range#add_provider(s:provider)

If you interest it, it can be including vim-lsp 2021.

hrsh7th · 2021-01-06T08:38:48Z

I found the way to support vim/nvim both.

The way is using substitute command for applying TextEdit.

…n instead of LSP position)

prabirshrestha · 2021-01-07T17:42:20Z

Even I'm not fan of callback. Hence instead of adding more features I spent my time working on observable style apis with callbag.

+1 for LSP/Editor features. I have always wanted easier unit testing and better separation.

I didn't completely understand the provider refactor your proposed so a sample PR even if it doesn't work but just with comments might help here. For example how would one register a documentSymbol provider. I would want to support the current mechanism by showing it in the quickfix but also want to add other featurs such as show in treeview so can navigate differntly, support custom operators so can do something like select inside function, select around function, select inside class, select inside block and so on, implement something like goto type that is similar to vscode but uses quickpick. What does registering a provider mean for documentsymbol?

performance seems the number one feature request for 2021 so we need to make sure new pattern allows us to swap certain bottlenecks with lua. I know autocomplete is a huge bottleneck so we could start with refactoring completion as an example for provider. some language servers uses utf-8 while by default most support utf-16. does it make sense to have utils functions aware of this so it picks the right offset? At least I haven't seen utf-16 being the botteleneck currently.

As for vital I was thinking if the source of truth should be in vim-lsp instead of vim-lamp but that would depend the goals of vim-lamp.

hrsh7th · 2021-01-08T07:25:25Z

As for vital I was thinking if the source of truth should be in vim-lsp instead of vim-lamp but that would depend the goals of vim-lamp.

I haven't advertised vim-lamp at this time because it's just my sandbox/experimental environment (it often has a destructive change and probably won't work on windows).

But I think https://github.com/hrsh7th/vim-vital-vs can be our source of truth.
I already used it on the vim-lamp and vim-vsnip.

I didn't completely understand the provider refactor your proposed so a sample PR even if it doesn't work but just with comments might help here. For example how would one register a documentSymbol provider. I would want to support the current mechanism by showing it in the quickfix but also want to add other featurs such as show in treeview so can navigate differntly, support custom operators so can do something like select inside function, select around function, select inside class, select inside block and so on, implement something like goto type that is similar to vscode but uses quickpick. What does registering a provider mean for documentsymbol?

I think document_symbol_provider will be just a feature that returns document_symbols as callbag stream.
It has a benefit that is we can add the document_symbol_provider from ctags or tree-sitter etc (it does not depends on LSP).

The UI uses the features. The features does not depends on LSP but LSP can be connected to the features.

performance seems the number one feature request for 2021 so we need to make sure new pattern allows us to swap certain bottlenecks with lua. I know autocomplete is a huge bottleneck so we could start with refactoring completion as an example for provider. some language servers uses utf-8 while by default most support utf-16. does it make sense to have utils functions aware of this so it picks the right offset? At least I haven't seen utf-16 being the botteleneck currently.

OK. I will try to refactor the completion as the provider pattern.
I think we don't have to care about utf8/utf16 index basically. we just handle the Position as character boundary... maybe.

prabirshrestha · 2021-01-08T19:09:57Z

@hrsh7th looking for the new refactors.

One thing we should try to do is add proper license to vital-vs. In some countries public domain and other license are not valid. So using a well known such as mit or apache is better. Once you have contributors and you modify the license you need to reach out to old contributors and it is a hassle to change the license so would be good to get this right in the first go.

prabirshrestha

added comments.

prabirshrestha · 2021-01-08T19:13:47Z

autoload/lsp/utils/range.vim

 endfunction

+" Returns the range contains specified position or not.
+function! lsp#utils#range#_contains(range, position) abort


seems like this can be used to solve this issue now. #888

prabirshrestha · 2021-01-08T19:14:57Z

autoload/lsp/internal/linked_editing_range.vim

+    \     lsp#callbag#pipe(
+    \         lsp#callbag#fromEvent(['InsertEnter']),
+    \         lsp#callbag#filter({ -> g:lsp_linked_editing_range_enabled }),
+    \         lsp#callbag#flatMap({ -> s:request_sync() }),


is there a reason it needs to be sync?

I afraid to lost keypress if the user presses quickly. So I'm choosing it to be sync.

But if text-prop/extmark is working correct, we can choose debouncing it.

prabirshrestha · 2021-01-08T19:17:00Z

autoload/lsp/internal/linked_editing_range.vim

+    \         lsp#callbag#subscribe({ -> s:clear() })
+    \     ),
+    \     lsp#callbag#pipe(
+    \         lsp#callbag#fromEvent(['TextChanged', 'TextChangedI', 'TextChangedP']),


i think for now it is ok. but TextChangedP is not available in all versions. One easy fix would be to check for TextChangedP support in s:enabled

this will also hopefully encourage folks to migrate to newer versions.

autoload/lsp/internal/linked_editing_range.vim

prabirshrestha · 2021-01-09T02:42:06Z

autoload/lsp/internal/linked_editing_range.vim

+
+function! s:sync() abort
+    let l:bufnr = bufnr('%')
+    if s:state['bufnr'] != l:bufnr


for callbags this is usually not preferred. you can instead use switchMap and takeUntil similar to this https://github.com/prabirshrestha/vim-lsp/blob/master/autoload/lsp/internal/document_highlight.vim

Feel free to change or ignore it for now and I can refactor it later when it is merged.

I usually use RxJS or other Rx libraries does to understand callbag.

High level these are what it means.

switchMap -> when ever you have a next, cancel the existing one and start a new one. Good example for this is autocomplete where if I type he and then type hello I want to cancel he and automatically start hello.

takeUntil -> this is use for cancellation. basically it means when a next is called in takeuntil cancel the stream.

But since you are using wait what you have should be ok.

Some good reading here. https://medium.com/@benlesh/rxjs-dont-unsubscribe-6753ed4fda87

Thank you. I learned a lot.

hrsh7th · 2021-01-11T17:13:17Z

Thank you for your review.

I will update it later.
But I'm now tackling to solve the TextMark problem.

- Use lsp#callbag#filter - s:request_sync returns value instead of stream - Add workaround for extmark - Update vital modules

hrsh7th · 2021-01-12T16:23:50Z

doc/vim-lsp.txt

+	nmap <buffer> d <Plug>(lsp-linked-editing-range-prepare)<Plug>(vimrc-d)
+	xmap <buffer> d <Plug>(lsp-linked-editing-range-prepare)<Plug>(vimrc-d)
+	nmap <buffer> c <Plug>(lsp-linked-editing-range-prepare)<Plug>(vimrc-c)
+	xmap <buffer> c <Plug>(lsp-linked-editing-range-prepare)<Plug>(vimrc-c)


Sorry. I noticed it does not work.

But "nnoremap d lsp#internal#linked_editing_range#prepare() . 'd'" is works fine.

I don't know how to expose mapping function in vim-lsp. (lsp#mapping#linked_editing_range_prepare()?)

hrsh7th · 2021-01-22T12:46:42Z

neovim's extmark has bug (may be).
neovim/neovim#13816

stale · 2021-03-26T10:34:42Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Implement linkedEditingRange

9c99f91

hrsh7th commented Jan 5, 2021

View reviewed changes

autoload/lsp/internal/linked_editing_range.vim Outdated Show resolved Hide resolved

Update vital modules

59469e5

hrsh7th force-pushed the linked-editing-range branch from f01d0bb to 59469e5 Compare January 5, 2021 10:41

hrsh7th commented Jan 5, 2021

View reviewed changes

autoload/lsp/internal/linked_editing_range.vim Show resolved Hide resolved

hrsh7th commented Jan 5, 2021

View reviewed changes

autoload/lsp/internal/linked_editing_range.vim Outdated Show resolved Hide resolved

hrsh7th commented Jan 5, 2021

View reviewed changes

autoload/lsp/internal/linked_editing_range.vim Outdated Show resolved Hide resolved

Improve implementation and add docs

e5f975e

hrsh7th force-pushed the linked-editing-range branch from 64948a6 to e5f975e Compare January 5, 2021 17:18

Update vital modules

2c6ada3

hrsh7th added 2 commits January 6, 2021 03:16

Check availability

a513c3a

Check feature flags

761ac6d

hrsh7th marked this pull request as ready for review January 5, 2021 18:35

hrsh7th force-pushed the linked-editing-range branch from 34d81d2 to 4545247 Compare January 6, 2021 09:19

Always use method in TextEdit

7db44b6

hrsh7th force-pushed the linked-editing-range branch from 4545247 to 7db44b6 Compare January 6, 2021 09:22

Fix for undo history

71b1c56

hrsh7th force-pushed the linked-editing-range branch from 8b6f9c6 to 71b1c56 Compare January 6, 2021 09:53

Follow vital module updates (The TextMark has now using vim's positio…

4362c7d

…n instead of LSP position)

prabirshrestha requested changes Jan 9, 2021

View reviewed changes

prabirshrestha mentioned this pull request Jan 9, 2021

add support for linkedEditingRange for react tsx tags typescript-language-server/typescript-language-server#179

Closed

hrsh7th added 2 commits January 13, 2021 01:11

- Use switchMap instead of flatMap

1fcd013

- Use lsp#callbag#filter - s:request_sync returns value instead of stream - Add workaround for extmark - Update vital modules

Check TextChangedP

dc67af1

hrsh7th commented Jan 12, 2021

View reviewed changes

stale bot added the wontfix label Mar 26, 2021

stale bot closed this Apr 3, 2021

Implement linkedEditingRange (experimental) #1022

Implement linkedEditingRange (experimental) #1022

Uh oh!

Conversation

hrsh7th commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notable changes

Replace TextEdit implementation to vim-vital-vs's one.

Use vim-vital-vs's TextMark implementation.

How to test this?

TODO

DEMO

Uh oh!

Uh oh!

hrsh7th commented Jan 5, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hrsh7th commented Jan 5, 2021

Uh oh!

prabirshrestha commented Jan 6, 2021

Uh oh!

hrsh7th commented Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hrsh7th commented Jan 6, 2021

Uh oh!

prabirshrestha commented Jan 7, 2021

Uh oh!

hrsh7th commented Jan 8, 2021

Uh oh!

prabirshrestha commented Jan 8, 2021

Uh oh!

prabirshrestha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hrsh7th commented Jan 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hrsh7th commented Jan 22, 2021

Uh oh!

stale bot commented Mar 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hrsh7th commented Jan 5, 2021 •

edited

Loading

Replace `TextEdit` implementation to `vim-vital-vs`'s one.

Use `vim-vital-vs`'s `TextMark` implementation.

hrsh7th commented Jan 6, 2021 •

edited

Loading