[vim/vim] [RFC] Enhance OptionSet even to use v:event (#4118)
latricewilgus
See also discussion in #4043.
This patch set extends the OptionSet event to use the v:event variable to give more information what happened. When an option is set the v:event variable is populated with the following entries:
name: the full name of the option being set
valuenew: the new value of the option
localold: Only set if the local value of an option was changed. In that case it contains the old local value. Not present when the option was only changed globally.
globalold: Only set if the global value of an option was changed. In that case it contains the old global value. Not present when the option was only changed locally.
The types of these entries are currently all strings (which should be the same as the type of v:option_new, v:option_old)
Things to look out for when reviewing:
- Did I catch all possible ways on how the OptionSet event is triggered?
- Do I handle the creation of v:event correctly? (I copied that part from another event)
- Memory leaks. Especially in the code for the string options as I have no clue when strings get allocated and who is responsible for freeing them.
- I converted only the first test in the
Test_OptionSetintest_autocmd.vim. The others will follow if this gets approved. Consequently the test suite currently fails. - local and global string option seem to share their values. Thus, the current oldlocal value is sometimes empty "" (see test script below for the first setlocal call of 'define'). Suggestions are welcome.
Here is the test script I used for testing by hand:
" Test num option
setlocal foldcolumn=11
setglobal foldcolumn=12
augroup test
autocmd!
autocmd OptionSet foldcolumn :echomsg v:event
augroup END
setlocal foldcolumn=1
setglobal foldcolumn=2
set foldcolumn=3
" Reset
setlocal foldcolumn=11
setglobal foldcolumn=12
let &l:foldcolumn=1
let &g:foldcolumn=2
let &foldcolumn=3
" Test bool option
setlocal number
setglobal nonumber
augroup test
autocmd!
autocmd OptionSet number :echomsg v:event
augroup END
setlocal number
setglobal nonumber
set number
setlocal number
setglobal nonumber
let &l:number = 1
let &g:number = 0
let &number = 1
" Test string option
setlocal define=a
setglobal define=b
augroup test
autocmd!
autocmd OptionSet define :echomsg v:event
augroup END
setlocal define=A
setglobal define=B
set define=C
let &l:define = "d"
let &g:define = "e"
let &define = "f"
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/4118
Commit Summary
- Populate v:event when setting a number option
- Populate v:event when setting a bool option
- Populate v:event when setting a string option
- Update OptionSet event tests to also compare v:event
File Changes
- M src/eval.c (6)
- M src/option.c (93)
- M src/testdir/test_autocmd.vim (6)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
Bram Moolenaar
> See also discussion in #4043.
>
> This patch set extends the OptionSet event to use the v:event variable to give more information what happened. When an option is set the `v:event` variable is populated with the following entries:
>
> `name`: the full name of the option being set
> `valuenew`: the new value of the option
This is already available in v:option_new
> `localold`: Only set if the local value of an option was changed. In
> that case it contains the old local value. Not present when the option
> was only changed globally.
> `globalold`: Only set if the global value of an option was changed. In
> that case it contains the old global value. Not present when the
> option was only changed locally.
value. The only value you appear to be adding is the old local value.
But this should be the same as v:option_old when using :setlocal. The
global value doesn't change then, thus can be obtained with &g:option.
I'm not sure what problem you are trying to tackle here. Perhaps the
only thing missing is what action changed the option: :set, :setlocal,
:setglobal, ":let" or a modeline. Only the info about a modeline
appears to be relevant and currently missing.
--
hundred-and-one symptoms of being an internet addict:
78. You find yourself dialing IP numbers on the phone.
/// 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 ///
latricewilgus
Hi
I'm not sure what problem you are trying to tackle here.
As explained in issue #4043, the SetOption autocmd event is currently incapable of detection the difference whether :set or :setglobal was used. The reason I needed this was to "sync"/"bind" to option together.
This patch tries to solve this problem by providing more information to the listener of the SetOption event in form of additional fields to v:event (see #4043 for some discussion related to that.)
name: the full name of the option being set
isn't that already available with
<amatch>?
valuenew: the new value of the option
This is already available in
v:option_new
Indeed they are. The charm of the extended v:event is that it contains all information with respect to the OptionSet event. No need to gather the data from different sources (expand("<amatch>"), v:option_new, ...). See again discussion in #4043.
I, personally, like a consistent access API more than a mixture of different styles. You can already see the incoherence of getting the name of the option and the value of the option in the test suite (src/testdir/test_autocmd.vim line 469):
There the option name has to be passed to the event function, while the option values are read inside of the listening function. Also expand("<amatch>") is more clunky than using a variable but YMMV.
Bram Moolenaar
> > I'm not sure what problem you are trying to tackle here.
>
> As explained in issue #4043, the `SetOption` autocmd event is
> currently incapable of detection the difference whether `:set` or
> `:setglobal` was used. The reason I needed this was to "sync"/"bind"
> to option together.
>
> This patch tries to solve this problem by providing more information
> to the listener of the `SetOption` event in form of additional fields
> to `v:event` (see #4043 for some discussion related to that.)
Or that it was from a modeline. It would be a lot easier to understand
if that was directly given, instead of deducting it from whetere some
value is present.
Currently v:option_type uses "global" for both :set and :setglobal. We
can't really change this for backwards compatibility. We can add
v:option_command, which would be "set", "setlocal", "setglobal" or
"modeline".
> > > `name`: the full name of the option being set
> >
> > isn't that already available with `<amatch>` ?
> >
> >
> > This is already available in `v:option_new`
>
> Indeed they are. The charm of the extended `v:event` is that it
> need to gather the data from different sources (`expand("<amatch>"`),
> `v:option_new`, ...). See again discussion in #4043.
Using <amatch> is consistent with other autocommands.
> I, personally, like a consistent access API more than a mixture of
> different styles. You can already see the incoherence of getting the
> name of the option and the value of the option in the test suite
> (src/testdir/test_autocmd.vim line 469):
> There the option name has to be passed to the event function,
> while the option values are read inside of the listening function.
Getting the value of <amatch> could have been done inside the function.
> Also `expand("<amatch>")` is more clunky than using a variable but
> YMMV.
--
ASCII stupid question, get a stupid ANSI.
/// 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 ///
—
Christian Brabandt
You can already see the incoherence of getting the name of the option and the value of the option in the test suite (src/testdir/test_autocmd.vim line 469):
I am probably guilty of that one.
There the option name has to be passed to the event function, while the option values are read inside of the listening function.
True, it could have been done in the event function, but wasn't. But I don't see the problem with that approach. Is there anything wrong with the existing test?
latricewilgus
Hi
@chrisbra wrote:
There the option name has to be passed to the event function, while the option values are read inside of the listening function.
True, it could have been done in the event function, but wasn't. But I don't see the problem with that approach. Is there anything wrong with the existing test?
No, there is not, don't worry. It's just a matter of taste. You are the author, you made the choice, that choice works and I'm fine with that.
Instead, I'd like to come back to the original problem I have: The OptionSet event does not provide enough information to "sync" an option value with another one.
@brammool wrote:
Currently v:option_type uses "global" for both :set and :setglobal. We can't really change this for backwards compatibility. We can add v:option_command, which would be "set", "setlocal", "setglobal" or "modeline".
That's ok for me. But please notice (as pointed out by @andymass in #4043) that in the case with :set one of the old option values is still not provided. Thus, we need an additional variable like v:option_old_global or v:option_old_alt(ernative). (At that point v:event came as an alternative implementation approach into play.)
If you think two new v:option_something variables are better suited than (new) entries to v:event, then that's ok for me, too, as it allows me to solve my problem.
Regarding the modeline. Honestly, I did not think about modelines as I would have treated them like :setlocal. If we decide to distinguish them, that's ok (and will surely solve a problem for some other person in the future).
Bram Moolenaar
> @chrisbra wrote:
> > > There the option name has to be passed to the event function, while the option values are read inside of the listening function.
> >
> > True, it could have been done in the event function, but wasn't. But I don't see the problem with that approach. Is there anything wrong with the existing test?
>
> No, there is not, don't worry. It's just a matter of taste. You are the author, you made the choice, that choice works and I'm fine with that.
>
>
> Instead, I'd like to come back to the original problem I have: The `OptionSet` event does not provide enough information to "sync" an option value with another one.
>
> @brammool wrote:
> > Currently v:option_type uses "global" for both :set and :setglobal. We can't really change this for backwards compatibility. We can add v:option_command, which would be "set", "setlocal", "setglobal" or "modeline".
>
> If you think two new `v:option_something` variables are better suited than (new) entries to `v:event`, then that's ok for me, too, as it allows me to solve my problem.
As an alternative, if both the global and the local value changes, we
could trigger OptionSet twice, once for the global and once for the
local value. But then maybe the correlation between the two is hard to
figure out.
> Regarding the modeline. Honestly, I did not think about modelines as I
> them, that's ok (and will surely solve a problem for some other person
> in the future).
semantics, thus I'm sure some day someone will ask for this info.
--
From "know your smileys":
:~) A man with a tape recorder up his nose
/// 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 ///
—
latricewilgus
Hi
@brammool wrote:
As an alternative, if both the global and the local value changes, we could trigger OptionSet twice, once for the global and once for the local value. But then maybe the correlation between the two is hard to figure out.
I wouldn't use that approach. Since as you said it would be hard to correlate a single option set statement with the corresponding OptionSet events and this might also break some existing code which expects only a single OptionSet event to be fired.
latricewilgus
If I change the proposed implementation to
- adding a v:option_command containing either "set", "setglobal", "setlocal" (or "modeline")
- adding a v:option_value_alt which is only populate in the case of "set" to the old global value (v:option_value contains the old local value in this case). This is chosen to match the ":set" returns local value and two of the three paths set v:option_value to the old local value.
would this pull request then be accepted?
Remark: :help :set-verbose states that vim also tracks option setting from --cmd argument, the -c argument, or an environment variable. As I have no clue how to detect that in the C code, I will stick with "set" , "setglobal", "setlocal" (and "modeline" if that happens to be indicated by the OPT_MODELINE bit the opt_flags argument).
Bram Moolenaar
I suppose this would be OK, but I don't see an update to the help, thus I'm not totally sure. Also, the tests fail.
latricewilgus
So here is the updated pull request to use the variables v:option_command and v:option_old_alt.
Things to look out for when reviewing:
-
I introduced the two new variables in vim.h and eval.c. The commit 84c0587 adds them to the end. The last commit (ab1497a) reorders them (and thus redefines a lot of variables constants in the process). If you don't like that then leave the last commit out.
-
I'm not familiar with the allocating/freeing of strings when it comes to passing them to vim variables. Please take a look whether I've introduced a space leak of some sort.
-
The code for filling buf_old_global via printf is executed always, although technically it is only needed in the case when :set is used. However, I abstained from moving that line into the if clause to have the code modulized into a part where the values (buf_old, buf_new, ...) are prepared and one section where the vim variables (v:option_new, v:option_old, ...) are assigned.
-
For local-global string options we have that when no local value is set, then the old local value is empty. Would it be better to return in this case the global value as old local value (similar to what
setlocal option?would return)? If so, then I need some guidance on how to implement that as I was not able to figure out how to obtain the value for a local-global option which references the global value (I thought get_varp_scope() would do it but apparently it doesn't). -
I assumed that when opt_flags has the OPT_MODELINE bit set, then the option was set via the modeline. Please correct me if I'm wrong.
latricewilgus
* I'm not familiar with the allocating/freeing of strings when it comes to passing them to vim variables. Please take a look whether I've introduced a space leak of some sort.
Seems I have. However, I cannot figure out how the CI AddressSanitizer failure is related and can be fixed. Help is welcome.
latricewilgus
Could someone lend me a hand with this string issue as I have no clue how the string sharing between C and VimL works. Thanks.
Andy Massimino
I don't think it has to do with the vimscript interface in this case. One problem I see is origlval is saved, but might this be a pointer to the global value == origval, for options like 'backspace.' However, the memory pointed to by origval is freed, for instance here. Thus, it's a use-after-free. I'm not sure if 'backspace' is the only instance of this problem and the issue is very subtle in that some options are presented differently than how they are stored internally. Besides, 'backspace' is global only so maybe it doesn't even make sense to set both v:option_old and v:option_old_alt?
Also it seems like this code is changing the meaning of v:option_old in this set_vim_var_string(VV_OPTION_OLD, oldlval, -1);. I think v:option_old should stay the same as before and be better documented, and then just set v:option_old_local in this case following the source code comment "use the global value here"
latricewilgus
Thanks @andymass for having a look and the hints. I'll have a second look keeping your remarks in mind. The string option code uses some quirks (presumably to save a byte or two in memory) which I do not yet fully understand. And that brings us to:
Also it seems like this code is changing the meaning of v:option_old in this set_vim_var_string(VV_OPTION_OLD, oldlval, -1);. I think v:option_old should stay the same as before and be better documented, and then just set v:option_old_local in this case following the source code comment "use the global value here"
Year it seems that way. However, this is the correct way to preserve the old behaviour of v:option_old. It took me a while to figure that out. If I remember correctly this seems due to saving some bytes and replacing the local value very early with a reference to the global one, so for the set call the variable names seem to be swapped (but not for setlocal orsetglobal). More precisely, the code in (commit 2deb573 Line 4813):
origlval = *(char_u **)get_varp_scope(&(options[opt_idx]), OPT_LOCAL);
seems to return the (old) global value!
You can convince yourself that the behaviour did not change by having a look at the commit of the tests (5bbd163): The second value in the g:options list is the v:option_old (in the old and also in the new version) which I did not change.
latricewilgus
@bram, @andymass: I had a second look at the problem and found that v:option_old is handled inconsistently by vim (i.e. before this pull request). Take a look at the output of the following script:
:echomsg
\ "|" printf("%-9s", "new")
\ "|" printf("%-9s", "old")
\ "|"
" Boolean
"""""""""""
" global boolean option
set wrapscan
" local-global (to buffer) boolean option
setlocal autoread
setglobal noautoread
" local (to buffer) boolean option
setlocal cindent
setglobal nocindent
" local-global (to window) boolean option
"(no such option exists)
" local (to window) boolean option
setlocal cursorcolumn
setglobal nocursorcolumn
" Number
"""""""""""
" global number option
set cmdheight=2
" local-global (to buffer) number option
setlocal undolevels=1
setglobal undolevels=8
" local (to buffer) number option
setlocal wrapmargin=1
setglobal wrapmargin=8
" local-global (to window) number option
"(no such option exists)
" local (to window) number option
setlocal foldcolumn=1
setglobal foldcolumn=8
" String
"""""""""""
" global string option
set backupext=foo
" local-global (to buffer) string option
setlocal tags=L
setglobal tags=G
" local (to buffer) string option
setlocal spelllang=L
setglobal spelllang=G
" local-global (to window) string option
setlocal statusline=L
setglobal statusline=G
" local (to window) string option
setlocal foldignore=L
setglobal foldignore=G
augroup test
autocmd!
autocmd OptionSet * :echomsg
\ "|" printf("%-9s", v:option_new)
\ "|" printf("%-9s", v:option_old)
\ "|"
augroup END
:echomsg "String options (L = local, G = global):"
:echomsg "global string option (backupext)"
set backupext=c
:echomsg "local-global (to buffer) string option (tags)"
set tags=c
:echomsg "local (to buffer) string option (spelllang)"
set spelllang=c
:echomsg "local-global (to window) string option (statusline)"
set statusline=c
:echomsg "local (to window) string option (foldignore)"
set foldignore=c
:echomsg "Number options (1 = local, 8 = global):"
:echomsg "global number option (cmdheight)"
set cmdheight=3
:echomsg "local-global (to buffer) number option (undolevels)"
set undolevels=3
:echomsg "local (to buffer) number option (wrapmargin)"
set wrapmargin=3
:echomsg "local-global (to window) number option (-)"
:echomsg "(no such option exists)"
:echomsg "local (to window) number option (foldcolumn)"
set foldcolumn=3
:echomsg "Boolean options (1 = local, 0 = global):"
:echomsg "global boolean option (wrapscan)"
set wrapscan
:echomsg "local-global (to buffer) boolean option (autoread)"
set autoread
:echomsg "local (to buffer) boolean option (cindent)"
set cindent
:echomsg "local-global (to window) boolean option (-)"
:echomsg "(no such option exists)"
:echomsg "local (to window) boolean option (cursorcolumn)"
set cursorcolumn
The v:old_value is the global one for global-local string options but the local one for number, boolean and other string options.
Keeping backwards compatibility would mean that the new v:option_old_alt would sometimes be the old global value and sometimes the old local value. So I guess we should break backwards compatibility.
If so, we can do this is two ways:
-
let
v:option_oldalways be the old global value. This breaks most options (all boolean and number as well as all non global-local string options), however, v:option_old would always refer to the global value. -
let
v:option_oldalways be the old local value except for global options there it would be the old global value. This would only break the global-local string options but has the inconsistency of global/local for v:option_old.
In both cases v:option_old_alt would be the other old value.
What should be the way forward?
Bram Moolenaar
> @bram, @andymass: I had a second look at the problem and found that
> `v:option_old` is handled inconsistently by vim (i.e. before this pull
> request). Take a look at the output of the following script:
> The `v:old_value` is the *global* one for global-local string options
> but the *local* one for number, boolean and other string options.
With what commands? I see a buffer-local option and a global-local
option behave the same way:
au! OptionSet * call ReportValues()
func ReportValues()
echomsg printf('%s (%s) was: %s, new: %s',
\ expand('<amatch>'), v:option_type,
\ v:option_old, v:option_new)
endfunc
set spelllang=en
echomsg printf('global: %s, local: %s', &g:spelllang, &l:spelllang)
setl spelllang=de
echomsg printf('global: %s, local: %s', &g:spelllang, &l:spelllang)
setg spelllang=fr
echomsg printf('global: %s, local: %s', &g:spelllang, &l:spelllang)
echomsg '----'
set stl=one
echomsg printf('global: %s, local: %s', &g:stl, &l:stl)
setl stl=two
echomsg printf('global: %s, local: %s', &g:stl, &l:stl)
setg stl=three
echomsg printf('global: %s, local: %s', &g:stl, &l:stl)
Output:
spelllang (global) was: de, new: en
global: en, local: en
spelllang (local) was: en, new: de
global: en, local: de
--1 spelllang (global) was: en, new: fr
global: fr, local: de
----
statusline (global) was: three, new: one
global: one, local:
statusline (local) was: one, new: two
global: one, local: two
--2 statusline (global) was: one, new: three
global: three, local: two
At --1 the change in the global value is reported, the local value
(which is effective) stays the same.
At --2 the same thing happens.
What is interesting when setting a global-local option to an empty
string, then it goes back to using the global value. So it's not clear
what old value to report then, we would to mention both.
> Keeping backwards compatibility would mean that the new
> `v:option_old_alt` would sometimes be the old global value and
> sometimes the old local value. So I guess we should break backwards
> compatibility.
>
> If so, we can do this is two ways:
>
> options (all boolean and number as well as all non global-local string
> options), however, v:option_old would always refer to the global
> value.
>
> options there it would be the old global value. This would only break
> the global-local string options but has the inconsistency of
> global/local for v:option_old.
>
> In both cases `v:option_old_alt` would be the other old value.
>
> What should be the way forward?
is. We can add v:option_old_local and v:option_old_global to avoid the
confusion for new code.
--
Mushrooms always grow in damp places and so they look like umbrellas.
/// 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 ///
—
latricewilgus
Hi @brammool
@bram, @andymass: I had a second look at the problem and found that
v:option_oldis handled inconsistently by vim (i.e. before this pull request). Take a look at the output of the following script:
Sorry, there is too much text here, please summarize your point.
The text you skipped was the code to reproduce the issue. It has a very important difference to the code you posted: The set command is used after setlocal/setglobal. (Note that the set command would be the only instance in the new code triggering v:option_old and v:option_old_alt to be present.)
When running my vim script you see that for a set command (not for the setglobal command as you pointed out in your code) the v:option_old is sometimes the old global value (for global-local string options) and sometimes the old local value (for boolean, number and other string options).
So there is an inconsistency whether this counts as a bug is up to debate.
@chrisbra If I'm not mistaken, you implemented that mechanism. Was this behaviour intentional?
Christian Brabandt
@latricewilgus I don't think that was intentional. However I couldn't clearly follow your script.
latricewilgus
@chrisbra Ok here the stripped down version of the script only testing certain options and only printing the v:option_old value:
" Run the script inside vim with ':so %'
" Set up the variables such that their
"
" - global value is 'G' or '8' and
" - local value is 'L' or '1'
" local-global (to buffer) number option
setlocal undolevels=1
setglobal undolevels=8
" local (to buffer) number option
setlocal wrapmargin=1
setglobal wrapmargin=8
" local-global (to buffer) string option
setlocal tags=L
setglobal tags=G
" local (to buffer) string option
setlocal spelllang=L
setglobal spelllang=G
" Create an autogroup putting out the v:option_old value
augroup test
autocmd!
autocmd OptionSet * :echomsg v:option_old
augroup END
" trigger the autocommand for different kinds of options
" local-global (to buffer) number option
set undolevels=3
" local (to buffer) number option
seta wrapmargin=3
" local-global (to buffer) string option
set tags=foo
" local (to buffer) string option
set spelllang=foo
" Output
" 1 (local-global number option) --- (1)
" 1 (local number option)
" G (local-global string option) --- (2)
" L (local string option)
" Mismatch in (1) and (2)!
As can be seen the v:option_old returns the values of different scopes depending on whether it is a local-global number or a local-global string option. In the first case it returns the local value, in the second the global one. Hope this makes it clear(er).
The question is: Is this a bug and if so should we fix it in this overhaul of the SetOption command?
Or should we go the way of leaving v:option_old as is and introduce v:option_old_local and v:option_old_global (as @brammool suggested in his previous comment).
Houl
(I think somebody mentioned this before) ... (maybe I don't get it)
Does this patch mean that every OptionSet event fills a v:event dict? Isn't this a bit wasted? I mean there are many cases where a lot of options will be set at once (saving, setting and restoring of options). Or is this only done when the user actually defines some OptionSet autocommands? Maybe I'll add a permanent :set ei+=OptionSet. Or maybe there will be no noticable speed penalty, that would be ok. Did somebody think about a different way of "syncing" options?
Houl
Isn't one of the problems that there is no notation of "undefined" for the local value of a global-local option?
For a missing string value, the empty string means "undefined" (which is less than ideal sometimes).
For a boolean option, looks like it's -1: :echo &l:autoread.
And for a number option? :echo &l:undolevels ...-123456 (er, rly?).
Bram Moolenaar
> @chrisbra Ok here the stripped down version of the script only testing certain options and only printing the `v:option_old` value:
> ```
> " Run the script inside vim with ':so %'
>
> " Set up the variables such that their
> "
> " - global value is 'G' or '8' and
> " - local value is 'L' or '1'
>
> " local-global (to buffer) number option
> setlocal undolevels=1
> setglobal undolevels=8
> " local (to buffer) number option
> setlocal wrapmargin=1
> setglobal wrapmargin=8
>
> " local-global (to buffer) string option
> setlocal tags=L
> setglobal tags=G
> " local (to buffer) string option
> setlocal spelllang=L
> setglobal spelllang=G
>
> " Create an autogroup putting out the v:option_old value
> autocmd!
> autocmd OptionSet * :echomsg v:option_old
> augroup END
>
> " trigger the autocommand for different kinds of options
>
> " local-global (to buffer) number option
> set undolevels=3
> " local (to buffer) number option
> seta wrapmargin=3
>
> " local-global (to buffer) string option
> set tags=foo
> " local (to buffer) string option
> set spelllang=foo
>
>
> " Output
> " 1 (local-global number option) --- (1)
> " 1 (local number option)
> " G (local-global string option) --- (2)
> " L (local string option)
>
> " Mismatch in (1) and (2)!
> ```
>
> As can be seen the `v:option_old` returns the values of different
> scopes depending on whether it is a local-global number or a
> local-global string option. In the first case it returns the local
> value, in the second the global one. Hope this makes it clear(er).
changes both the global and the local value. Both values change, so
v:option_old reporting either of them would be OK, although confusing.
One could argue that when changing 'statusline' the reported old value
is for the global value, but the effective value was the local value.
It depends on how you look at it.
> The question is: Is this a bug and if so should we fix it in this
> overhaul of the `SetOption` command?
> Or should we go the way of leaving v:option_old as is and introduce
> `v:option_old_local` and `v:option_old_global` (as @brammool suggested
> in his previous comment).
wrong, so let's not change it. Just add two variables, one for the old
global value and one for the old local value.
--
Friends? I have lots of friends! In fact, I have all episodes ever made.
/// 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 ///
—
Christian Brabandt
:echo &l:undolevels ...-123456
Yes, that was changed when the undolevels option was enhanced to be buffer-local (which I believe was on your request ;)). So we needed a special numeric value, that is not 0 and not -1. So this was used.
(Oh wow, the commit is almost 6 years ago, I still remember when I did that 😏
)
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
latricewilgus
@Houl wrote:
Does this patch mean that every OptionSet event fills a v:event dict? Isn't this a bit wasted?
Not any more. The updated implementation will set two additional (string) variables v:option_old_local and v:option_old_global.
@Houl wrote:
Did somebody think about a different way of "syncing" options?
I did in so far that I did not find any way with vim's current capabilities. If you have additional ideas (mechanismwise, implementationwise, or otherwise), I'm all ears.
Codecov
Codecov Report
Merging #4118 into master will increase coverage by
0.09%.
The diff coverage is98.78%.
@@ Coverage Diff @@ ## master #4118 +/- ## ========================================== + Coverage 80.38% 80.47% +0.09% ========================================== Files 105 109 +4 Lines 142461 142803 +342 ========================================== + Hits 114519 114925 +406 + Misses 27942 27878 -64
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/option.c | 86.33% <100%> (+0.23%) |
⬆️ |
| src/eval.c | 86.05% <80%> (+0.03%) |
⬆️ |
| src/json_test.c | 100% <0%> (ø) |
|
| src/memfile_test.c | 100% <0%> (ø) |
|
| src/kword_test.c | 66.66% <0%> (ø) |
|
| src/message_test.c | 100% <0%> (ø) |
|
| src/gui_gtk_x11.c | 48.67% <0%> (+0.04%) |
⬆️ |
| src/move.c | 87.3% <0%> (+0.08%) |
⬆️ |
| src/term.c | 73.63% <0%> (+0.1%) |
⬆️ |
| ... and 9 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered by Codecov. Last update 8f46e4c...9ccde81. Read the comment docs.
—
You are receiving this because you are subscribed to this thread.
latricewilgus
Ok ready for a review.
Note that the name v:option_old_global has 17 characters but the current limit for option variables is 16. So I increased the limit to 17 (and in the process renamed its type to reflect that). During that I also found a magic number in the code which I replaced by a #define constant DICTITEM17_KEY_LEN.
The coverage test fails. However, it claims twice a decrease for files (src/window.c and src/gui_gtk_x11.c) I didn't even touch nor where I see any correlation to the option handling code.
The last decrease is a line in eval.c and this is expected as this is basically a compile time check: all variables and their length are known at compile time. So this code is never run at all.
latricewilgus
Any comments on this?
Andy Massimino
I personally don't like the bump up from dictitem16_T to dictitem17_T; it adds noise to the patch and 17 is not a nice number compared to 16. Is it better to shorten the name?
Bram Moolenaar
v:option_oldglobal ?
latricewilgus
Ok, I renamed the variables to v:option_oldglobal and v:option_oldlocal (so it is consistent).
I also dropped the patch for renaming dictitem16_* to dictitem17_*. However, I've included a patch which removes the magic number 16 in eval_init() and replaces it with a #define-constant. Feel free to omit this patch or add it as a separate pull request/patch series.
@andymass wrote:
17 is not a nice number compared to 16.
The underlying data-structure in dictitem16_* has the length 17. As the number 18 is clearly more pleasing than 17, dictitem17_* is the nicer data type (it has length 18 after all). ;-)
K.Takata
@k-takata commented on this pull request.
In src/vim.h:
> +#define VV_TYPE_LIST 77 +#define VV_TYPE_DICT 78 +#define VV_TYPE_FLOAT 79 +#define VV_TYPE_BOOL 80 +#define VV_TYPE_NONE 81 +#define VV_TYPE_JOB 82 +#define VV_TYPE_CHANNEL 83 +#define VV_TYPE_BLOB 84 +#define VV_TERMRFGRESP 85 +#define VV_TERMRBGRESP 86 +#define VV_TERMU7RESP 87 +#define VV_TERMSTYLERESP 88 +#define VV_TERMBLINKRESP 89 +#define VV_EVENT 90 +#define VV_VERSIONLONG 91 +#define VV_LEN 92 /* number of v: vars */
// should be used for the comment here.
K.Takata
@k-takata commented on this pull request.
In src/option.c:
> @@ -8451,6 +8516,12 @@ set_bool_option( ) && (options[opt_idx].flags & P_SECURE)) return e_secure; + /* Safe the global value before changeing anything. This is needed as for
s/Safe/Save/ ?
Christ van Willegen
@k-takata commented on this pull request.
In src/option.c:
> @@ -8451,6 +8516,12 @@ set_bool_option( ) && (options[opt_idx].flags & P_SECURE)) return e_secure; + /* Safe the global value before changeing anything. This is needed as fors/Safe/Save/ ?
vim-dev ML
> ------------------------------
>
> In src/option.c
> <https://github.com/vim/vim/pull/4118#discussion_r294029144>:
>
> > @@ -8451,6 +8516,12 @@ set_bool_option(
> ) && (options[opt_idx].flags & P_SECURE))
> return e_secure;
>
> + /* Safe the global value before changeing anything. This is needed as for
>
> s/Safe/Save/ ?
>
Also, it should be "changing"
Christ van Willegen
>
latricewilgus
Updated patchset according to @k-takata's and Christ's suggestions. Thanks for the review guys!
—
You are receiving this because you commented.