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.

Sign up for GitHub

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

Jump to bottom

vim._with() does not properly restore global-local options #29253

Closed
echasnovski opened this issue Jun 9, 2024 · 3 comments · Fixed by #29280
Closed

vim._with() does not properly restore global-local options #29253

echasnovski opened this issue Jun 9, 2024 · 3 comments · Fixed by #29280
Assignees
@dundargoc
Labels
bug issues reporting wrong behavior

Comments

@echasnovski
Copy link
Member

Problem

When using options context in vim._with(), it can result into unwanted side effects.

Steps to reproduce

  1. Create the 'script.lua' file with the following content: 
    vim.go.commentstring, vim.bo.commentstring = '- %s', '-- %s'

_G.log = {}
table.insert(_G.log, { 'Before', go = vim.go.commentstring, bo = vim.bo.commentstring })
vim._with({ options = { commentstring = '## %s' } }, function()
  table.insert(_G.log, { 'Inside', go = vim.go.commentstring, bo = vim.bo.commentstring })
end)
table.insert(_G.log, { 'After', go = vim.go.commentstring, bo = vim.bo.commentstring })
  2. nvim --clean -- script.lua
  3. :source
  4. :=log.

The output now (notice difference in go values):

:=log
{ { "Before",
    bo = "-- %s",
    go = "- %s"
  }, { "Inside",
    bo = "## %s",
    go = "## %s"
  }, { "After",
    bo = "-- %s",
    go = "-- %s"
  } }

Expected behavior

Both global and local option values are the same after executing vim._with() as they were before it.

Neovim version (nvim -v)

NVIM v0.11.0-dev-200+g9afa1fd35

Vim (not Nvim) behaves the same?

No (doesn't have this functionality)

Operating system/version

EndeavourOS Linux x86_64 (6.9.3-arch1-1)

Terminal name/version

st-0.9

$TERM environment variable

st-256color

Installation

appimage
@echasnovski echasnovski added the bug issues reporting wrong behavior label Jun 9, 2024
@echasnovski
Copy link
Member Author

I think the better design might be to have explicit go, wo, bo, maybe o, and maybe env as separate context tables instead of a single options. This will allow temporarily setting options (and environment variables) with as much precision as user/callback wants.

@dundargoc
Copy link
Member

dundargoc commented Jun 10, 2024 •
edited
Loading

I think the better design might be to have explicit go, wo, bo, maybe o, and maybe env as separate context tables instead of a single options. This will allow temporarily setting options (and environment variables) with as much precision as user/callback wants.

I'm cautiously optimistic about this suggestion. My only worry is that it will sacrifice convenience over maximal flexibility. I assume one would in majority of the cases want to change the options of the buffer you switched to rather than another buffer.

I'm guessing you were thinking something like the following signature?

vim._with({buf = 1, bo = {buf = 2, options = { comments = spelllang= 'fr'}}}, function()
end)

It is for sure more flexible. I don't mind testing this out and see if it's something we like.

I was wondering though if this couldn't be achieved with the current approach already? I haven't tested this out but something like the following should work (or at least be made to work):

vim._with({buf = 2, options = { comments = spelllang= 'fr'}}}, function()
        vim._with({buf = 1}, function()
        end)
end)

There is about as much code, but we'd be avoiding the o, bo, go, wo spaghetti.

dundargoc added a commit to dundargoc/neovim that referenced this issue Jun 10, 2024
@dundargoc
fix(vim._with): don't modify global-local options
2efabeb 
The global options are not properly restored after using `vim._with`.
Circumvent this problem by always using "local" scope. A solution to
allow modifying global options might be added later on, but this is a
good enough fix for the time being.

Closes neovim#29253.
dundargoc added a commit to dundargoc/neovim that referenced this issue Jun 10, 2024
@dundargoc
fix(vim._with): don't modify global-local options
64388a9 
The global options are not properly restored after using `vim._with`.
Circumvent this problem by always using "local" scope. A solution to
allow modifying global options might be added later on, but this is a
good enough fix for the time being.

