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

willothy · 2024-01-23T11:06:26Z

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

Bekaboo · 2024-01-23T14:27:55Z

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 · 2024-01-23T15:13:29Z

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 · 2024-01-23T15:33:39Z

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 · 2024-01-23T17:14:41Z

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

willothy · 2024-01-23T17:26:40Z

@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 · 2024-01-23T17:28:22Z

Partially, but there are still the header cases to handle

Why the header will affect the alignment?

willothy · 2024-01-23T17:30:10Z

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 · 2024-01-23T17:39:57Z

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

willothy · 2024-01-23T17:43:13Z

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 · 2024-01-23T17:57:07Z

Should be good now

Bekaboo · 2024-01-24T08:06:10Z

@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 · 2024-01-24T16:26:02Z

@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 · 2024-01-24T16:26:52Z

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

willothy · 2024-01-24T16:55:42Z

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 · 2024-01-24T17:11:59Z

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 · 2024-01-24T17:21:59Z

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 · 2024-01-24T17:27:11Z

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 · 2024-01-24T18:04:35Z

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

willothy · 2024-01-25T01:26:36Z

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 · 2024-01-25T03:24:13Z

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

willothy · 2024-01-25T03:30:46Z

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 · 2024-01-25T03:42:36Z

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

willothy · 2024-01-25T04:01:56Z

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 · 2024-02-03T07:49:46Z

@willothy Is this ready to merge?

willothy · 2024-02-03T08:05:31Z

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 · 2024-02-03T08:06:00Z

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

willothy · 2024-02-03T08:09:03Z

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

fix(fzf): vertical offset for border refactor(fzf): move `win_config` elements into default config

willothy · 2024-02-05T03:12:36Z

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 · 2024-02-05T03:29:29Z

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 · 2024-02-05T05:38:43Z

@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 · 2024-02-05T05:39:33Z

Hmm interesting, let me check that out.

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

willothy · 2024-02-05T05:46:20Z

Alright, I think it should work now

Bekaboo · 2024-02-05T05:49:01Z

@willothy fzf window sticking out

willothy · 2024-02-05T05:50:39Z

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

willothy · 2024-02-05T05:58:13Z

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

willothy · 2024-02-05T06:00:24Z

Sorry for the confusion, just lots of edge cases here haha. I think it should work now, hopfully I'm not wrong again lol

Bekaboo · 2024-02-05T06:09:58Z

Sorry for the confusion, just lots of edge cases here haha. I think it should work now, hopfully I'm not wrong again lol

Yeah it's annoying. I think we could simplify the logic by expanding table-valued border whose length is less than 8 to the equivalent border table with 8 elements first then change the borders in different conditions. It can be a utils function in utils/menu.lua.

But let's merge this first.

willothy · 2024-02-05T06:20:26Z

Yeah, that would definitely make things simpler, as would utilities so we're not repeating any of this border code anywhere.

Bekaboo mentioned this pull request Jan 23, 2024

feat(ui-select): support optional preview of items #124

Merged

Bekaboo pushed a commit that referenced this pull request Jan 23, 2024

fix(menu): make sure fzf-window aligns with menu window (#131)

a296281

github-actions bot mentioned this pull request Jan 23, 2024

chore(master): release 8.1.0 #130

Merged

Bekaboo pushed a commit that referenced this pull request Jan 23, 2024

fix(menu): force fzf-window anchor to 'NW' to ensure alignment (#131)

589c1cf

willothy force-pushed the fix-fzf-overlap branch from d5b5c5a to e701600 Compare January 23, 2024 17:56

willothy force-pushed the fix-fzf-overlap branch 2 times, most recently from 6ad710d to 66b9661 Compare January 25, 2024 03:40

willothy force-pushed the fix-fzf-overlap branch from 101d73a to 38f222f Compare January 25, 2024 05:53

willothy marked this pull request as draft February 3, 2024 08:08

willothy added 3 commits February 4, 2024 18:56

feat(fzf): allow win_configs options to be functions

0e86254

fix(fzf): vertical offset for border refactor(fzf): move `win_config` elements into default config

fix(ui-select): modify fzf border depending on prompt

eb277ad

refactor(fzf): move fzf win configs to config functions

e21206b

willothy force-pushed the fix-fzf-overlap branch from 38f222f to e21206b Compare February 5, 2024 02:56

fix: row and col when menu border is 'none'

9eda2be

willothy marked this pull request as ready for review February 5, 2024 03:10

fix: differing fzf and menu widths

7cdcfd4

willothy added 2 commits February 4, 2024 21:43

fix: border width accumulation

e251123

fix: shadow border

76fcdbc

refactor: simplify condition

36457a9

fix: fzf offset for 4-char array border

3689774

Bekaboo merged commit ef73236 into Bekaboo:master Feb 5, 2024
4 checks passed

github-actions bot mentioned this pull request Feb 4, 2024

chore(master): release 8.2.0 #137

Merged

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

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

Conversation

willothy commented Jan 23, 2024

Bekaboo commented Jan 23, 2024 • edited

willothy commented Jan 23, 2024

willothy commented Jan 23, 2024

Bekaboo commented Jan 23, 2024

willothy commented Jan 23, 2024

Bekaboo commented Jan 23, 2024

willothy commented Jan 23, 2024 • edited

Bekaboo commented Jan 23, 2024

willothy commented Jan 23, 2024

willothy commented Jan 23, 2024

Bekaboo commented Jan 24, 2024

willothy commented Jan 24, 2024

willothy commented Jan 24, 2024

willothy commented Jan 24, 2024

Bekaboo commented Jan 24, 2024

willothy commented Jan 24, 2024

Bekaboo commented Jan 24, 2024 • edited

willothy commented Jan 24, 2024

willothy commented Jan 25, 2024

Bekaboo commented Jan 25, 2024

willothy commented Jan 25, 2024 • edited

willothy commented Jan 25, 2024

willothy commented Jan 25, 2024

Bekaboo commented Feb 3, 2024

willothy commented Feb 3, 2024 • edited

willothy commented Feb 3, 2024

willothy commented Feb 3, 2024

willothy commented Feb 5, 2024 • edited

willothy commented Feb 5, 2024

Bekaboo commented Feb 5, 2024

willothy commented Feb 5, 2024 • edited

willothy commented Feb 5, 2024

Bekaboo commented Feb 5, 2024

willothy commented Feb 5, 2024

willothy commented Feb 5, 2024

willothy commented Feb 5, 2024

Bekaboo commented Feb 5, 2024

willothy commented Feb 5, 2024

Bekaboo commented Jan 23, 2024 •

edited

willothy commented Jan 23, 2024 •

edited

Bekaboo commented Jan 24, 2024 •

edited

willothy commented Jan 25, 2024 •

edited

willothy commented Feb 3, 2024 •

edited

willothy commented Feb 5, 2024 •

edited

willothy commented Feb 5, 2024 •

edited