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.

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

feat: disable auto-setup #438

Merged
merged 2 commits into from
Apr 5, 2023
Merged

feat: disable auto-setup #438

merged 2 commits into from
Apr 5, 2023

Conversation

Iron-E
Copy link
Collaborator

@Iron-E Iron-E commented Apr 4, 2023

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 Iron-E added the request New feature or request label Apr 4, 2023
@Iron-E Iron-E requested a review from romgrk April 4, 2023 23:29
@Iron-E Iron-E self-assigned this Apr 4, 2023
@Iron-E
Copy link
Collaborator Author

Iron-E commented Apr 5, 2023

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

@romgrk
Copy link
Owner

romgrk commented Apr 5, 2023

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
Copy link
Collaborator Author

Iron-E commented Apr 5, 2023

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 (Can't disable icons without nvim dev icons #431);
  2. the automatic setup makes implementing lazy-loading for this plugin unlike others (Lazy init hindered? #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',
  },
}
  1. 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)
  2. If someone isn't using lazy, they and they are calling setup by themselves (e.g. Can't disable icons without nvim dev icons #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
Copy link
Collaborator Author

Iron-E commented Apr 5, 2023

@lucassperez I changed it to barbar_auto_setup = false

@romgrk
Copy link
Owner

romgrk commented Apr 5, 2023

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
Copy link
Owner

romgrk commented Apr 5, 2023

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
Copy link
Collaborator Author

Iron-E commented Apr 5, 2023

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
Copy link
Owner

romgrk commented Apr 5, 2023

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
Copy link
Collaborator Author

Iron-E commented Apr 5, 2023

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: #394 (comment)) 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
Copy link
Owner

romgrk commented Apr 5, 2023

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

@Iron-E
Copy link
Collaborator Author

Iron-E commented Apr 5, 2023

My apologies 🙏

@Iron-E Iron-E merged commit d4d0e06 into master Apr 5, 2023
@Iron-E Iron-E deleted the fix/431 branch April 5, 2023 17:35
@Iron-E Iron-E mentioned this pull request Apr 5, 2023
7 tasks
frantisekstanko added a commit to frantisekstanko/dotfiles that referenced this pull request Jun 14, 2023
This is the most minimal look I could achieve with barbar.
Such minimal look was impossible with lualine.

Explanation of the configuration as it is now:

- `barbar_auto_setup = false` and using `config` instead of `opts` is
  because this is the only way how to disable icons and not get an
  error. for more info, see: romgrk/barbar.nvim#438

- I did not find a better way to specify the colors (that is unrelated
  to the previous point)

- the empty string in `separator.left` must be there, otherwise,
  if this value is empty, the tabs jump when cycling through them

- to focus the tab on the right when closing a tab is what feels the
  most natural to me

- inserting a new tab at the end is also what feels the most natural

- maximum_padding set to 0 instead of 1 seems to change nothing. this
  makes me uncertain if 0 is a valid value. I am therefore setting it
  to "1".
frantisekstanko added a commit to frantisekstanko/dotfiles that referenced this pull request Jun 14, 2023
This is the most minimal look I could achieve with barbar.
Such minimal look was impossible with lualine.

Explanation of the configuration as it is now:

- `barbar_auto_setup = false` and using `config` instead of `opts` is
  because this is the only way how to disable icons and not get an
  error. for more info, see: romgrk/barbar.nvim#438

- I did not find a better way to specify the colors (that is unrelated
  to the previous point)

- the empty string in `separator.left` must be there, otherwise,
  if this value is empty, the tabs jump when cycling through them

- to focus the tab on the right when closing a tab is what feels the
  most natural to me

- inserting a new tab at the end is also what feels the most natural

- maximum_padding set to 0 instead of 1 seems to change nothing. this
  makes me uncertain if 0 is a valid value. I am therefore setting it
  to "1".
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't disable icons without nvim dev icons
2 participants