akinsho / bufferline.nvim

A snazzy bufferline for Neovim
GNU General Public License v3.0
3.4k stars 194 forks source link

[Bug]: separator_visible is not used from the highlights for custom separator style #924

Open maulik13 opened 2 months ago

maulik13 commented 2 months ago

Is there an existing issue for this?

Yes, but it was closed. https://github.com/akinsho/bufferline.nvim/issues/859

I understand that you do not want to provide full flexibility, however what is provided in the plugin should work consistently. The option with custom separators in separator_style is only about taking in left and right characters, the coloring of them should be the same as standard options like "slant".

I am open to creating a PR for this.

What happened?

When configuring highlights separator_visible has no effect on separators set using separator_style = {'', ''}. I did not find any reference to that parameter in the code when using individual separators.

What did you expect to happen?

Plugin should respect visible, selected for custom separators the same way it does for standard separator styles.

Config

I am using LazyVim to load the plugin.

-- Customize the entire bufferline theme from catppuccin
local function getHighlights()
  local C = require("catppuccin.palettes").get_palette("macchiato")
  local O = require("catppuccin").options
  local transparent_background = O.transparent_background
  local active_bg = transparent_background and "NONE" or C.crust
  local inactive_bg = transparent_background and "NONE" or C.base
  local separator_fg = O.transparent_background or C.crust
  local inactive_fg = C.surface2
  local styles = { "bold", "italic" }

  return {
    -- buffers
    background = { bg = inactive_bg, fg = inactive_fg },
    buffer_visible = { fg = inactive_fg, bg = inactive_bg },
    buffer_selected = { fg = C.text, bg = active_bg, bold = true, italic = true },
    -- Duplicate
    duplicate_selected = { fg = C.text, bg = active_bg, bold = true, italic = true },
    duplicate_visible = { fg = C.surface1, bg = inactive_bg, bold = true, italic = true },
    duplicate = { fg = C.surface1, bg = inactive_bg, bold = true, italic = true },
    -- tabs
    tab = { fg = C.surface2, bg = inactive_bg },
    tab_selected = { fg = C.sky, bg = C.surface1, bold = true, italic = true },
    tab_separator = { fg = separator_fg, bg = inactive_bg },
    tab_separator_selected = { fg = C.surface1, bg = C.surface1 },
    tab_close = { fg = C.red, bg = inactive_bg },

    indicator_visible = { fg = C.peach, bg = inactive_bg, bold = true, italic = true },
    indicator_selected = { fg = C.peach, bg = active_bg, bold = true, italic = true },
    -- separators
    separator = { fg = C.red, bg = inactive_bg },
    separator_visible = { fg = inactive_bg, bg = inactive_bg },
    separator_selected = { fg = inactive_bg, bg = active_bg },
    offset_separator = { fg = C.red, bg = inactive_bg },
    -- close buttons
    close_button = { fg = C.surface1, bg = inactive_bg },
    close_button_visible = { fg = C.surface1, bg = inactive_bg },
    close_button_selected = { fg = C.red, bg = active_bg },
    -- Empty fill
    fill = { bg = inactive_bg },
    -- Numbers
    numbers = { fg = C.subtext0, bg = inactive_bg },
    numbers_visible = { fg = C.subtext0, bg = inactive_bg },
    numbers_selected = { fg = C.subtext0, bg = active_bg, bold = true, italic = true },
    -- Errors
    error = { fg = inactive_fg, bg = inactive_bg },
    error_visible = { fg = inactive_fg, bg = inactive_bg },
    error_selected = { fg = C.red, bg = active_bg, bold = true, italic = true },
    error_diagnostic = { fg = inactive_fg, bg = inactive_bg },
    error_diagnostic_visible = { fg = inactive_fg, bg = inactive_bg },
    error_diagnostic_selected = { fg = C.red, bg = active_bg },
    -- Warnings
    warning = { fg = inactive_fg, bg = inactive_bg },
    warning_visible = { fg = inactive_fg, bg = inactive_bg },
    warning_selected = { fg = C.yellow, bg = active_bg, bold = true, italic = true },
    warning_diagnostic = { fg = inactive_fg, bg = inactive_bg },
    warning_diagnostic_visible = { fg = inactive_fg, bg = inactive_bg },
    warning_diagnostic_selected = { fg = C.yellow, bg = active_bg },
    -- Infos
    info = { fg = inactive_fg, bg = inactive_bg },
    info_visible = { fg = inactive_fg, bg = inactive_bg },
    info_selected = { fg = C.sky, bg = active_bg, bold = true, italic = true },
    info_diagnostic = { fg = inactive_fg, bg = inactive_bg },
    info_diagnostic_visible = { fg = inactive_fg, bg = inactive_bg },
    info_diagnostic_selected = { fg = C.sky, bg = active_bg },
    -- Hint
    hint = { fg = inactive_fg, bg = inactive_bg },
    hint_visible = { fg = inactive_fg, bg = inactive_bg },
    hint_selected = { fg = C.teal, bg = active_bg, bold = true, italic = true },
    hint_diagnostic = { fg = inactive_fg, bg = inactive_bg },
    hint_diagnostic_visible = { fg = inactive_fg, bg = inactive_bg },
    hint_diagnostic_selected = { fg = C.teal, bg = active_bg },
    -- Diagnostics
    diagnostic = { fg = inactive_fg, bg = inactive_bg },
    diagnostic_visible = { fg = inactive_fg, bg = inactive_bg },
    diagnostic_selected = { fg = C.subtext0, bg = active_bg, style = styles },
    -- Modified
    modified = { fg = C.peach, bg = inactive_bg },
    modified_visible = { fg = C.peach, bg = inactive_bg },
    modified_selected = { fg = C.peach, bg = active_bg },
  }
