-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support drawing popup frame #4313
Conversation
Drawing borders for popups feel like a good default whereas having a config option for it feels a bit overkill. I suppose some might prefer a sleeker look without the border? |
I think an option for it seems ok. I prefer the borderless look but I've seen some discussions and matrix messages about wanting a border around popups |
helix-term/src/ui/lsp.rs
Outdated
let margin = Margin::horizontal(1); | ||
area.height -= 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the change here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it now, and added .clip_bottom(1) instead.
Before the change in this function, the text would be rendered on the bottom border line for signature help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clip_bottom
should probably be gated on the new config option as well - there's a noticeable blank line when the option is off
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the lack of bottom margin was unintentional since other popups have margins around them, and here we only miss it from the bottom.
I changed this now, so without borders if should look as it originally was.
e1285fd
to
00c55c5
Compare
I want this to be default. I use And I haven't seen any popup in nvim without borders. The same codeactionin nvim with lspsage.nvim plugin: |
This change covers popups (like signature-help) but not menus (completion, code-actions). I think an option to enable/disable borders on both would be good |
56ce875
to
73e707f
Compare
It covers now both menus and popups, with settings to enable one of them, both, or none. |
Updated after the changes to popups having scrollbars. |
be830c7
to
7e579ba
Compare
I decided to take out the changes related to selection popup whitespace since it has nothing to do with borders. |
I think this pr is ready for merge. |
helix-term/src/ui/menu.rs
Outdated
|
||
if render_borders { | ||
Widget::render(Block::default().borders(Borders::ALL), area, surface); | ||
area = area.clip_top(1).clip_bottom(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
area.inner doesn't work with Borders?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does, i changed it to use area.inner now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the mut from area in function parameter and instead create a new variable "inner" to make it look more consistent with popup render code.
If you prefer the previous version i can revert this change.
badbff3
to
2577f97
Compare
Nice job on the PR, has been a staple in my fork for like ever. |
I cleaned up the code, added popup-borders typable command, and fixed the issues with dap variable popup (thanks @SoraTenshi), and with menu only border setting. |
I think this PR is from my perspective in a good state and unless requested i dont plan to do any changes to it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience on this PR! This is something I'd like to use myself; we just only have so much time to review all the PRs.
helix-term/src/ui/menu.rs
Outdated
if let Some(cursor) = self.cursor { | ||
let offset_from_top = cursor - scroll; | ||
let left = &mut surface[(area.left(), area.y + offset_from_top as u16)]; | ||
left.set_style(selected); | ||
let right = &mut surface[( | ||
area.right().saturating_sub(1), | ||
area.y + offset_from_top as u16, | ||
)]; | ||
right.set_style(selected); | ||
let render_borders = cx.editor.menu_border; | ||
|
||
if !render_borders { | ||
if let Some(cursor) = self.cursor { | ||
let offset_from_top = cursor - scroll; | ||
let left = &mut surface[(area.left(), area.y + offset_from_top as u16)]; | ||
left.set_style(selected); | ||
let right = &mut surface[( | ||
area.right().saturating_sub(1), | ||
area.y + offset_from_top as u16, | ||
)]; | ||
right.set_style(selected); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, since I'm not super familiar with the rendering code, so I could be misunderstanding something, but this looks like if we want to draw borders around popups that we are deciding not to render the cursor? Why is that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code colors the first and last cell of the selected line in menu.
With borders turned on, the colors would run through the borders.
helix-term/src/ui/popup.rs
Outdated
let win_height = inner.height as usize - 2 * border; | ||
let len = self.child_size.1 as usize - 2 * border; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do a saturating_sub
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
helix-view/src/editor.rs
Outdated
pub popup_border: bool, | ||
pub menu_border: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could these just be helper methods instead of new fields on the Editor
object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, i changed them
helix-term/src/commands/typed.rs
Outdated
fn set_borders( | ||
cx: &mut compositor::Context, | ||
args: &[Cow<str>], | ||
event: PromptEvent, | ||
) -> anyhow::Result<()> { | ||
if event != PromptEvent::Validate { | ||
return Ok(()); | ||
} | ||
|
||
if args.is_empty() { | ||
let borders = (cx.editor.popup_border, cx.editor.menu_border); | ||
cx.editor.set_status(match borders { | ||
(true, true) => "all", | ||
(true, false) => "popup", | ||
(false, true) => "menu", | ||
(false, false) => "none", | ||
}); | ||
|
||
return Ok(()); | ||
} | ||
|
||
let arg = args.get(0).context("argument missing")?.to_lowercase(); | ||
|
||
if let Some(borders) = match arg { | ||
arg if arg.starts_with("all") => Some((true, true)), | ||
arg if arg.starts_with("popup") => Some((true, false)), | ||
arg if arg.starts_with("menu") => Some((false, true)), | ||
arg if arg.starts_with("none") => Some((false, false)), | ||
_ => None, | ||
} { | ||
cx.editor.popup_border = borders.0; | ||
cx.editor.menu_border = borders.1; | ||
} else { | ||
cx.editor | ||
.set_status("Valid options are: none, popup, menu, all"); | ||
}; | ||
|
||
Ok(()) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need its own command? Wouldn't just doing :set popup-border <choice>
work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, i deleted this command now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok this looks good to me! I tried it out locally too, and it looks good. Thanks for the PR.
Bump |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me but I'll leave it up to @archseer to merge
let inner = if self | ||
.contents | ||
.type_name() | ||
.starts_with("helix_term::ui::menu::Menu") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems very hacky to me but I don't see a good way around it - there isn't a way to compare the type to Menu<_>
without dealing with strings like this. I think I'm fine with this for now but it's something we should try to eliminate in the future (though it may take a large refactor to the compositor system to do it)
6e58c6c
Co-authored-by: Blaž Hrastnik <[email protected]>
Co-authored-by: Blaž Hrastnik <[email protected]>
Co-authored-by: Blaž Hrastnik <[email protected]>
Co-authored-by: Blaž Hrastnik <[email protected]>
Usability improvement:
popup-frame
.On some terminals / themes the popups are hard to differentiate from the background.
This makes (for example) LSP information hard to read, and makes the daily work using ssh sessions not as good as it could be.
With this change popups and menus can be configured have a border, and is easy to tell them apart from background with every theme and terminal.
Before:
After: