feat(fzf): allow fzf win_configs to contain functions, fix vertical offsets

[willothy]

This PR fixes the issue with fzf placement I mentioned in #123.

[Bekaboo]

Thanks! It is not clear to me what is the exact bug and how to reproduce it.

---

EDIT: In addition could you explain how it works and what problem it addresses? I tried to understand but the logic seems a bit twisted.

---

EDIT 2: Also observed that the offset of fzf window is wrong when border is 'single' (not introduced by this PR):

Could we fix it in this PR too?

[willothy]

Thanks! It is not clear to me what is the exact bug and how to reproduce it.

EDIT: In addition could you explain how it works and what problem it addresses? I tried to understand but the logic seems a bit twisted.

Essentially, the fzf menu uses relative=win, so with anchor the position can be off by a few cells depending on the border and can end up overlapping the menu (covering entries).

This fixes the vertical overlap and ensures the fzf menu is always just a small bar at the bottom.

EDIT 2: Also observed that the offset of fzf window is wrong when border is 'single' (not introduced by this PR):

Could we fix it in this PR too?

Sure! That's exactly the issue I was trying to fix (some other cases of it as well). It was overlapping vertically on some occasions, that's what I've fixed so far. I'll look into the horizontal position now.

[willothy]

I'll try to provide a video or something later but honestly this one is hard to explain lol.

It happens with certain scroll positions, when the menu opens above the cursor only.

[Bekaboo]

@willothy The misalignment should be fixed now, can you confirm?

[willothy]

@willothy The misalignment should be fixed now, can you confirm?

Partially, but there are still the header cases to handle. I'll update this PR.

[Bekaboo]

Partially, but there are still the header cases to handle

Why the header will affect the alignment?

[willothy]

It affects vertical alignment. There can be a gap above the fzf window, or an empty row if there's a header (like in the ui-select menu, which uses only a top border).

(the header is covering the second available code action)

Edit: Can confirm horizontal alignment is fixed now, nice!

[Bekaboo]

@willothy Do you mean the header of the fzf window?

[willothy]

Yeah, it can inherit a header from the parent window. I will update this PR to only fix that and remove the things you fixed.

[willothy]

Should be good now

[Bekaboo]

@willothy I cannot reproduce the bug where the menu is overlapped with the title/header of the fzf window using the script below, with vim.ui.select() being replaced by dropbar's select menu:

vim.ui.select({ "tabs", "spaces" }, {
    prompt = "Select tabs or spaces:",
    format_item = function(item)
        return "I'd like to choose " .. item
    end,
}, function() end)

