fix(border): Failing to display if row negative

[filipdutescu]

Fixes GH-239

Proposed changes

• Modify the if statement checking that the row is positive and non-zero for a Border, by allowing negative values for cursor-relative row numbers, if they are not outside of bounds
• Add an else branch to remove top border thickness if the topline should not be drawn

Motivation behind changes

As described in GH-239, this commit aims to fix an issue with the Border component, which makes it impossible to draw the top and bottom lines and makes the middle line replace the former, if the row is a negative value. This also makes it impossible to use the planery.popup cursor-relative positioning, as a value such as 'cursor-2' would break the popup.

Test plan

Tests from the tests and lua/luassert directories are used for validation, as I am not too familiar with the current code base. If others should be made, please let me know so as to add them.

[l-kershaw]

Thanks for writing this PR 👍

I agree that this resolves the issue in most cases, but it seems to introduce problems when the popup is right at the top of the screen (so the border would go off the edge).
My guess is that this is what the if statement was put in place to avoid.
Therefore I would suggest amending the if statement rather than removing it.

Maybe something like:
if content_win_options.row > 0 or content_win_options.relative == "cursor" then
would also fix the issue, without introducing new bugs?

[filipdutescu]

I see. Makes sense. I propose another solution, we could check the following:

content_win_options.row > 0 or (content_win_options.relative == 'cursor' and content_win_options.row + vim.api.nvim_win_get_cursor(0)[1] > 0)

I am not at my computer at the moment, so I might've used the wrong function, hopefully the intention is clear anyhow. I think this could also account for when the position is relative to the cursor and the border would be out of bounds once drawn. What do you think @l-kershaw ?

[l-kershaw]

@filipdutescu that seems like a good way to cover the edge cases to me 👍

[filipdutescu]

I've just tested on my setup (seems like there is also an issue with plenary.popup if you go beyond the boundary), but for the border, this looks like it covers at least the basic cases:

Note: Cursor is on line 2 in this image. Its position and the <cword> looks hidden due to the popup window being outside the boundaries, but the top border looks fine.

[l-kershaw]

Ah, I think we would also need to adjust the "border thickness" at the top when we don't draw the top border.
So maybe an else for this if statement that sets border_win_options.border_thickness.top=0?

[filipdutescu]

It semi-fixes the issue. Here is the current result, using the same test case:

There is also the following problem for when the cursor is on line 3:

[filipdutescu]

@l-kershaw do you have any idea why placement looks so weird? the values I have used for planery.popup are cursor-2 for the two screenshots above. I am not sure if my changes impacted the rendering, but I fail to see how they could've.

[filipdutescu]

It seems like the problem is only for popups with border that should show up above the cursor when there are less than 3 lines until the top of the window.

Note: Once the cursor is on any row with a value higher than 3, the popup, borders included, are drawn as expected.

[l-kershaw]

@filipdutescu I can't reproduce your issue with "cursor-2" on row 3. Could you do a screen record of how you can reproduce it?

Also, could you not keep force-pushing, it makes it harder to keep track of commits, and means that I need to keep approving the linting for you. We always squash PRs before merging into master, so it doesn't make much difference for the git log of having extra commits.

[filipdutescu]

Yes, sure, sorry for force pushing, was trying to fix the styling issues, which seems to have ended in yet another failure. If you don't mind random fix: style commits than it's easier for me as well haha.

I can do a screen record, but I am not sure how it will help without the underlying code. The code I am using to create the plugin is on a repo of mine. If you don't mind I can link it to you here, if that is OK with you of course. If not I could try and record the code as well?

[l-kershaw]

Don't worry about extra commits for linting, my PRs often have several commits trying to keep stylua happy 😅

It would be better if you could find a way to reproduce the issue using just the plenary code (probably easier if you use plenary.popup as Border is hard to test on its own). That way we know it is definitely something going wrong within plenary and not your own code elsewhere.

[filipdutescu]

Will do, no problem. But how would you like to proceed? Shall I make a different PR for that issue (with steps to reproduce) and leave this as an encapsulated fix or use this PR for that as well?

[filipdutescu]

It seems like you still need to approve my runs, even without force-pushing, since I am not a contributor, yet (hopefully soon 🤞).

[l-kershaw]

I think the issue is related enough to this one to be in the same PR, particularly as we may have caused it here 😅
If you give me steps to reproduce then I can see what I can figure out 🙂

---

Yeh, I thought that it was the force-pushing that caused it, but clearly I was wrong 😅

[filipdutescu]

Sure, just so I'm not appearing as MIA, I will experiment a bit, try to reproduce with only plenary and, if successful, will come back with a comment containing the steps to reproduce, if not, will let you know it's an issue only on my side.

