romgrk / barbar.nvim

The neovim tabline plugin.
2.2k stars 81 forks source link

feat: disable auto-setup #438

Closed Iron-E closed 1 year ago

Iron-E commented 1 year ago

Enables users to disable the auto-setup in plugin/barbar.lua. All you have to do is:

vim.g.barbar_auto_setup = false
let g:barbar_auto_setup = v:false

Closes #431

Iron-E commented 1 year ago

barbar_auto_setup = false might be better. Let me know what you think

romgrk commented 1 year ago

I don't like that users might need to add an additional line of code outside their packages config just for us. Don't we have another way to fix this? The issue is that we'd want to run after lazy's config call right? Can't we find a way to do that? Does VimEnter work for us?

Iron-E commented 1 year ago

The issue is that we'd want to run after lazy's config call right?

There's a few things going on here, which could happen with any plugin manager (not just lazy):

  1. a setup is being run automatically on startup, which sometimes uses options that are incompatible with a user's configuration (#431);
  2. the automatic setup makes implementing lazy-loading for this plugin unlike others (#415); and
  3. any user who customizes barbar via setup at all is effectively doubling the effect that this plugin has on their startup time, because setup is called twice.

I'm attempting to solve all three problems with vim.g.barbar_auto_setup.

Can't we find a way to do that? Does VimEnter work for us?

I thought about that, but unfortunately, no. Here is a collection of valid lazy configs to illustrate (but again, you could reproduce this with packer as well):

{'romgrk/barbar.nvim', event = 'BufEnter', config = true} -- works with VimEnter
{'romgrk/barbar.nvim', event = 'BufEnter', config = false} -- broken with VimEnter
{'romgrk/barbar.nvim', lazy = false} -- works with VimEnter
{'romgrk/barbar.nvim', lazy = true} -- sometimes works with VimEnter

Additionally, my own setup only sometimes works with the VimEnter autocmd solution. If more than two buffers open from the shell (nvim a b), the our VimEnter event will fire. Otherwise, nvim then :edit a | edit b it will fail.

This is kind of a halting problem: how can we guarantee, from the context of plugin/barbar.lua, that a user doesn't call setup themselves? I've thought about this a for a bit, and I can't come up with an automatic solution. We can vim.defer_fn, but the user might delay calling setup for an amount of time greater. We can use an autocmd, but a user might use a later one. The only way I've thought of to guarantee this is for the user to annotate that they are calling setup themselves.

Of course, we could grep the user's stdpath, but that seems really smelly (and doesn't even guarantee it— someone could be using a pre-configured plugin group)

I don't like that users might need to add an additional line of code outside their packages config just for us.

A couple things about this:

  1. If someone is using lazy, they can include this statement in their plugin spec:
    require('lazy').setup {
    {'romgrk/barbar.nvim',
      dependencies = 'nvim-tree/nvim-web-devicons',
      init = function() vim.g.barbar_setup = true end, -- ← here
      opts = {animation = true, insert_at_start = true, --[[…etc]]},
      version = '^1.0.0',
    },
    }
  2. If someone isn't using lazy, they and they aren't calling setup by themselves, then they don't need to set this variable (because they want auto-setup)
  3. If someone isn't using lazy, they and they are calling setup by themselves (e.g. #431), they already need to add additional lines of code outside their package's config just for us (i.e. require('barbar').setup({…})) so it's just one more line.

See the modified README in this PR for in-context examples.


FWIW I agree with you, I wish there was a better way (but there's nothing I can see). Issues like these are why I think we should do away with auto-setup in v2: we're back to g: variables and setup which we just tried to simplify (though I agree it'd hurt QoL)

Iron-E commented 1 year ago

@lucassperez I changed it to barbar_auto_setup = false

romgrk commented 1 year ago

The root cause of all this is of course that some idiot decided that adding a .setup() lua function was a nice way to configure plugins, instead of using the standard passive global variables approach that prevented these kind of issues -_-

Anyway, this is temporary until we migrate away from g:bufferline, right? At which point we'll just use a .setup() function like everyone else resolved to do.

romgrk commented 1 year ago

Ideally I'd like if we'd stop migrating & breaking stuff like this for a while, plan everything we want to break, and break everything at once so users only have to worry about us once and then they're off the hook with our changes.

Iron-E commented 1 year ago

Anyway, this is temporary until we migrate away from g:bufferline, right? At which point we'll just use a .setup() function like everyone else resolved to do.

We'll have to keep it if we keep auto-setup, unfortunately. If we require users to call setup themselves it can be removed :/

romgrk commented 1 year ago

I'm fine requiring users to call setup. Not because it's a good option, but simply because many other plugins do it so we should just follow the trend to give our users a consistent UX with other plugins. I hate that neovim devs didn't create stronger standards when they had the chance to.

Iron-E commented 1 year ago

Ideally I'd like if we'd stop migrating & breaking stuff like this for a while, plan everything we want to break, and break everything at once so users only have to worry about us once and then they're off the hook with our changes.

We should definitely discuss the items in #394 at some point. g:bufferline was deprecated just in the notion of giving users advance notice (re: https://github.com/romgrk/barbar.nvim/issues/394#issuecomment-1489634044) but no further work has been done yet


Edit:

I'm fine requiring users to call setup. Not because it's a good option, but simply because many other plugins do it so we should just follow the trend to give our users a consistent UX with other plugins

Alright, I'll tick it on the v2 changes list.

romgrk commented 1 year ago

Well, I hate this PR but you can merge it, there is no better option and it patches the issue for now.

Iron-E commented 1 year ago

My apologies :pray: