Patch 8.2.2426

[Bram Moolenaar]

Patch 8.2.2426
Problem: Allowing 'completefunc' to switch windows causes trouble.
Solution: use "textwinlock" instead of "textlock".
Files: src/insexpand.c, src/testdir/test_ins_complete.vim,
src/testdir/test_popup.vim


*** ../vim-8.2.2425/src/insexpand.c 2020-12-27 18:03:18.688859822 +0100
--- src/insexpand.c 2021-01-29 19:55:26.090343143 +0100
***************
*** 2218,2226 ****
pos = curwin->w_cursor;
curwin_save = curwin;
curbuf_save = curbuf;
! // Lock the text to avoid weird things from happening. Do allow switching
! // to another window temporarily.
! ++textlock;

// Call a function, which returns a list or dict.
if (call_vim_function(funcname, 2, args, &rettv) == OK)
--- 2218,2227 ----
pos = curwin->w_cursor;
curwin_save = curwin;
curbuf_save = curbuf;
! // Lock the text to avoid weird things from happening. Also disallow
! // switching to another window, it should not be needed and may end up in
! // Insert mode in another buffer.
! ++textwinlock;

// Call a function, which returns a list or dict.
if (call_vim_function(funcname, 2, args, &rettv) == OK)
***************
*** 2243,2249 ****
break;
}
}
! --textlock;

if (curwin_save != curwin || curbuf_save != curbuf)
{
--- 2244,2250 ----
break;
}
}
! --textwinlock;

