Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix bugs in popup #10573

Merged
merged 3 commits into from
Apr 28, 2024
Merged

Fix bugs in popup #10573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4ea2f20
fix popup size calculation
pascalkuthe Apr 23, 2024
9573b7d
fix required_size implementation of signature help
pascalkuthe Apr 23, 2024
be15619
move popup when cursor line changes
pascalkuthe Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 2 additions & 7 deletions helix-term/src/commands/dap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use dap::{StackFrame, Thread, ThreadStates};
use helix_core::syntax::{DebugArgumentValue, DebugConfigCompletion, DebugTemplate};
use helix_dap::{self as dap, Client};
use helix_lsp::block_on;
use helix_view::{editor::Breakpoint, graphics::Margin};
use helix_view::editor::Breakpoint;

use serde_json::{to_value, Value};
use tokio_stream::wrappers::UnboundedReceiverStream;
Expand Down Expand Up @@ -581,12 +581,7 @@ pub fn dap_variables(cx: &mut Context) {
}

let contents = Text::from(tui::text::Text::from(variables));
let margin = if cx.editor.popup_border() {
Margin::all(1)
} else {
Margin::none()
};
let popup = Popup::new("dap-variables", contents).margin(margin);
let popup = Popup::new("dap-variables", contents);
cx.replace_or_push_layer("dap-variables", popup);
}

Expand Down
11 changes: 1 addition & 10 deletions helix-term/src/commands/lsp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ use helix_stdx::path;
use helix_view::{
document::{DocumentInlayHints, DocumentInlayHintsId},
editor::Action,
graphics::Margin,
handlers::lsp::SignatureHelpInvoked,
theme::Style,
Document, View,
Expand Down Expand Up @@ -733,15 +732,7 @@ pub fn code_action(cx: &mut Context) {
});
picker.move_down(); // pre-select the first item

let margin = if editor.menu_border() {
Margin::vertical(1)
} else {
Margin::none()
};

let popup = Popup::new("code-action", picker)
.with_scrollbar(false)
.margin(margin);
let popup = Popup::new("code-action", picker).with_scrollbar(false);

compositor.replace_or_push("code-action", popup);
};
Expand Down
10 changes: 1 addition & 9 deletions helix-term/src/ui/completion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ use crate::{
use helix_view::{
document::SavePoint,
editor::CompleteAction,
graphics::Margin,
handlers::lsp::SignatureHelpInvoked,
theme::{Modifier, Style},
ViewId,
Expand Down Expand Up @@ -334,16 +333,9 @@ impl Completion {
}
});

let margin = if editor.menu_border() {
Margin::vertical(1)
} else {
Margin::none()
};

let popup = Popup::new(Self::ID, menu)
.with_scrollbar(false)
.ignore_escape_key(true)
.margin(margin);
.ignore_escape_key(true);

