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

Allow different configuration for BufferLinePick #972

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

joaovfsousa
Copy link
Contributor

@joaovfsousa joaovfsousa commented Oct 22, 2024

I've added some new options to allow me to only use my homerow keys to switch between buffers. I think it might be interesting for other people too, so let me know if you're interested in merging it and/or if it needs any modifications.

The default behavior is still the same.

Copy link
Owner

@akinsho akinsho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joaovfsousa thanks for the PR generally looks good to me just a couple of comments nothing too major I think

@@ -35,15 +33,18 @@ end
---@param element bufferline.Tab|bufferline.Buffer
---@return string?
function M.get(element)
local valid_alphabet = config.options.pick.alphabet
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the multiple uses of config.options.pick could be aliased, just a stylistic preference but I don't like have these deep access all over the place on the off chance that ever needed to change there'd be multiple areas in the code to tweak.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akinsho done

Comment on lines 687 to 688
With the configuration above, bufferline will use the first letter available
in the alphabet.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be me missing this a bit but from this description I'm still a tiny bit unclear on how this would work as a reader, does this mean that by disabling this it will just pick the next arbitrarily available letter?

Is there a particular need for this whilst flexibility is generally nice I find that from the plugin developers perspective it tends to mean a larger surface area for issues/bugs/feature requests/complaints so I try to be very clear about if adding things is strictly necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this mean that by disabling this it will just pick the next arbitrarily available letter?
Yes, that's it.

Well, althought setting the alphabet already makes it almost perfect, the order may become weird in some situations.
For example, let's say I have the alphabet set to 'asdfghjkl' and 3 open buffers in order: 'abc.txt', 'def.txt', 'xyz.txt'. The letters assigned without this option would be, respectively, 'a', 'd' and 's', instead of the more natural(in my opinion and for my workflow) 'a', 's' and 'd', because the second buffer's first letter is in the alphabet.

As I said, it's very specific to my workflow, so I completely understand if you find it doesn't make sense to merge to mainstream.

Another alternative I found was to have something similar to name_formatter in the config.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh @joaovfsousa the name formatter is a perfect example of what I describe of something I added for flexibility that I have had several requests to tweak to pass in x,y,z and the kitchen sink for so many hyper specific usecases on particular person has

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense.

So, how do you wanna proceed? Go with only the alphabet part or close the PR?
@akinsho

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joaovfsousa I think the PR is useful to have, I think I'd ideally just stick with the alphabet and see if the other use case comes up frequently enough to address.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akinsho I updated the PR accordingly.

Copy link
Owner

@akinsho akinsho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @joaovfsousa haven't had time to actually test this so hopefully it's working as expected. Will have to roll back if there are any complaints 🤞🏿

@akinsho akinsho merged commit 5cc447c into akinsho:main Oct 24, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants