popupmenu: Increase cmdheight when winborder is set #210
Conversation
If winborder option is set in neovim then cmdborder needs to be increased for the window not to overlap with cmdline. Fix this in s:get_position().
There was a problem hiding this comment.
Pull request overview
This PR attempts to address an issue where the popup menu overlaps with the command line when a window border is set in Neovim. The fix adds logic to increase the cmdheight calculation by 2 when the winborder option is detected and set to a non-empty, non-'none' value.
Changes:
- Modified
s:get_position()function to check forwinborderoption existence - Added conditional logic to increase
cmdheightby 2 when borders are enabled
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| let l:cmdheight = wilder#renderer#get_cmdheight() | ||
| if exists('&winborder') | ||
| if &winborder !=# '' && &winborder !=# 'none' |
There was a problem hiding this comment.
The magic number 2 for the cmdheight adjustment lacks documentation or justification. It's unclear why the cmdheight should be increased by exactly 2 when a border is present. This value should either be:
- Documented with a comment explaining why 2 is the correct value
- Made configurable if different border styles require different adjustments
- Calculated based on actual border dimensions
Additionally, this assumes all borders add the same amount of space, which may not be true for different border configurations.
| if &winborder !=# '' && &winborder !=# 'none' | |
| if &winborder !=# '' && &winborder !=# 'none' | |
| " Account for one line above and one line below the popup when a window | |
| " border is present. This assumes standard &winborder styles that add | |
| " exactly one line on the top and bottom. |
|
|
||
| let l:cmdheight = wilder#renderer#get_cmdheight() | ||
| if exists('&winborder') | ||
| if &winborder !=# '' && &winborder !=# 'none' |
There was a problem hiding this comment.
The condition checks if &winborder is not empty and not 'none', but doesn't validate what valid values for this option might be. If the option does exist (which needs verification), the check should validate against the actual valid values for the option rather than just checking if it's non-empty and not 'none'. This could lead to incorrect behavior if the option is set to an invalid or unexpected value.
| if exists('&winborder') | ||
| if &winborder !=# '' && &winborder !=# 'none' | ||
| let l:cmdheight += 2 | ||
| endif | ||
| endif |
There was a problem hiding this comment.
The option 'winborder' does not exist in standard Neovim. As of Neovim 0.10, there is no global &winborder option. Floating windows can have borders configured via the border parameter in nvim_open_win(), but this is not a global option that can be checked with exists('&winborder').
If this is intended to check for a future Neovim option or a custom option, please verify that such an option actually exists. Otherwise, this code will never execute as exists('&winborder') will always return false.
| if exists('&winborder') | |
| if &winborder !=# '' && &winborder !=# 'none' | |
| let l:cmdheight += 2 | |
| endif | |
| endif |
1 participant
If winborder option is set in neovim then cmdborder needs to be increased for the window not to overlap with cmdline. Fix this in s:get_position().