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

Bug fix in omnifunc #360

Closed
wants to merge 1 commit into from
Closed

Bug fix in omnifunc #360

wants to merge 1 commit into from

Conversation

girishji
Copy link
Contributor

@girishji girishji commented Jul 18, 2023

  1. LspOmniFunc() causes inserted text (when selecting from a popup) to
    appear in the wrong column. This happens only when using this
    function separately (through complete() function) in combination
    with other completion functions. When findstart=1, this function
    returns column width of 1 for tab (if tabstop is set to 8), instead
    of 8. To fix the problem I replaced use of charcol() with
    col(). :h complete-functions recommends using col().
    Normal users of omnifunc () should not notice any
    difference.

  2. Added a function to check if omnifunc is waiting for LSP response.
    This is helpful for writing adapters for external completion
    plugins out there.

Since LspOmniFunc() works correctly when used with
this PR is not essential. I leave it to the discretion of Yegappan to
decide whether to include or not.

I use this omnifunc in conjunction with other omnifunc's. I call
them first with findstart=1 and then again with findstart=0, then
combine the returned items and show them using complete().
I use them in async manner. The advantage of this change is that
now you can adapt this LSP to any external completion plugin
using few lines of stub code.

Here is how the bug shifts popup window (and inserts in wrong column):

image

M autoload/lsp/completion.vim

1) LspOmniFunc() causes inserted text (when selecting from a popup) to
   appear in the wrong column. This happens *only* when using this
   function separately (through `complete()` function) in combination
   with other completion functions. When findstart=1, this function
   returns column width of 1 for tab (if tabstop is set to 8), instead
   of 8. To fix the problem I replaced use of `charcol()` with
   `col()`. `:h complete-functions` recommends using `col()`.
   Normal users of omnifunc (<c-x><c-o>) should not notice any
   difference.

2) Added a function to check if omnifunc is waiting for LSP response.
   This is helpful for writing adapters for async completion
   plugins out there.

M  autoload/lsp/completion.vim
@yegappan
Copy link
Owner

yegappan commented Jul 18, 2023

Thanks for the patch. Do you have a minimal script that reproduces this issue? Are you referring to the popup window showing up in column 1 instead of column 8 in your picture?

Note that the completion function should return a column number (byte index) and not a virtual column number. So a tab character is counted as one byte and the index will be 1 for a tab character.

@@ -472,6 +467,13 @@ def g:LspOmniFunc(findstart: number, base: string): any
endif
enddef

# For plugins that implement async completion from various sources this function
# indicates if omnifunc is waiting for LSP response.
def g:LspOmniCompletePending(): bool
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update the doc with a description and use of this function?

@girishji
Copy link
Contributor Author

girishji commented Jul 18, 2023

Thanks for the patch. Do you have a minimal script that reproduces this issue? Are you referring to the popup window showing up in column 1 instead of column 8 in your picture?

Note that the completion function should return a column number (byte index) and not a virtual column number. So a tab character is counted as one byte and the index will be 1 for a tab character.

I made a mistake. I did not realize that startcol argument of complete() and column number returned by calling LspOmniFunc(1, '') differ by 1. When calling complete() you have to add +1 to the output of omnifunc to get the popup and inserted text aligned correctly. Initially I did not add +1 and it lead to some confusion (this PR is unnecessary). Here is the minimal script I used to debug this problem.

vim9script
setbufvar(bufnr(), '&completeopt', 'menuone,popup,noinsert,noselect')
setbufvar(bufnr(), '&completepopup', 'width:80,highlight:Pmenu,align:item')

def Completor()
    var startcol = g:LspOmniFunc(1, '') + 1 # +1 is important
    if startcol < 0
	return
    endif
    var line = getline('.')->strpart(0, col('.') - 1)
    var base = line->slice(startcol - 1)
    var items = g:LspOmniFunc(0, base)
    if !items->empty()
	items->complete(startcol)
    endif
enddef
autocmd TextChangedI <buffer> call Completor()

Turn off the autocomplete in .vimrc:

autocmd VimEnter * call LspOptionsSet({ autoComplete: false })

GIven what I discovered, there is no need to make changes to g:LspOmniFunc(). It is still nice to add g:LspOmniCompletePending() to know the waiting state of LSP. Maybe I should submit a new PR.

@girishji girishji closed this Jul 18, 2023
@girishji girishji deleted the buffer-completion-timeout branch July 18, 2023 23:03
@yegappan
Copy link
Owner

For optimization reasons, it makes sense to include your changes. The current code does a regular expression comparison for each character. With your change, only a single regular expression comparison will be used. Can you reopen the PR?

@girishji
Copy link
Contributor Author

I cannot reopen this PR. The button is grayed out. Maybe because I deleted the branch. But I will submit a new PR with same changes.

@mfbayraktar
Copy link

you could push the branch to the server than try to reopen it

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.

3 participants