[patch] OptionSet not triggered for setwinvar()

[Christian Brabandt]

Bram,
it has been mentioned, that the OptionSet autocommand is not triggered,
for setwinvar() function calls. Reason is, the setwinvar() function
calls switch_win() to switch to the window which will block
autocommands.

Therefore here is a patch, that unblocks the autocommands, if an
optionset autocommand is defined and while I was there, I also made the
swich_win() function only trigger, if it will actually needs to change
windows or tabpages.

Attached is the patch, including a test.

Another question has been raised. Sometimes options will get set
directly (e.g. diffmode). Should those place converted to use the
set_option_...() so that the autocommand is triggered? Currently, using
:diffthis does not trigger the autocommand. If so, should it be
triggered only for setting the diffmode or for every setting?

Best,
Christian
--
Das Ärgste weiß die Welt von mir, und ich kann sagen, ich bin besser
als mein Ruf.
-- Friedrich Johann Christoph Schiller

[Christian Brabandt]

There was a small error in the patch,
attached again.


Mit freundlichen Grüßen
Christian
--
Aus Murphy's Gesetze:
Das einzige Backup, das du je brauchst, ist das, für das du keine Zeit
hattest.

[h_east]

Hi Chris.B and Bram!

2015-9-26(Sat) 6:21:20 UTC+9 Christian Brabandt:

> There was a small error in the patch,
> attached again.

Here is a simple solution patch.
https://gist.github.com/h-east/a26febcb48eb868b0676
The autocommands does not blocked, when does not switch window (and tabpage) in settabwinvar(), setwinvar(), gettabwinvar() and getwinvar().

I was previously discussed the similar topic. (sorry in Japanese)
https://github.com/vim-jp/issues/issues/765

I don't know the autocommand block/unblock policy.

Thanks.
--
Best regards,
Hirohito Higashi (a.k.a h_east)

[Bram Moolenaar]

Christian Brabandt wrote:

> Bram,
> it has been mentioned, that the OptionSet autocommand is not triggered,
> for setwinvar() function calls. Reason is, the setwinvar() function
> calls switch_win() to switch to the window which will block
> autocommands.
>
> Therefore here is a patch, that unblocks the autocommands, if an
> optionset autocommand is defined and while I was there, I also made the
> swich_win() function only trigger, if it will actually needs to change
> windows or tabpages.
>
> Attached is the patch, including a test.

I'm not sure it is such a good idea to trigger OptionSet when setting an
option in another window. It may have nasty side effects.

Perhaps we should document how this works, and suggest using WinEnter to
check option values when needed?

> Another question has been raised. Sometimes options will get set
> directly (e.g. diffmode). Should those place converted to use the
> set_option_...() so that the autocommand is triggered? Currently, using
> :diffthis does not trigger the autocommand. If so, should it be
> triggered only for setting the diffmode or for every setting?

This whole code needs to be cleaned up. We should consistently use the
code that checks the new value and takes care of side-effects, no matter
what command was used to set the option. Except for the first
initialization. Also ":set all&" should work differently.

--
hundred-and-one symptoms of being an internet addict:
18. Your wife drapes a blond wig over your monitor to remind you of what she
looks like.

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

[Justin M. Keyes]

Speaking of that, it would be nice if ":verbose set ..."  indicated that a given setting came from ":set all&".

--
Justin M. Keyes

[Christian Brabandt]

On Sa, 26 Sep 2015, Bram Moolenaar wrote:
> Christian Brabandt wrote:
>
> > Bram,
> > it has been mentioned, that the OptionSet autocommand is not triggered,
> > for setwinvar() function calls. Reason is, the setwinvar() function
> > calls switch_win() to switch to the window which will block
> > autocommands.
> >
> > Therefore here is a patch, that unblocks the autocommands, if an
> > optionset autocommand is defined and while I was there, I also made the
> > swich_win() function only trigger, if it will actually needs to change
> > windows or tabpages.
> >
> > Attached is the patch, including a test.
>
> I'm not sure it is such a good idea to trigger OptionSet when setting an
> option in another window. It may have nasty side effects.

Yes true. Especially since this may be evaluated in the wrong
buffer/window.

> Perhaps we should document how this works, and suggest using WinEnter to
> check option values when needed?

