[vim/vim] Regression: completefunc cannot temporarily switch windows any longer (after 8.2.0614) (#6017)

[Ingo Karkat]

Describe the bug
:help E840 (under :help complete-items) reads

The function is not allowed to move to another window or delete text.

Previously, move to another window meant that after the completefunc finishes, the current window has to be the same as the original one (where the completion was triggered). Temporarily switching windows (and going back) used to work fine. Since this change:

commit ff06f28
Author: Bram Moolenaar Br...@vim.org
Date: 2020-04-21 22:01:14 +0200

patch 8.2.0614: get ml_get error when deleting a line in 'completefunc'

Problem:    Get ml_get error when deleting a line in 'completefunc'. (Yegappan
            Lakshmanan)
Solution:   Lock the text while evaluating 'completefunc'.

any attempt to move to another window (e.g. via :wincmd w) raises E565: Not allowed to change text here.

I believe that this is an unintended side effect of using textlock for preventing the deletion of lines. (If this were an intended, compatibility-breaking tightening of rules, E840 should be raised instead of E565.)

To Reproduce
Detailed steps to reproduce the behavior:

1. Run vim --clean (or gvim --clean, etc.)
2. Define a dummy completion

    fun! TestComplete(findstart, base)
        wincmd w
        wincmd w
        return (a:findstart ? col('.') : ['foo'])
    endfun

1. Trigger the completion with more than one window

    :set completefunc=TestComplete
    :new " To have more than one window.
    :execute "normal! a\<C-x>\<C-u>"
    Error detected while processing function TestComplete:
    line    1:
    565: Not allowed to change text here

Expected behavior
I understand the motivation to prevent unexpected changes during completion. However, switching to other windows temporarily should be harmless, is convenient to implement custom completions [1], and sometimes even necessary [2].

1. In an attempt to simplify the implementation of custom completions, my CompleteHelper plugin searches other windows in order to offer the same functionality as :set complete+=w.
2. In many cases, the matching of completion candidates could be done without going to the corresponding window (e.g. using getbufline()), but as matching in the current buffer usually is done via search(), it is convenient to reuse that simple implementation for other windows as well. I have some completions like the MotionComplete plugin that uses a queried {motion} to obtain matches. That could not be emulated via low-level string-matching functions, but really has to be executed in that buffer. (And in order to use other windows as sources, it has to go there.)

Environment (please complete the following information):

• Vim version Vim 8.2.654
• OS: Ubuntu 16.04 LTS 64-bit

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

[Yegappan Lakshmanan]

Hi,

On Thu, Apr 30, 2020 at 9:02 AM Ingo Karkat <vim-dev...@256bit.org> wrote:

Describe the bug
:help E840 (under :help complete-items) reads

The function is not allowed to move to another window or delete text.

Previously, move to another window meant that after the completefunc finishes, the current window has to be the same as the original one (where the completion was triggered). Temporarily switching windows (and going back) used to work fine. Since this change:

commit ff06f28
Author: Bram Moolenaar Br...@vim.org
Date: 2020-04-21 22:01:14 +0200

patch 8.2.0614: get ml_get error when deleting a line in 'completefunc'

Expected behavior
I understand the motivation to prevent unexpected changes during completion. However, switching to other windows temporarily should be harmless, is convenient to implement custom completions [1], and sometimes even necessary [2].

1. In an attempt to simplify the implementation of custom completions, my CompleteHelper plugin searches other windows in order to offer the same functionality as :set complete+=w.
2. In many cases, the matching of completion candidates could be done without going to the corresponding window (e.g. using getbufline()), but as matching in the current buffer usually is done via search(), it is convenient to reuse that simple implementation for other windows as well. I have some completions like the MotionComplete plugin that uses a queried {motion} to obtain matches. That could not be emulated via low-level string-matching functions, but really has to be executed in that buffer. (And in order to use other windows as sources, it has to go there.)

Instead of switching to the window, if you use the win_execute() function to run

a command in a particular window from the custom complete function, does this

error happen?

- Yegappan

 

[vim-dev ML]

Hi,

On Thu, Apr 30, 2020 at 9:02 AM Ingo Karkat <vim-dev...@256bit.org>
wrote:

> *Describe the bug*
> :help E840 <https://vimhelp.appspot.com/insert.txt.html#E840> (under :help
> complete-items
> <https://vimhelp.appspot.com/insert.txt.html#complete-items>) reads

>
> The function is not allowed to move to another window or delete text.
>

> Previously, *move to another window* meant that after the completefunc

> finishes, the current window has to be the same as the original one (where

> the completion was triggered). *Temporarily* switching windows (and going

> back) used to work fine. Since this change:
>
> commit ff06f28

> <https://github.com/vim/vim/commit/ff06f283e3e4b3ec43012dd3b83f8454c98f6639>

> Author: Bram Moolenaar Br...@vim.org
> Date: 2020-04-21 22:01:14 +0200
>
> patch 8.2.0614: get ml_get error when deleting a line in 'completefunc'
>

> *Expected behavior*

> I understand the motivation to prevent unexpected changes during

> completion. However, switching to other windows *temporarily* should be

> harmless, is convenient to implement custom completions [1], and sometimes
> even necessary [2].
>

> 1. In an attempt to simplify the implementation of custom completions,
> my CompleteHelper plugin
> <http://www.vim.org/scripts/script.php?script_id=3914> searches other

> windows in order to offer the same functionality as :set complete+=w.

> 2. In many cases, the matching of completion candidates could be done

> without going to the corresponding window (e.g. using getbufline()),
> but as matching in the current buffer usually is done via search(), it
> is convenient to reuse that simple implementation for other windows as
> well. I have some completions like the MotionComplete plugin

> <http://www.vim.org/scripts/script.php?script_id=4265> that uses a
> queried {motion} to obtain matches. That could *not* be emulated via

> low-level string-matching functions, but really has to be executed in that
> buffer. (And in order to use other windows as sources, it has to go there.)
>
>

Instead of switching to the window, if you use the win_execute() function
to run
a command in a particular window from the custom complete function, does
this
error happen?

- Yegappan

[Christian Brabandt]

Is that the same as issue #6008?

> Am 30.04.2020 um 18:25 schrieb vim-dev ML <notifi...@github.com>:
>
> 

> Hi,
>
> On Thu, Apr 30, 2020 at 9:02 AM Ingo Karkat <vim-dev...@256bit.org>
> wrote:
>
> > *Describe the bug*
> > :help E840 <https://vimhelp.appspot.com/insert.txt.html#E840> (under :help
> > complete-items

> > <https://vimhelp.appspot.com/insert.txt.html#complete-items>) reads

> >
> > The function is not allowed to move to another window or delete text.
> >

> > Previously, *move to another window* meant that after the completefunc

> > finishes, the current window has to be the same as the original one (where

> > the completion was triggered). *Temporarily* switching windows (and going

> > back) used to work fine. Since this change:
> >
> > commit ff06f28

> > <https://github.com/vim/vim/commit/ff06f283e3e4b3ec43012dd3b83f8454c98f6639>

> > Author: Bram Moolenaar Br...@vim.org
> > Date: 2020-04-21 22:01:14 +0200
> >
> > patch 8.2.0614: get ml_get error when deleting a line in 'completefunc'
> >

> > *Expected behavior*

> > I understand the motivation to prevent unexpected changes during

> > completion. However, switching to other windows *temporarily* should be

> > harmless, is convenient to implement custom completions [1], and sometimes
> > even necessary [2].
> >

> > 1. In an attempt to simplify the implementation of custom completions,
> > my CompleteHelper plugin
> > <http://www.vim.org/scripts/script.php?script_id=3914> searches other

> > windows in order to offer the same functionality as :set complete+=w.

> > 2. In many cases, the matching of completion candidates could be done

> > without going to the corresponding window (e.g. using getbufline()),
> > but as matching in the current buffer usually is done via search(), it
> > is convenient to reuse that simple implementation for other windows as
> > well. I have some completions like the MotionComplete plugin

> > <http://www.vim.org/scripts/script.php?script_id=4265> that uses a
> > queried {motion} to obtain matches. That could *not* be emulated via

> > low-level string-matching functions, but really has to be executed in that
> > buffer. (And in order to use other windows as sources, it has to go there.)
> >
> >

> Instead of switching to the window, if you use the win_execute() function
> to run
> a command in a particular window from the custom complete function, does
> this
> error happen?
>
> - Yegappan
> —

> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub, or unsubscribe.

—
You are receiving this because you commented.

[Ingo Karkat]

Yegappan, win_execute() does not cause an error:

fun! TestComplete(findstart, base)
    call win_execute(win_getid((winnr() == 1) + 1), 'let g:line = getline(1)')
    return (a:findstart ? col('.') : [g:line])
endfun

However, it wouldn't be trivial to change the plugin, and both the translation of currently open windows to ids and the transfer of the data (above done via global variable) don't look very elegant. On the other hand, win_execute() seems to have at least one nice property that my current implementation has to work around: When entering a window, the height may increase from 0 (which can happen with Rolodex mode) to 1. Too bad that it's only been introduced in version 8.2.

Besides, what's the use of E839: Completion function changed window if this is now pre-empted by E565: Not allowed to change text here?!

—
You are receiving this because you commented.

[Ingo Karkat]

Is that the same as issue #6008?

@chrisbra No, I don't think so. This problem definitely only got introduced by 8.2.0614, 8.1 is not affected. And it's about switching windows, not opening new buffers.

—
You are receiving this because you commented.

[vim-dev ML]

Hi,

On Thu, Apr 30, 2020 at 12:04 PM Ingo Karkat <vim-dev...@256bit.org>
wrote:

> Yegappan, win_execute() does not cause an error:
>
> fun! TestComplete(findstart, base)
>
> call win_execute(win_getid((winnr() == 1) + 1), 'let g:line = getline(1)')
>
>

You can do this without using a global variable.

let line = win_execute(win_getid((winnr() == 1) + 1), 'echon

getline(1)')


>
> return (a:findstart ? col('.') : [g:line])
> endfun
>
> However, it wouldn't be trivial to change the plugin, and both the
> translation of currently open windows to ids and the transfer of the data
> (above done via global variable) don't look very elegant. On the other
> hand, win_execute() seems to have at least one nice property that my
> current implementation has to work around: When entering a window, the

> height may increase from 0 (which can happen with *Rolodex mode*) to 1.

> Too bad that it's only been introduced in version 8.2.
>
> Besides, what's the use of E839: Completion function changed window if

> this is now pre-empted by E565: Not allowed to change text here?!
>
>
>
I will let Bram comment on this change.

Regards,
Yegappan

[Yegappan Lakshmanan]

Hi,

On Thu, Apr 30, 2020 at 12:04 PM Ingo Karkat <vim-dev...@256bit.org> wrote:

Yegappan, win_execute() does not cause an error:

fun! TestComplete(findstart, base) 

    call win_execute(win_getid((winnr() == 1) + 1), 'let g:line = getline(1)')

You can do this without using a global variable.

     let line = win_execute(win_getid((winnr() == 1) + 1), 'echon getline(1)')

 

    return (a:findstart ? col('.') : [g:line])
endfun

However, it wouldn't be trivial to change the plugin, and both the translation of currently open windows to ids and the transfer of the data (above done via global variable) don't look very elegant. On the other hand, win_execute() seems to have at least one nice property that my current implementation has to work around: When entering a window, the height may increase from 0 (which can happen with Rolodex mode) to 1. Too bad that it's only been introduced in version 8.2.

Besides, what's the use of E839: Completion function changed window if this is now pre-empted by E565: Not allowed to change text here?!

[Bram Moolenaar]

Ingo Karkat wrote:

> **Describe the bug**
> [`:help E840`](https://vimhelp.appspot.com/insert.txt.html#E840) (under [`:help complete-items`](https://vimhelp.appspot.com/insert.txt.html#complete-items)) reads

>
> > The function is not allowed to move to another window or delete text.
>

> Previously, _move to another window_ meant that after the completefunc

> finishes, the current window has to be the same as the original one

> (where the completion was triggered). _Temporarily_ switching windows

> (and going back) used to work fine. Since this change:
>

> > commit ff06f283e3e4b3ec43012dd3b83f8454c98f6639

> > Author: Bram Moolenaar <Br...@vim.org>
> > Date: 2020-04-21 22:01:14 +0200
> >
> > patch 8.2.0614: get ml_get error when deleting a line in 'completefunc'
> >
> > Problem: Get ml_get error when deleting a line in 'completefunc'. (Yegappan
> > Lakshmanan)
> > Solution: Lock the text while evaluating 'completefunc'.
>

> any attempt to move to another window (e.g. via `:wincmd w`) raises
> `E565: Not allowed to change text here`.

>
> I believe that this is an unintended side effect of using textlock for
> preventing the deletion of lines. (If this were an intended,
> compatibility-breaking tightening of rules, `E840` should be raised

> instead of `E565`.)

Well, it was not unintended in the sense that it's better to stay on the
safe side. But it should be OK to switch to another window temporarily.
So long as no buffer is loaded, window split, etc. Anything that
changes what buffer is in what window may cause trouble.

I'l make a change to relax this a bit. It's not trivial, I hope it will
allow enough without allowing too much.


--
Give a man a computer program and you give him a headache,
but teach him to program computers and you give him the power
to create headaches for others for the rest of his life...
R. B. Forest

/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///

—
You are receiving this because you commented.

[Christian Brabandt]

Closed #6017.

—
You are receiving this because you commented.

[Christian Brabandt]

fixed by 6adb9ea

—
You are receiving this because you commented.

[Ingo Karkat]

I'l make a change to relax this a bit. It's not trivial, I hope it will
allow enough without allowing too much.

That seems to have done the trick; my completion library can access other windows again without causing an error.

Thanks Bram for the prompt fix; this is highly appreciated! Also thanks to Yegappan for recommending the very interesting win_execute() alternative, which over the long term may simplify several of my plugin implementations!

—
You are receiving this because you commented.