barbar.nvim icon indicating copy to clipboard operation
barbar.nvim copied to clipboard

`BufferlineOnOptionChanged` is not called when setting `g:bufferline` from Lua

Open Iron-E opened this issue 2 years ago • 2 comments

I discovered this while investigating #218. To reproduce, run this script:

" Must be global -_-
function! BufferlineOnOptionChanged(dict, key, changes) abort
  echom "foo"
  call luaeval("require'bufferline'.on_option_changed(nil, _A, nil)", a:key)
endfunction

call dictwatcheradd(g:bufferline, '*', 'BufferlineOnOptionChanged')

Then run:

let g:bufferline.foo = v:true " foo is echoed

Finally, run:

vim.g.bufferline.foo = true -- nothing is echoed

This issue will have been longstanding since vim.g was created. I can think of two solutions:

  1. Remove vim.g.bufferline in favor of requiring users to use bufferline.setup (breaking change, not ideal)
  2. Create a bufferline.options.get() function that runs BufferlineOnOptionChanged on vim.g.bufferline before internal use (poor performance, not ideal)
  3. Make a meta-table for vim.g that manages vim.g.bufferline (pollutes shared resource, not ideal)

There exists a OptionChanged autocommand event, but that is only for builtin options e.g. :set foo. AFAIK there is no lua-equivalent to dictwatchadd(), and if we were to use metatables then it would be broken for users with VimScript (and any Lua user who did vim.g.bufferline = {} after)

Is there a solution I haven't thought of? If not, option two seems like the best out of the three.

Iron-E avatar May 27 '22 19:05 Iron-E

I'm not sure what's the real issue here?

If we need to modify the dict, the solution would be: vim.cmd('let g:bufferline.foo = v:true'). If we want to help users modify their config, yes a setup() function could be exposed. But I'm not sure how useful this would be? Seems like doing so would be quite rare.

romgrk avatar May 31 '22 07:05 romgrk

If we need to modify the dict, the solution would be: vim.cmd('let g:bufferline.foo = v:true')

You're right, this is only a user-facing issue. If we know we are changing the dict we can call the on_setup_changed ourselves and it isn't an issue. The problem comes from what needs to happen whenever vim.g.bufferline is changed by a user. For context, here is what needs to happen in that case:

function bufferline.on_option_changed(_, key, _)
  vim.g.bufferline = vim.tbl_extend('keep', vim.g.bufferline or {}, DEFAULT_OPTIONS)
  if key == 'letters' then
    require'bufferline.jump_mode'.initialize_indexes()
  end
end

In #218, packer.nvim was setting vim.g.bufferline after running the initial setup so all of our defaults not present in the user config were overwritten to nil. Thus, when we were looking to reference them (e.g. vim.g.bufferline.animation) it would be nil rather than a boolean value. Since we expose bufferline.setup I told them to use that, though this seems like a potential footgun for packer.nvim users :sweat_smile:

Not only that — and while this is rare, as you say — if a user changes the vim.g.bufferline.letters from Lua as opposed to VimScript there will be no re-initialization of the jump mode indices. However if a user does let g:bufferline.letters then it will work.


To summarize, we have a need to assure that on_option_changed is run when the user sets vim.g.bufferline (which successfully happens on startup if vim.g.bufferline is set, and whenever let g:bufferline occurs), but it is possible for the user to edit this table outside of a VimScript setting and/or after the initial bufferline.setup(vim.g.bufferline) call on Neovim's startup. This leads to errors when the state of the user config is not what the plugin expects, so issues like #218 might crop up later depending on how a user has implemented their init file.

Iron-E avatar May 31 '22 14:05 Iron-E