However, I can observe that the fzf window has an improperly thick top border because it inherits the border from the select menu (border = { "", " ", "", "", "", "", "", "" }:

Can we just merge opts.win_configs and fzf-window-specific window configs in fuzzy_find_open(), excluding self._win_configs? That should be a simpler solution. Notice that we have to re-evaluate the merged result using eval_win_configs().

[willothy]

@willothy I cannot reproduce the bug where the menu is overlapped with the title/header of the fzf window using the script below, with vim.ui.select() being replaced by dropbar's select menu:

It can occur both ways, either with overlap or the thick border, depending on factors I have not yet determined. I will see if your new commits have fixed it on my end.

[willothy]

Ok I now don't have the overlap issue, but I do have the thick border issue.

[willothy]

Can we just merge opts.win_configs and fzf-window-specific window configs in fuzzy_find_open(), excluding self._win_configs? That should be a simpler solution. Notice that we have to re-evaluate the merged result using eval_win_configs().

I don't think so, because then it would break some configs that use borders. I think the complex checks are kinda needed here tbh. Not 100% sure though, I'm testing the idea right now. Have just not found a solution that works for all edge cases.

[Bekaboo]

I don't think so, because then it would break some configs that use borders

Why does it break configs for borders given that opts.win_configs contains options for borders?

[willothy]

I don't think so, because then it would break some configs that use borders

Why does it break configs for borders given that opts.win_configs contains options for borders?

Because those can be fzf-specific I think. Honestly I'm not sure. But without self._win_configs.border the border doesn't inherit properly. I am looking into it to provide a more specific repro, sorry for not explaining well lol

[Bekaboo]

Emm I think the border of the menu is calculated from opts.menu.win_configs right? So it may be sufficient to use the option value to determine the fzf window's border.

---

To make it clearer:

• opts.menu.win_configs contains border configuration that the user WANTs it to be
• self._win_configs is the EVALUATED window configs (function to actual win config value) with MODIFICATIONS to make sure a ui-select header will be displayed when provided, so it does not always reflect users' intention.

For example, we will change the original border config to add a thick upper border if a header is provided to the ui-select function but opts.menu.win_configs.border=='none', in this case merging self._win_configs will give the fzf-window an undesirable thick upper border. So the proper way is to ignore self._win_configs and just merge opts.menu.win_configs with some fzf-specific settings.

[willothy]

Sounds good, I will keep trying different configurations of this.

[willothy]

I am struggling to implement something that works with all three of (1) full borders provided by name, (2) full borders provided by array and (3) partial borders provided by array.

I can get either partial borders or full borders to work but not both lol

[Bekaboo]

@willothy You can leave it to me if you find it hard to implement. I will give it a closer look later.

[willothy]

I think I'll be able to do it (feel free to have a look though),I am just not sure how the fzf window can inherit the parent window's border without having the thick header, unless we manually remove the header or add a hack just for the ui-select implementation which I don't think is ideal.

Using self.opts.win_configs has the same issue because the header is defined in self.opts.win_configs.

[willothy]

This seems to work well, it is a mix of your idea and removing the top border when possible.

[willothy]

Actually, maybe we should leave the border alone since the border for the fzf window is already configurable separately from the menu, so if the users would like to get rid of the thicker header they can. We can also update the defaults so that the fzf menu does not have a thick header by default. Thoughts?

[Bekaboo]

@willothy Sorry my fault, I thought that opts.win_configs is opts.menu.win_configs but turns out it is opts.fzf.win_configs. Why is this config field empty by default? It should be like opts.menu.win_config, having all necessary fields. We should do all the calculations there and user can easily tweak it to their liking as well in this way.

---

I am also thinking about allowing win_configs to have type func(menu) -> table so that we don't need to use a separate function for each dynamic field.

[willothy]

No worries!

I think it is nice to have them dynamic though, so maybe nice to keep updating in the function - I have an idea that involves changing the border at runtime, I will take a video of it and push the required change.

[Bekaboo]

I have an idea that involves changing the border at runtime

What is the use case for this?

[willothy]

It's kinda useless so no worries if you don't want to add it, but it looks super nice. Here:

Basically the border is totally seamless between the menu and fzf window.

This actually only requires the menu's border to be dynamic though, my bad

https://github.com/Bekaboo/dropbar.nvim/assets/38540736/c0fcd991-edf0-4829-a68c-05702b1677f6

[willothy]

Moved as much config as could be moved into the defaults.

[Bekaboo]

It's kinda useless so no worries if you don't want to add it, but it looks super nice. Here: 2024-01-24.20-13-16.mp4

This looks cool and is achievable if we allow win_configs to be a function. One can change the border of the menu window using nvim_open_win() and register a autocmd to restore the original border on WinClosed

[willothy]

The change in this PR now allows win_configs to be a function (it already did since we're using opts.win_configs now), and also recomputes a menu's border when the fzf window is opened/closed.

[Bekaboo]

No, I mean allowing win_configs itself to be a function, not a table containing functions. It will be more convenient when you want to change a bunch of window config fields in different situations.

[Bekaboo]

I will see if I can move more window configs to the opts table and eliminate the top border later.

[willothy]

No, I mean allowing win_configs itself to be a function, not a table containing functions. It will be more convenient when you want to change a bunch of window config fields in different situations.

Ohhh interesting! I like that idea.

[willothy]

I will see if I can move more window configs to the opts table and eliminate the top border later.

I actually don't think we should change the border manually as it could introduce unexpected behavior. I have a different idea: I think we should allow each menu to have a win_configs field for its fzf window, so that fzf windows can be configured per-menu as well as globally. That solves the ui-select issue without doing anything hacky that only affects that part of the plugin.

Edit: win_configs as function should be working, fzf options is WIP.

[Bekaboo]

I will see if I can move more window configs to the opts table and eliminate the top border later.

I actually don't think we should change the border manually as it could introduce unexpected behavior. I have a different idea: I think we should allow each menu to have a win_configs field for its fzf window, so that fzf windows can be configured per-menu as well as globally. That solves the ui-select issue without doing anything hacky that only affects that part of the plugin.

Sounds good!

[Bekaboo]

win_configs as function should be working, fzf options is WIP.

Thanks! I will be a bit busy in the following two or three days, I will be back and pick these changes as soon as possible and sorry for the latency in advance.

[willothy]

No worries at all! No need to apologize.

Anyways, it will give me a few days to test and fix any issues haha

[willothy]

Hmm actually after thinking about it I don't think the full win_config should be a function because then the different configs can't be merged until after they're executed, meaning initialization will be tricky.

It also means that if a user wants to change one part of the default win_config, they will have to reimplement the whole thing themselves.

[Bekaboo]

@willothy Is this ready to merge?

[willothy]

I think so, yeah. I've been using my fork with it for a week or so

Edit: nevermind, do not merge - found a bug lol

[willothy]

Might be worth making sure it works for you too tho if you haven't tested the latest version :)

[willothy]

Agh it behaves differently in the ui-select menu vs the winbar dropdowns. I'll try to finish this up tomorrow.

[willothy]

Ok I've fixed the issues I found, I think this is ready for merge. There are still some edge cases, but none that weren't present before afaik. Just things like different borders between fzf and the menu:

• If menu has a border and the fzf window has no border, the fzf window is not wide enough but it is aligned properly.
• If the fzf window has a border but the menu doesn't, then the fzf window is too wide, but again aligned properly.

imo those are obscure use cases and can be fixed in followups.

But if you want me to work out the width things in this PR I can.

[willothy]

Nvm, fixing the widths was pretty trivial so everything seems to be working now.

It does require potentially evaluating the fzf window border twice, but I don't think that's a huge deal.

[Bekaboo]

@willothy I have the following error on fix-fzf-overlap branch when I press i in the ui-select menu:

E5108: Error executing lua: /home/zeng/Code/dropbar.nvim/lua/dropbar/configs.lua:374: attempt to perfor
m arithmetic on local 'left' (a boolean value)
stack traceback:
        /home/zeng/Code/dropbar.nvim/lua/dropbar/configs.lua:374: in function 'border_width'
        /home/zeng/Code/dropbar.nvim/lua/dropbar/configs.lua:377: in function 'v'
        /home/zeng/Code/dropbar.nvim/lua/dropbar/menu.lua:910: in function 'merge_win_configs'
        /home/zeng/Code/dropbar.nvim/lua/dropbar/menu.lua:956: in function 'fuzzy_find_open'
        /home/zeng/Code/dropbar.nvim/lua/dropbar/configs.lua:278: in function </home/zeng/Code/dropbar.
nvim/lua/dropbar/configs.lua:273>

Code snippet used to open the ui-select menu (same as the example in :h ui.select):

vim.ui.select({ "tabs", "spaces" }, {
    prompt = "Select tabs or spaces:",
    format_item = function(item)
        return "I'd like to choose " .. item
    end,
}, function(choice)
    if choice == "spaces" then
        vim.o.expandtab = true
    else
        vim.o.expandtab = false
    end
end)

config:

require('dropbar').setup({
  menu = {
    scrollbar = { background = false },
    win_configs = {
      border = { '', '', '', '' },
    }
  },
})
vim.ui.select = require('dropbar.utils.menu').select

[willothy]

Hmm interesting, let me check that out.

Edit: too many ternaries haha I will fix that now.

[willothy]

Alright, I think it should work now

[Bekaboo]

@willothy fzf window sticking out

[willothy]

Ahhhh ok just a sec. I think that's specific to ui-select.

[willothy]

Ah figured it out. I was confused about how nvim handled 4-char borders.