Like this?
diff --git a/runtime/doc/eval.txt b/runtime/doc/eval.txt
--- a/runtime/doc/eval.txt
+++ b/runtime/doc/eval.txt
@@ -5646,6 +5646,9 @@ settabwinvar({tabnr}, {winnr}, {varname}
:call settabwinvar(1, 1, "&list", 0)
:call settabwinvar(3, 2, "myvar", "foobar")
< This function is not available in the |sandbox|.
+
+ When setting options using |:let-&|, won't trigger the
+ |OptionSet| autocommand, to avoid nasty side effects.

setwinvar({nr}, {varname}, {val}) *setwinvar()*
Like |settabwinvar()| for the current tab page.

>
> > Another question has been raised. Sometimes options will get set
> > directly (e.g. diffmode). Should those place converted to use the
> > set_option_...() so that the autocommand is triggered? Currently, using
> > :diffthis does not trigger the autocommand. If so, should it be
> > triggered only for setting the diffmode or for every setting?
>
> This whole code needs to be cleaned up. We should consistently use the
> code that checks the new value and takes care of side-effects, no matter
> what command was used to set the option. Except for the first
> initialization.

Might be a good Google Summer of Code project?

> Also ":set all&" should work differently.

Why? Is anybody actually using this?

Best,
Christian
--
Wußten Sie schon...
... daß Taucher, die vor Wut kochen, deswegen noch lange
keine Tauchsieder sind?

[Christian Brabandt]

On Sa, 26 Sep 2015, Justin M. Keyes wrote:
> Speaking of that, it would be nice if ":verbose set ..." indicated
> that a given setting came from ":set all&".

Is this ":set all&" really used? What is the use case for this?

Best,
Christian
--
Sein Gewissen war rein, er benutzte es nie.
-- Stanislaw Jerzy Lec (eig. S. J. de Tusch-Letz)

[Bram Moolenaar]

I was thinking of still doing it for the current window, since
":let &option = val" is equivalent to ":set option=val".
If someone doesn't want this prepend ":noau".

> > > Another question has been raised. Sometimes options will get set
> > > directly (e.g. diffmode). Should those place converted to use the
> > > set_option_...() so that the autocommand is triggered? Currently, using
> > > :diffthis does not trigger the autocommand. If so, should it be
> > > triggered only for setting the diffmode or for every setting?
> >
> > This whole code needs to be cleaned up. We should consistently use the
> > code that checks the new value and takes care of side-effects, no matter
> > what command was used to set the option. Except for the first
> > initialization.
>
> Might be a good Google Summer of Code project?

If we ever find mentors...

> > Also ":set all&" should work differently.
>
> Why? Is anybody actually using this?

Probably not, but it should work like setting each option back to its
default value, including all the side effects.

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

19. All of your friends have an @ in their names.

[Justin M. Keyes]

On Sep 26, 2015 4:10 PM, "Christian Brabandt" <cbl...@256bit.org> wrote:
>
> On Sa, 26 Sep 2015, Justin M. Keyes wrote:
> > Speaking of that, it would be nice if ":verbose set ..."  indicated
> > that a given setting came from ":set all&".
>
> Is this ":set all&" really used? What is the use case for this?

It is very useful to reach a known default state, in order to undo the garbage that various distributions ship in /etc/vimrc.

Setting each option explicitly is annoying: it defeats the purpose of having _defaults_.

[Christian Brabandt]

Hi Bram!

On Sa, 26 Sep 2015, Bram Moolenaar wrote:

>

In that case the patch from Hirohito Higashi fixes it.

> > > This whole code needs to be cleaned up. We should consistently use the
> > > code that checks the new value and takes care of side-effects, no matter
> > > what command was used to set the option. Except for the first
> > > initialization.
> >
> > Might be a good Google Summer of Code project?
>
> If we ever find mentors...

If we ask early enough, I am sure we'll find some. I would volunteer.

Best,
Christian
--
Gewissen Geistern muss man ihre Idiotismen lassen.
-- Goethe, Maximen und Reflektionen, Nr. 269

[Ben Fritz]

I agree with others that triggering the autocmd for the :let syntax is too useful not to support. I use :let rather than set for all sorts of options from errorformat to showbreak that can contain a lot of spaces and/or backslashes.

[Christian Brabandt]

Hi Ben!

let syntax is already supported. Only setwinvar()/settabvar() do not
trigger it currently, because when switching windows/tabs autocommands
are blocked and the lock is only released after returning to the
original window.

Best
Christian
--
Es gibt drei Sorten von Menschen: solche, die sich zu Tode sorgen;
solche, die sich zu Tode arbeiten; und solche, die sich zu Tode
langweilen.
-- Winston Spencer Churchill

[Ben Fritz]

On Monday, September 28, 2015 at 9:45:18 AM UTC-5, Christian Brabandt wrote:
> Hi Ben!
>
> On Mo, 28 Sep 2015, Ben Fritz wrote:
>
> > I agree with others that triggering the autocmd for the :let syntax is
> > too useful not to support. I use :let rather than set for all sorts of
> > options from errorformat to showbreak that can contain a lot of spaces
> > and/or backslashes.
>
> let syntax is already supported. Only setwinvar()/settabvar() do not
> trigger it currently, because when switching windows/tabs autocommands
> are blocked and the lock is only released after returning to the
> original window.
>

OK. I was concerned because one of your patches contained this:

+ When setting options using |:let-&|, won't trigger the
+ |OptionSet| autocommand, to avoid nasty side effects.

I was voicing my concern about that statement, I'll be happy if it no longer applies.