---

Anyways, just wanted to let you know I appreciate your promptness and willingness to help me and to get this fix working. So for that, thank you kindly! 😁

[l-kershaw]

Sounds like a good plan to me 👍
Just ping me when you're done 🙂

---

No worries, very happy to help. Thanks for taking the initiative to write a PR 😃

[filipdutescu]

@l-kershaw I managed to reproduce the issue with the current PR changes and with the latest planery, from master.

Steps to reproduce

1. I used the following code, put in plenary/lua/plenary/tmp/init.lua (in my directory where I installed the Neovim plugins, including plenary):

local popup = require'plenary.popup'

local tmp = {}

function tmp.make_popup()
    local cword = vim.fn.expand '<cword>'
    local prompt_col_no = 1
    local prompt_line_no = 2
    local title = 'Test'

    local popup_opts = {
        title = title,
        padding = { 0, 0, 0, 0 },
        border = true,
        borderchars = { '─', '│', '─', '│', '╭', '╮', '╯', '╰' },
        width = #title + 4,
        line = 'cursor-' .. prompt_line_no,
        col = 'cursor-' .. prompt_col_no,
        cursor_line = true,
        enter = true,
        callback = function() print('Hello World!') end,
    }
    print(popup_opts.line, popup_opts.col)
    local prompt_win_id, prompt_opts = popup.create(cword, popup_opts)

    vim.cmd [[startinsert]]
    vim.api.nvim_win_set_option(prompt_win_id, 'wrap', false)
    vim.api.nvim_win_set_option(prompt_win_id, 'winhl', 'Normal:Normal')
    vim.api.nvim_win_set_option(prompt_win_id, 'winblend', 0)

    return prompt_win_id, {
        opts = popup_opts,
        border_opts = prompt_opts.border
    }
end

return tmp

2. From Neovim I used the following command to call my function, while having the code above opened and on line 3, on the tmp word: :lua require'plenary.tmp'.make_popup().

Results

Current PR changes

With the changes from the current PR the result was:

Current master

With the current master branch the result was:

[filipdutescu]

@l-kershaw also I would like to say that for a popup that appears on lines 1 and 2, it is fine to hide what's under the cursor and have a popup without border of 2 rows of height. It makes sense. So the border thickness thing you mentioned works wonderfully. The problem is only with the 3rd row, line 3. Personally, I would have it show the same as the previous 2, so no topline and 2 row height, starting from line 2.

Does this make sense?

Then again, this is not the previous behaviour, so maybe we should not handle it this way, but only make sure that the buffer text in put in the middle. Now that I think about it, this option seems better to me.

I think it is from plenary.popup, it does not seem to account for when the row and the top border row are equal. It should "push" the content 1 row down in this case.

[l-kershaw]

Ah, I think our nvim configs differ in a way which gives different results. I have set showtabline=2 which then leaves extra space for the border in some cases.

This is made me think that we would get some weird stuff with near the top of a lower horizontal split, which it seems we do (see picture)

I think the problem is that the calculation using nvim_win_get_cursor is the cursor position relative to the window, not relative to the editor. We should probably adjust to relative to the editor using nvim_win_get_position. Then we would only cut off the top of the border when at the top of the editor, not just the top of the window.

[filipdutescu]

I do not have any settings for showtabline, so if it is not the default, then you are correct. It stems from this difference.

I really like the suggestion of using nvim_win_get_position instead of nvim_win_get_cursor. I did not know of this behaviour for the latter.

Does this change only need to be implemented for the planery.popup component or are there others related to this PR?

[l-kershaw]

Switching the second condition to
or (content_win_options.relative == "cursor" and content_win_options.row + vim.api.nvim_win_get_cursor(0)[1] + vim.api.nvim_win_get_position(0)[1] > 1)
Seems to give reasonable behaviour in all the tests I can think of.

[l-kershaw]

Ah, I think you misunderstood my intention; nvim_win_get_position gives the position of the window (not the cursor) relative to the editor. You need to use both this and nvim_win_get_cursor to get the cursor position relative to the editor.

[filipdutescu]

Ah I see, now it makes a whole lot more sense haha. I will test it out tomorrow on my setup as well and let you know if everything is sound for me as well. Thanks!

[filipdutescu]

@l-kershaw I can confirm it fixes the issue for me as well:

I have also squashed my commits, in order to preserve the messages describing what this PR aims to fix and how. Hope that's OK with you, if not feel free to change the message.

[l-kershaw]

Thanks for all your hard work @filipdutescu!

[filipdutescu]

And thank you as well for your kind help and for accepting my changes!