VonHeikemen / searchbox.nvim

Start your search from a more comfortable place, say the upper right corner?
MIT License
336 stars 6 forks source link

popup position error when using confirm=menu in SearchBoxReplace #19

Closed SrWither closed 2 years ago

SrWither commented 2 years ago

as it says in the title, the popup is not positioned correctly when replacing words image image image

VonHeikemen commented 2 years ago

That's weird...

Hopefully I fixed it now in e576743.

SrWither commented 2 years ago

mmm, the error continues, but in a different way

https://user-images.githubusercontent.com/59105868/151888585-880c8f80-5bf4-4681-b7de-0731692aa91a.mp4

VonHeikemen commented 2 years ago

Can't seem to reproduce this one.

Does it happen if you replace sum with something different like zz?

SrWither commented 2 years ago

Does it happen if you replace sum with something different like zz?

Yes

VonHeikemen commented 2 years ago

The cursor is moving to the right places, nui.nvim's popup should follow. It doesn't make sense unless the function that sets the cursor position is non-blocking, and somehow is not fast enough.

I see you have a fork of this plugin. I assume you know a little bit about lua. Can you put a little delay in the confirm_action function? Replace this block.

https://github.com/VonHeikemen/searchbox.nvim/blob/e576743d422c006ecd494f8901d86590d2906a81/lua/searchbox/search-types.lua#L422-L429

With this:

    vim.defer_fn(function()
      menu.confirm_action({
        on_close = function()
          clear_matches(state)
        end,
        on_submit = function(item)
          fn.execute(item, pos)
        end
      })
    end, 5)
SrWither commented 2 years ago

Now it works fine

Akianonymus commented 2 years ago

With this:

    vim.defer_fn(function()
      menu.confirm_action({
        on_close = function()
          clear_matches(state)
        end,
        on_submit = function(item)
          fn.execute(item, pos)
        end
      })
    end, 5)

This works. Why not merged ?

Also, no need of 5 ms, just 0 ms is fine.

VonHeikemen commented 2 years ago

Is merged now.

I managed to reproduce this bug with a "clean" config, which makes this whole situation even more strange. The function cursor_pos should block menu.confirm_action but is not doing that... sounds like bug in neovm but I can't be sure.

Also, no need of 5 ms, just 0 ms is fine.

0 didn't work for me, so 5 it is.

Akianonymus commented 2 years ago

There is still a lot of edgecases left. I haven't studied the code but i think it's mostly creating these edgecases because popup is not created relatively.

For example, imagine the file is at this position: Screenshot_20220210-085659

After submitting, it first tries to scroll the buffer if there isn't ample space available above the match ( i don't know why it's trying to do that, obviously ), in this case it isn't, so it did. As the buffer is scrolled, the matched word postion is also changed, but popup is still created below the line number where the word was matched.

Screenshot_20220210-085709

Match was found at 6th line, popup created at 7th line.

VonHeikemen commented 2 years ago

Well, I guess this is another case of "why neovim doesn't wait until previous function finish?"

I would very much like to know which global setting is making this work on my config. I don't have this bug with my normal neovim setup.


It looks like set wrap is the one causing trouble. I have vim.opt.wrap = false, but if I set it to true everything goes to hell.

VonHeikemen commented 2 years ago

Would you mind testing the branch fix-menu to see if that solves the issue?

Akianonymus commented 2 years ago

Yes, it does solves the issue. Awesome.

Btw, found another bug.

Open replace dialog Type a single character ( except numbers ). Enter anything in "With" menu Then try pressing "n" to discard. It won't work.

VonHeikemen commented 2 years ago

I pushed another change in fix-menu, that should fix it.

Akianonymus commented 2 years ago

Fixed. 👌

VonHeikemen commented 2 years ago

OK. Commits have been merge into main now.