if (curwin_save != curwin || curbuf_save != curbuf)
{
***************
*** 3226,3232 ****
return -1;

if (compl_leader != NULL
! && (compl_shown_match->cp_flags & CP_ORIGINAL_TEXT) == 0)
{
// Set "compl_shown_match" to the actually shown match, it may differ
// when "compl_leader" is used to omit some of the matches.
--- 3227,3233 ----
return -1;

if (compl_leader != NULL
! && (compl_shown_match->cp_flags & CP_ORIGINAL_TEXT) == 0)
{
// Set "compl_shown_match" to the actually shown match, it may differ
// when "compl_leader" is used to omit some of the matches.
*** ../vim-8.2.2425/src/testdir/test_ins_complete.vim 2021-01-28 18:34:27.783630494 +0100
--- src/testdir/test_ins_complete.vim 2021-01-29 19:58:29.401677250 +0100
***************
*** 562,592 ****
call setline(1, ['', 'abcd', ''])
call assert_fails('exe "normal 2G$a\<C-X>\<C-U>"', 'E578:')

- set completefunc&
- delfunc CompleteFunc
- delfunc CompleteFunc2
- close!
- endfunc
-
- func Test_completefunc_error_not_asan()
- " The following test causes an ASAN failure.
- CheckNotAsan
-
" Jump to a different window from the complete function
! func! CompleteFunc(findstart, base)
if a:findstart == 1
return col('.') - 1
endif
wincmd p
return ['a', 'b']
endfunc
! set completefunc=CompleteFunc
new
! call assert_fails('exe "normal a\<C-X>\<C-U>"', 'E839:')
close!

set completefunc&
delfunc CompleteFunc
endfunc

" Test for returning non-string values from 'completefunc'
--- 562,585 ----
call setline(1, ['', 'abcd', ''])
call assert_fails('exe "normal 2G$a\<C-X>\<C-U>"', 'E578:')

" Jump to a different window from the complete function
! func CompleteFunc3(findstart, base)
if a:findstart == 1
return col('.') - 1
endif
wincmd p
return ['a', 'b']
endfunc
! set completefunc=CompleteFunc3
new
! call assert_fails('exe "normal a\<C-X>\<C-U>"', 'E565:')
close!

set completefunc&
delfunc CompleteFunc
+ delfunc CompleteFunc2
+ delfunc CompleteFunc3
+ close!
endfunc

" Test for returning non-string values from 'completefunc'
*** ../vim-8.2.2425/src/testdir/test_popup.vim 2020-12-24 18:38:37.181858419 +0100
--- src/testdir/test_popup.vim 2021-01-29 21:03:16.416179578 +0100
***************
*** 342,348 ****
setlocal completefunc=DummyCompleteOne
call setline(1, 'one')
/^one
! call assert_fails('call feedkeys("A\<C-X>\<C-U>\<C-N>\<Esc>", "x")', 'E578:')
call assert_equal(winid, win_getid())
call assert_equal('onedef', getline(1))
q!
--- 342,348 ----
setlocal completefunc=DummyCompleteOne
call setline(1, 'one')
/^one
! call assert_fails('call feedkeys("A\<C-X>\<C-U>\<C-N>\<Esc>", "x")', 'E565:')
call assert_equal(winid, win_getid())
call assert_equal('onedef', getline(1))
q!
***************
*** 642,649 ****
set completefunc=MessComplete
new
call setline(1, 'Ju')
! call feedkeys("A\<c-x>\<c-u>/\<esc>", 'tx')
! call assert_equal('Oct/Oct', getline(1))
bwipe!
set completefunc=
endfunc
--- 642,649 ----
set completefunc=MessComplete
new
call setline(1, 'Ju')
! call assert_fails('call feedkeys("A\<c-x>\<c-u>/\<esc>", "tx")', 'E578:')
! call assert_equal('Jan/', getline(1))
bwipe!
set completefunc=
endfunc
*** ../vim-8.2.2425/src/version.c 2021-01-28 20:18:04.800631715 +0100
--- src/version.c 2021-01-29 19:56:42.314065568 +0100
***************
*** 752,753 ****
--- 752,755 ----
{ /* Add new patch number below this line */
+ /**/
+ 2426,
/**/

--
hundred-and-one symptoms of being an internet addict:
265. Your reason for not staying in touch with family is that
they do not have e-mail addresses.

/// 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 ///

[Ingo Karkat]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 29/01/2021 21:07, Bram Moolenaar wrote:
>
> Patch 8.2.2426 Problem: Allowing 'completefunc' to switch
> windows causes trouble. Solution: use "textwinlock" instead of
> "textlock". Files: src/insexpand.c,
> src/testdir/test_ins_complete.vim, src/testdir/test_popup.vim

I hope that use of win_execute() is still possible during completion,
right? You already had disabled window switching once in 8.2.0614, and
after I complained in https://github.com/vim/vim/issues/6017 you again
allowed switching as long as it returned to the original window.

Many of my completion plugins use window switching, and I never had
any issues reported because of it, so it's sad that you now decide it
has to be disallowed in general, just because _some_ code _can_
misbehave. As long as win_execute() can be used (and that does offer
some minor benefits over :wincmd w), I can adapt the plugin code (but
will then have to either drop old Vim versions that don't have it, or
keep maintaining the existing implementation in parallel).

- -- regards, ingo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJgFHQKAAoJEA7ziXlAzQ/v6FwH/19yswATLqxjfaPcTmyFvfjb
q+87zXc1hM3Mb9kGPne1iuP/SeojuijJPRr4r4+ACgOI2QSq1sfoFkAk7nnMZ/rX
Idaub35QBY10iEdSo4T2//ttNrkMujCNHiopfQGCwdhofqiSuxK3zT3jvY52UfpF
CRKZ4a7N12ImTP3KjDCW1iWJeIyP4ZBQePVpURU4gyIcQWNQWmULSncfQ/IoLu59
zg5gS8FWENdsDB4ICiTIn9ZL5QTXVzQIUVJK6yz1t+IvvucfjJMXaDe7ekPawSpv
bZITYJss86cLOPMfB5sh8G9bedcFG67IMtZnbGnq5/WW1g04LQAdYaCG+t7vRVs=
=R0Vs
-----END PGP SIGNATURE-----

[Bram Moolenaar]

Ingo Karkat wrote:

> On 29/01/2021 21:07, Bram Moolenaar wrote:
> >
> > Patch 8.2.2426 Problem: Allowing 'completefunc' to switch
> > windows causes trouble. Solution: use "textwinlock" instead of
> > "textlock". Files: src/insexpand.c,
> > src/testdir/test_ins_complete.vim, src/testdir/test_popup.vim
>
> I hope that use of win_execute() is still possible during completion,
> right? You already had disabled window switching once in 8.2.0614, and
> after I complained in https://github.com/vim/vim/issues/6017 you again
> allowed switching as long as it returned to the original window.

I forgot about that. And frankly, reading it again I don't quite
understand why I allowed window switching. If someone has autocommands
setup, it an fail miserably.

As I mentioned: "It's not trivial, I hope it will allow enough without
allowing too much". So now we found another case where it causes
trouble. I'm not sure if we can keep fixing every problem we find.

> Many of my completion plugins use window switching, and I never had
> any issues reported because of it, so it's sad that you now decide it
> has to be disallowed in general, just because _some_ code _can_
> misbehave. As long as win_execute() can be used (and that does offer
> some minor benefits over :wincmd w), I can adapt the plugin code (but
> will then have to either drop old Vim versions that don't have it, or
> keep maintaining the existing implementation in parallel).

I don't like existing plugins to fail, but I also don't want a
misbehaving plugin to cause trouble. In case a complete function bails
out and doesn't restore the window, what to do? Restoring the window
after the function returns might be best, but that might fail too
(again, because of autocommands). Perhaps we should disable
autocommands while in the complete function? It's getting too
complicated...

So, what functionality is missing to do what you want without actually
switching windows?


--
Two fish in a tank. One says to the other:
"Do you know how to drive this thing?"

[Ingo Karkat]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 29/01/2021 22:14, Bram Moolenaar wrote:
>
> Ingo Karkat wrote:
>
>> On 29/01/2021 21:07, Bram Moolenaar wrote:
>>>
>>> Patch 8.2.2426 Problem: Allowing 'completefunc' to switch
>>> windows causes trouble. Solution: use "textwinlock" instead
>>> of "textlock". Files: src/insexpand.c,
>>> src/testdir/test_ins_complete.vim, src/testdir/test_popup.vim
>>
>> I hope that use of win_execute() is still possible during
>> completion, right? You already had disabled window switching once
>> in 8.2.0614, and after I complained in
>> https://github.com/vim/vim/issues/6017 you again allowed
>> switching as long as it returned to the original window.
>
> I forgot about that. And frankly, reading it again I don't quite
> understand why I allowed window switching. If someone has
> autocommands setup, it an fail miserably.

Of course the plugin uses :noautocmd. It's the often-discussed tradeoff:
Vim's commands primarily focus on easy interactive use (and that's
good!); unfortunately, that makes plugins harder to write in a robust
way (but it is possible, with some effort and learning). But here I
think all completefunc's are written for plugins, never ad-hoc.

> As I mentioned: "It's not trivial, I hope it will allow enough
> without allowing too much". So now we found another case where it
> causes trouble. I'm not sure if we can keep fixing every problem
> we find.

It would just be nice from my perspective to detect the bad cases and
only then throw an error, instead of disallowing in general. But I fully
understand if that's not possible due to the complexity.

>> Many of my completion plugins use window switching, and I never
>> had any issues reported because of it, so it's sad that you now
>> decide it has to be disallowed in general, just because _some_
>> code _can_ misbehave. As long as win_execute() can be used (and
>> that does offer some minor benefits over :wincmd w), I can adapt
>> the plugin code (but will then have to either drop old Vim
>> versions that don't have it, or keep maintaining the existing
>> implementation in parallel).
>
> I don't like existing plugins to fail, but I also don't want a
> misbehaving plugin to cause trouble. In case a complete function
> bails out and doesn't restore the window, what to do? Restoring
> the window after the function returns might be best, but that might
> fail too (again, because of autocommands). Perhaps we should
> disable autocommands while in the complete function? It's getting
> too complicated...

I don't think you have to go so far. People will notice when completion
breaks and then learn about using :noautocmd, try...finally, etc.
Throwing an "E999: Must return to original window" would be fine.

> So, what functionality is missing to do what you want without
> actually switching windows?

If I can still win_execute() within completefunc() to visit other
windows temporarily, do search() and cursor() in there, that would be
fine. As I hinted at, it would even have some minor benefits, like not
having to restore the previous window and windows with a height of 0.

- -- regards, ingo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJgFH6RAAoJEA7ziXlAzQ/v12AH/3WtTP2o2pmOr0F3wX+X+Txv
Yzo2HNJ+eJ72SNMVxT1D6vtkOswY6Bp46O3n9J0Mf/+9qprCTjW3bPehPRNr/LpW
Loi5X2qjWL+YqfYr2OEevrRP6MomGGboJagl9fK94PJDf9hQzxD3OibmUCWZCi9K
tfRo0y4YAmtXeI/+/Nzj1m+30HTs894LaSc7h5rZbsXEw5XtQfhz/zXS9IO8jTG8
vc9swhIaQo0WE5CZroMgXh/GZDeY0K4Bi/6xGu+Ca7R9R+xPG1rJnLbQvF44+Ra9
nbaKsLwL6/O9j/UD1cn5yc7IxLngtQzH58Kp7H4OuAIQ7aHfWsUKhVSxpk3bDiQ=
=ub/4
-----END PGP SIGNATURE-----

[Bram Moolenaar]

As far as I can see win_execute() works. So you are OK then?

--
hundred-and-one symptoms of being an internet addict:

268. You get up in the morning and go online before getting your coffee.

[Ingo Karkat]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Yes, I can replace the :wincmd w iteration with win_execute() then.
I'll try to adapt my plugin in the coming days. I'll report back
should I run into any problems :-) Thanks for considering my use case!

- -- regards, ingo
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJgFUiZAAoJEA7ziXlAzQ/vCfMH/14KSQdG0lye2eIQ+FQeikon
Dcu/u9c1hP1PsrPAQ6P09QYXtQTsIX7OtySX23Vyow9luPvzeRFX3ZOxImgsux4c
MHndWSz/as8N58QGZO7TnbriWsTqOA1LKEWdKfI6hpITQB55bK8SaHjiapNc0oGN
jiK2Zuzuf3QfYuLM9dqPme5WcZ/SyECi6GGBsYUbPKZS5qgsZz0aku3V3NIjOHMo
SiTpVOQtxL4VCBTH0NOXDQ/kgR6rSsogLHVSh2ZbEjQRU6DvVpdQHoTQicYXVFAT
OYaoQYnhWoVK0mSPN7TN6IWBToASKki8J9LzzEIswQhqeMPfoTpiNWVslD1MR0I=
=lazK
-----END PGP SIGNATURE-----