end

return {
  "akinsho/bufferline.nvim",
  event = "VeryLazy",
  dependencies = {
    "catppuccin/nvim",
  },
  keys = {
    { "<Tab>", "<Cmd>BufferLineCycleNext<CR>", desc = "Next tab" },
    { "<S-Tab>", "<Cmd>BufferLineCyclePrev<CR>", desc = "Prev tab" },
  },
  opts = {
    options = {
      themable = true,
      always_show_bufferline = true,
      -- show_buffer_close_icons = true,
      show_close_icon = false,
      -- numbers = function(opts)
      --   return string.format("%s", opts.raise(opts.ordinal))
      -- end,
      indicator = {
        style = "underline",
      },
      -- This does not work due to a bug, will wait or fix this later
      separator_style = { "", "" },
      -- separator_style = "slant",
      offsets = {
        {
          filetype = "neo-tree",
          text_align = "left",
          text = "Explorer",
        },
      },
    },
    highlights = getHighlights(),
  },
}

Additional Information

...

commit

Checked the main branch

Finii commented 1 month ago

What I see from the behavior, is that separator_style = {'A', 'B'} is not like slant, at least not what I would have expected.

slant uses two different glyphs, one always left and one always right of the tab text. When they would be A and B that would result in

A tab1 BA tab2 BA tab3 B

But in contrast the code actually generates 'thin' separators, means only one character between the tabs like

X tab1 X tab2 X tab3 X

where X is separator_style[1] when the tab is focused and separator_style[2] unfocused (or was it the other way around?).

This can be for example used to have the focused tab with an X while the other have a O like

O tab1 X tab2 O tab3 O tab4

I'm not sure this is what had been intended, and that can be changed by the following diff - meaning that the two custom characters are used one left and one right.

diff --git a/lua/bufferline/ui.lua b/lua/bufferline/ui.lua
index 440b203..8424c8f 100644
--- a/lua/bufferline/ui.lua
+++ b/lua/bufferline/ui.lua
@@ -251,7 +251,7 @@ end
 --- @param focused boolean
 --- @param style table | string
 local function get_separator(focused, style)
-  if type(style) == "table" then return focused and style[1] or style[2] end
+  if type(style) == "table" then return style[1], style[2] end
   ---@diagnostic disable-next-line: undefined-field
   local chars = sep_chars[style] or sep_chars.thin
   if is_slant(style) then return chars[1], chars[2] end

Regarding the 'wrong color', at least I see something that can count as 'wrong color'. The color can be controlled by indicator_visible resp indicator_selected, and it is displayed even though I have indicators disabled (I though) via indicator = { style = 'none'},.

maulik13 commented 1 month ago

@Finii thank you for taking time to reply with details. I know that the author of the plugin does not intend to provide full flexibility with custom separators, so I am not expecting this to be sorted. But I would be happy to open a PR if the author is willing. Otherwise, when I get a chance I will fork and update the code fix this issue regardless.

Finii commented 1 month ago

Now I also read the previous Issue you linked. This triggered my curiosity.

Well, what are these custom separator_style things anyhow, lets try:

image

Ah, with separator_style = { a, b } with a and b being strings.

I can hardly imagine a useful setting for a and b here. But anyhow, the issue(s) is about the coloring. Lets check.

First with normal standard separators, where I adjusted the colors for easy identification:

image

and with custom separators, they all turn red:

image

Ok, this does not look good and seems like a bug.

But what was the intention for the custom separator style. There is a comment in the get_separator() function, and it seems like this is the expected use-case (all colors default):

image

And now all the behavior makes sense.

Well, at least for the active and the invisible tabs. The visible-but-not-active (number 4 in the image), this still looks strange.

So I must admit, this is not a bug but the behavior is needed to support this one specific use-case that seemed to have driven the development of the custom separators. It's just not what I (and you and other) would have expected this feature to be for. :-D


Edit:

The code is really really old, just dove into git blame. It seems that people wanted to adjust how wide the left and right vertical bar is, some wanted that smaller, some bigger, and this allowed to have any combination/selection of thick or thin or middle or whatever you like, from the box drawing glyphs.

image

Example archeological blame, see for example 1a1545b and cab12e4

maulik13 commented 3 weeks ago

@Finii that was a great analysis and you concluded very similar to what I had in mind. Technically things are working as intended if we go by the code and its comments. But based on how configuration is provided to a user, results are unexpected. I suppose this can be considered a limitation as intended by the author.