let (view, doc) = current_ref!(editor);
let text = doc.text().slice(..);
Expand Down
5 changes: 1 addition & 4 deletions helix-term/src/ui/lsp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,7 @@ impl Component for SignatureHelp {

let sig = &self.signatures[self.active_signature];

if PADDING >= viewport.1 || PADDING >= viewport.0 {
return None;
}
Comment on lines -158 to -160
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this the bug? Returning None instead of a default size?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this was an existing bug that caused crashes. required_size is only meant to always return None (for components that can't be embedded in a popup) or always meant to return Some. The API is a bit confusing and was very inconsistently used so this seems to have slipped trough at some point.

let max_text_width = (viewport.0 - PADDING).min(120);
let max_text_width = viewport.0.saturating_sub(PADDING).clamp(10, 120);

let signature_text = crate::ui::markdown::highlighted_code_block(
sig.signature.as_str(),
Expand Down
21 changes: 3 additions & 18 deletions helix-term/src/ui/menu.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,11 @@ use crate::{
use helix_core::fuzzy::MATCHER;
use nucleo::pattern::{Atom, AtomKind, CaseMatching};
use nucleo::{Config, Utf32Str};
use tui::{
buffer::Buffer as Surface,
widgets::{Block, Borders, Table, Widget},
};
use tui::{buffer::Buffer as Surface, widgets::Table};

pub use tui::widgets::{Cell, Row};

use helix_view::{
editor::SmartTabConfig,
graphics::{Margin, Rect},
Editor,
};
use helix_view::{editor::SmartTabConfig, graphics::Rect, Editor};
use tui::layout::Constraint;

pub trait Item: Sync + Send + 'static {
Expand Down Expand Up @@ -341,16 +334,8 @@ impl<T: Item + 'static> Component for Menu<T> {
.try_get("ui.menu")
.unwrap_or_else(|| theme.get("ui.text"));
let selected = theme.get("ui.menu.selected");
surface.clear_with(area, style);

let render_borders = cx.editor.menu_border();

let area = if render_borders {
Widget::render(Block::default().borders(Borders::ALL), area, surface);
area.inner(&Margin::vertical(1))
} else {
area
};
surface.clear_with(area, style);

let scroll = self.scroll;

Expand Down
182 changes: 101 additions & 81 deletions helix-term/src/ui/popup.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,23 @@ use helix_view::{
Editor,
};

const MIN_HEIGHT: u16 = 4;
const MIN_HEIGHT: u16 = 6;
const MAX_HEIGHT: u16 = 26;
const MAX_WIDTH: u16 = 120;

struct RenderInfo {
area: Rect,
child_height: u16,
render_borders: bool,
is_menu: bool,
}

// TODO: share logic with Menu, it's essentially Popup(render_fn), but render fn needs to return
// a width/height hint. maybe Popup(Box<Component>)

pub struct Popup<T: Component> {
contents: T,
position: Option<Position>,
margin: Margin,
area: Rect,
position_bias: Open,
scroll_half_pages: usize,
Expand All @@ -38,7 +46,6 @@ impl<T: Component> Popup<T> {
Self {
contents,
position: None,
margin: Margin::none(),
position_bias: Open::Below,
area: Rect::new(0, 0, 0, 0),
scroll_half_pages: 0,
Expand Down Expand Up @@ -71,11 +78,6 @@ impl<T: Component> Popup<T> {
self
}

pub fn margin(mut self, margin: Margin) -> Self {
self.margin = margin;
self
}

pub fn auto_close(mut self, auto_close: bool) -> Self {
self.auto_close = auto_close;
self
Expand Down Expand Up @@ -118,38 +120,38 @@ impl<T: Component> Popup<T> {
}

pub fn area(&mut self, viewport: Rect, editor: &Editor) -> Rect {
let child_size = self
.contents
.required_size((viewport.width, viewport.height))
.expect("Component needs required_size implemented in order to be embedded in a popup");

self.area_internal(viewport, editor, child_size)
self.render_info(viewport, editor).area
}

pub fn area_internal(
&mut self,
viewport: Rect,
editor: &Editor,
child_size: (u16, u16),
) -> Rect {
let width = child_size.0.min(viewport.width);
let height = child_size.1.min(viewport.height.saturating_sub(2)); // add some spacing in the viewport

let position = self
fn render_info(&mut self, viewport: Rect, editor: &Editor) -> RenderInfo {
let mut position = editor.cursor().0.unwrap_or_default();
if let Some(old_position) = self
.position
.get_or_insert_with(|| editor.cursor().0.unwrap_or_default());
.filter(|old_position| old_position.row == position.row)
{
position = old_position;
} else {
self.position = Some(position);
}

// if there's a orientation preference, use that
// if we're on the top part of the screen, do below
// if we're on the bottom part, do above
let is_menu = self
.contents
.type_name()
.starts_with("helix_term::ui::menu::Menu");

let mut render_borders = if is_menu {
editor.menu_border()
} else {
editor.popup_border()
};

// -- make sure frame doesn't stick out of bounds
let mut rel_x = position.col as u16;
let mut rel_y = position.row as u16;
if viewport.width <= rel_x + width {
rel_x = rel_x.saturating_sub((rel_x + width).saturating_sub(viewport.width));
}

// if there's a orientation preference, use that
// if we're on the top part of the screen, do below
// if we're on the bottom part, do above
let can_put_below = viewport.height > rel_y + MIN_HEIGHT;
let can_put_above = rel_y.checked_sub(MIN_HEIGHT).is_some();
let final_pos = match self.position_bias {
pascalkuthe marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -163,7 +165,40 @@ impl<T: Component> Popup<T> {
},
};

match final_pos {
// compute maximum space available for child
let mut max_height = match final_pos {
Open::Above => rel_y,
Open::Below => viewport.height.saturating_sub(1 + rel_y),
};
max_height = max_height.min(MAX_HEIGHT);
let mut max_width = viewport.width.saturating_sub(2).min(MAX_WIDTH);
render_borders = render_borders && max_height > 3 && max_width > 3;
if render_borders {
max_width -= 2;
max_height -= 2;
}

// compute required child size and reclamp
let (mut width, child_height) = self
.contents
.required_size((max_width, max_height))
.expect("Component needs required_size implemented in order to be embedded in a popup");

pascalkuthe marked this conversation as resolved.
Show resolved Hide resolved
width = width.min(MAX_WIDTH);
let height = if render_borders {
pascalkuthe marked this conversation as resolved.
Show resolved Hide resolved
(child_height + 2).min(MAX_HEIGHT)
} else {
child_height.min(MAX_HEIGHT)
};
if render_borders {
width += 2;
}
if viewport.width <= rel_x + width + 2 {
rel_x = viewport.width.saturating_sub(width + 2);
width = viewport.width.saturating_sub(rel_x + 2)
}

let area = match final_pos {
Open::Above => {
rel_y = rel_y.saturating_sub(height);
Rect::new(rel_x, rel_y, width, position.row as u16 - rel_y)
Expand All @@ -173,6 +208,12 @@ impl<T: Component> Popup<T> {
let y_max = viewport.bottom().min(height + rel_y);
Rect::new(rel_x, rel_y, width, y_max - rel_y)
}
};
RenderInfo {
area,
child_height,
render_borders,
is_menu,
}
}

Expand Down Expand Up @@ -259,69 +300,47 @@ impl<T: Component> Component for Popup<T> {
// tab/enter/ctrl-k or whatever will confirm the selection/ ctrl-n/ctrl-p for scroll.
}

fn required_size(&mut self, _viewport: (u16, u16)) -> Option<(u16, u16)> {
const MAX_WIDTH: u16 = 120;
const MAX_HEIGHT: u16 = 26;

let inner = Rect::new(0, 0, MAX_WIDTH, MAX_HEIGHT).inner(&self.margin);

let (width, height) = self
.contents
.required_size((inner.width, inner.height))
.expect("Component needs required_size implemented in order to be embedded in a popup");

let size = (
(width + self.margin.width()).min(MAX_WIDTH),
(height + self.margin.height()).min(MAX_HEIGHT),
);

Some(size)
}

fn render(&mut self, viewport: Rect, surface: &mut Surface, cx: &mut Context) {
let child_size = self
.contents
.required_size((viewport.width, viewport.height))
.expect("Component needs required_size implemented in order to be embedded in a popup");

let area = self.area_internal(viewport, cx.editor, child_size);
let RenderInfo {
area,
child_height,
render_borders,
is_menu,
} = self.render_info(viewport, cx.editor);
self.area = area;

let max_offset = child_size.1.saturating_sub(area.height) as usize;
let half_page_size = (area.height / 2) as usize;
let scroll = max_offset.min(self.scroll_half_pages * half_page_size);
if half_page_size > 0 {
self.scroll_half_pages = scroll / half_page_size;
}
cx.scroll = Some(scroll);

// clear area
let background = cx.editor.theme.get("ui.popup");
surface.clear_with(area, background);

let render_borders = cx.editor.popup_border();

let inner = if self
.contents
.type_name()
.starts_with("helix_term::ui::menu::Menu")
{
area
let background = if is_menu {
// TODO: consistently style menu
cx.editor
.theme
.try_get("ui.menu")
.unwrap_or_else(|| cx.editor.theme.get("ui.text"))
} else {
area.inner(&self.margin)
cx.editor.theme.get("ui.popup")
};
surface.clear_with(area, background);

let border = usize::from(render_borders);
let mut inner = area;
if render_borders {
inner = area.inner(&Margin::all(1));
pascalkuthe marked this conversation as resolved.
Show resolved Hide resolved
Widget::render(Block::default().borders(Borders::ALL), area, surface);
}
let border = usize::from(render_borders);

let max_offset = child_height.saturating_sub(inner.height) as usize;
let half_page_size = (inner.height / 2) as usize;
let scroll = max_offset.min(self.scroll_half_pages * half_page_size);
if half_page_size > 0 {
self.scroll_half_pages = scroll / half_page_size;
}
cx.scroll = Some(scroll);
self.contents.render(inner, surface, cx);

// render scrollbar if contents do not fit
if self.has_scrollbar {
let win_height = (inner.height as usize).saturating_sub(2 * border);
let len = (child_size.1 as usize).saturating_sub(2 * border);
let win_height = inner.height as usize;
let len = child_height as usize;
let fits = len <= win_height;
let scroll_style = cx.editor.theme.get("ui.menu.scroll");

Expand All @@ -336,7 +355,8 @@ impl<T: Component> Component for Popup<T> {

let mut cell;
for i in 0..win_height {
cell = &mut surface[(inner.right() - 1, inner.top() + (border + i) as u16)];
cell =
&mut surface[(inner.right() - 1 + border as u16, inner.top() + i as u16)];

let half_block = if render_borders { "▌" } else { "▐" };

Expand Down
Loading