Closes neovim#29253.
@dundargoc dundargoc mentioned this issue Jun 10, 2024
Closed
@echasnovski
Copy link
Member Author

I'm guessing you were thinking something like the following signature?

vim._with({buf = 1, bo = {buf = 2, options = { comments = spelllang= 'fr'}}}, function()
end)

No, not quite. I had in mind one nesting level less:

vim._with({ buf = 1, bo = { commentstring = '## %s' }, go = { spelllang = 'fr' } }, callback)

Which is essentially a thin wrapper for the following:

vim._with({ buf = 1 }, function()
  local cache_bo_cms, cache_go_spl = vim.bo.commentstring, vim.go.spelllang
  vim.bo.commentstring, vim.go.spelllang = '## %s', 'fr'
  callback()
  vim.bo.commentstring, vim.go.spelllang = cache_bo_cms, cache_go_spl
end)

This both reuses existing notation for global-local options and allows precise explicit intention on which option is meant to be used in context.

dundargoc added a commit to dundargoc/neovim that referenced this issue Jun 10, 2024
@dundargoc
feat(vim._with): allow specifying scope-specific options
fb016b7 
Replace `options` context with `bo`, `wo` and `go` contexts. This allows
users to specify which scope the option should work on.

Closes neovim#29253.
@dundargoc dundargoc mentioned this issue Jun 10, 2024
Closed
dundargoc added a commit to dundargoc/neovim that referenced this issue Jun 10, 2024
@dundargoc
feat(vim._with): allow specifying scope-specific options
972c9f9 
Replace `options` context with `bo`, `wo` and `go` contexts. This allows
users to specify which scope the option should work on.

Also add `env` context for changing `vim.env`.

Closes neovim#29253.
dundargoc added a commit to dundargoc/neovim that referenced this issue Jun 10, 2024
@dundargoc
feat(vim._with): allow specifying scope-specific options
c10795b 
Replace `options` context with `o`, `bo`, `wo` and `go` contexts. This
allows users to specify which scope the option should work on.

Also add `env` context for changing `vim.env`.

Closes neovim#29253.
dundargoc added a commit to dundargoc/neovim that referenced this issue Jun 10, 2024
@dundargoc
feat(vim._with): allow specifying scope-specific options
38f092f 
Replace `options` context with `o`, `bo`, `wo` and `go` contexts. This
allows users to specify which scope the option should work on.

Also add `env` context for changing `vim.env`.

Closes neovim#29253.
dundargoc added a commit to dundargoc/neovim that referenced this issue Jun 10, 2024
@dundargoc
feat(vim._with): allow specifying scope-specific options
8e6f1c2 
Add optional `scope` parameter to each option.

Closes neovim#29253.

---

Alternative to neovim#29268.
Alternative to neovim#29269.
Alternative to touching grass.

Will add tests once we've decided the the design/function signature.
@dundargoc dundargoc mentioned this issue Jun 10, 2024
Closed
dundargoc added a commit to dundargoc/neovim that referenced this issue Jun 10, 2024
@dundargoc
feat(vim._with): allow specifying scope-specific options
6102eda 
Add optional `scope` parameter to each option.

Closes neovim#29253.
dundargoc added a commit to dundargoc/neovim that referenced this issue Jun 10, 2024
@dundargoc
feat(vim._with): allow specifying scope-specific options
f288967 
Replace `options` context with `o`, `bo`, `wo` and `go` contexts. This
allows users to specify which scope the option should work on.

Also add `env` context for changing `vim.env`.

Closes neovim#29253.
dundargoc added a commit to dundargoc/neovim that referenced this issue Jun 11, 2024
@dundargoc
feat(vim._with): allow specifying scope-specific options
6aa50ae 
Replace `options` context with `o`, `bo`, `wo` and `go` contexts. This
allows users to specify which scope the option should work on.

Also add `env` context for changing `vim.env`.

Closes neovim#29253.
@echasnovski echasnovski mentioned this issue Jun 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dundargoc dundargoc

Labels
bug issues reporting wrong behavior
Projects
None yet
Milestone
No milestone
2 participants
@echasnovski @dundargoc