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(tabs): use separators matching current style #714

Merged
merged 3 commits into from
Mar 28, 2023

Conversation

matthis-k
Copy link
Contributor

@matthis-k matthis-k commented Mar 22, 2023

This is the PR for #569.
This works perfectly for slant and slope, but I am not sure what the behavior for thick and thin is supposed to be.
If you want the behavior to be unchanged I (or you) can change it this to this, which should only change the slant and slope and custom styles:

  local single_char = require("bufferline.config").options.separator_style == "thick"
    or require("bufferline.config").options.separator_style == "thin"
  return {
    single_char and {} or { highlight = separator_hl, text = chars[2] },
    {
      highlight = hl,
      text = name,
      attr = { prefix = tab_click_component(tabpage.tabnr) },
    },
    { highlight = separator_hl, text = chars[single_char and 2 or 1] },
  }

@akinsho
Copy link
Owner

akinsho commented Mar 24, 2023

Hi @matthis-k thanks for the PR, whilst it would be good to have this fixed I definitely would rather not have any regressions with it since this will definitely lead to 1 issue being closed and 3 opened to replace it.

Currently I see

Screenshot 2023-03-24 at 10 53 23

When using your PR i.e. there are strange gaps in between characters which has never historically been the case. I've also never been able to reproduce the parent issue here and am curious what terminal emulator causes this

@matthis-k
Copy link
Contributor Author

matthis-k commented Mar 27, 2023

I think I fixed my code, I think the problem was that in your code slant only used one char instead of one on each side.

Here are the results:

  • thin
    image
  • thick
    image
  • slant
    image
  • slope
    image

The only thing that might be left is modifying the x button to match the style, by adding the character that is on the left in front of it, to match the tabs

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.

@matthis-k thanks for tweaking it, seems like it looks better now. I've made some code style comments since I think the require is unnecessary and the conditional can be terser still.

local separator_component = chars[2]
local name = padding .. padding .. tabpage.tabnr .. padding
local name = padding .. tabpage.tabnr .. padding
local swapped_chars = require("bufferline.config").options.separator_style == "thick"
Copy link
Owner

Choose a reason for hiding this comment

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

This import should match those above rather than be inline like this i.e.

---@module "bufferline.config"
local config = lazy.require("bufferline.config")

Although it appears the style is already being passed into this function as an argument so I don't thing you need to even be requiring the config at all.

Copy link
Owner

Choose a reason for hiding this comment

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

This check could also be done a little more tersely with something like

local style = config.options.separator_style
local swapped_chars = ({thick = {1, 2}, thin = {1, 2}})[style] or {2, 1}
return {
 { highlight = separator_hl, text = swapped_chars[1] },
 -- ...
 { highlight = separator_hl, text = swapped_chars[2] },

@akinsho akinsho changed the base branch from main to dev March 27, 2023 20:51
@akinsho akinsho changed the title Fix: tabs do not use seperators correctly feat(tabs): use separators matching current style Mar 27, 2023
@akinsho
Copy link
Owner

akinsho commented Mar 27, 2023

It's also worth clarifying that it isn't a bug that the sloped style and the minimal tabs don't look the same, they were never intended to. This is a good change but just for clarity the minimal tabs were always initially intended to use the thin style regardless of if a user set sloped or anything like that

@matthis-k
Copy link
Contributor Author

matthis-k commented Mar 27, 2023

It's also worth clarifying that it isn't a bug that the sloped style and the minimal tabs don't look the same, they were never intended to. This is a good change but just for clarity the minimal tabs were always initially intended to use the thin style regardless of if a user set sloped or anything like that

Oh I see. I thought it was spposed to work, as you would still use the chars from the style here: local chars = constants.sep_chars[style] or constants.sep_chars.thin (so it should be terminal agnostic), but I guess it's in the past now

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 now 👍🏿 thanks for the changes @matthis-k

@akinsho akinsho merged commit fafc5d2 into akinsho:dev Mar 28, 2023
1 check passed
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.

None yet

2 participants