From 2d6d876a233ffc854bd3eb9d07c12948b0408e4b Mon Sep 17 00:00:00 2001 From: Pascal Kuthe Date: Tue, 23 Apr 2024 16:15:42 +0200 Subject: [PATCH] fix popup size calculation Co-authored-by: ath3 --- helix-term/src/commands/dap.rs | 9 +- helix-term/src/commands/lsp.rs | 11 +-- helix-term/src/ui/completion.rs | 10 +-- helix-term/src/ui/menu.rs | 21 +---- helix-term/src/ui/popup.rs | 154 +++++++++++++++++--------------- 5 files changed, 91 insertions(+), 114 deletions(-) diff --git a/helix-term/src/commands/dap.rs b/helix-term/src/commands/dap.rs index d62b0a4e5..eda94c44f 100644 --- a/helix-term/src/commands/dap.rs +++ b/helix-term/src/commands/dap.rs @@ -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; @@ -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); } diff --git a/helix-term/src/commands/lsp.rs b/helix-term/src/commands/lsp.rs index 88bd07919..e7ba9f0fc 100644 --- a/helix-term/src/commands/lsp.rs +++ b/helix-term/src/commands/lsp.rs @@ -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, @@ -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); }; diff --git a/helix-term/src/ui/completion.rs b/helix-term/src/ui/completion.rs index 7a08c90ce..f793dd484 100644 --- a/helix-term/src/ui/completion.rs +++ b/helix-term/src/ui/completion.rs @@ -5,7 +5,6 @@ use crate::{ use helix_view::{ document::SavePoint, editor::CompleteAction, - graphics::Margin, handlers::lsp::SignatureHelpInvoked, theme::{Modifier, Style}, ViewId, @@ -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(..); diff --git a/helix-term/src/ui/menu.rs b/helix-term/src/ui/menu.rs index f9f038e7a..c5006f958 100644 --- a/helix-term/src/ui/menu.rs +++ b/helix-term/src/ui/menu.rs @@ -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 { @@ -341,16 +334,8 @@ impl Component for Menu { .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; diff --git a/helix-term/src/ui/popup.rs b/helix-term/src/ui/popup.rs index 8d436fda9..7ee65db59 100644 --- a/helix-term/src/ui/popup.rs +++ b/helix-term/src/ui/popup.rs @@ -15,7 +15,16 @@ 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) @@ -23,7 +32,6 @@ const MIN_HEIGHT: u16 = 4; pub struct Popup { contents: T, position: Option, - margin: Margin, area: Rect, position_bias: Open, scroll_half_pages: usize, @@ -38,7 +46,6 @@ impl Popup { Self { contents, position: None, - margin: Margin::none(), position_bias: Open::Below, area: Rect::new(0, 0, 0, 0), scroll_half_pages: 0, @@ -71,11 +78,6 @@ impl Popup { 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 @@ -118,38 +120,32 @@ impl Popup { } 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 - + fn render_info(&mut self, viewport: Rect, editor: &Editor) -> RenderInfo { let position = self .position .get_or_insert_with(|| editor.cursor().0.unwrap_or_default()); - // 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 { @@ -163,7 +159,40 @@ impl Popup { }, }; - 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"); + + width = width.min(MAX_WIDTH); + let height = if render_borders { + (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) @@ -173,6 +202,12 @@ impl Popup { 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, } } @@ -259,35 +294,16 @@ impl Component for Popup { // 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 max_offset = child_height.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 { @@ -296,32 +312,30 @@ impl Component for Popup { 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)); Widget::render(Block::default().borders(Borders::ALL), area, surface); } + let border = usize::from(render_borders); 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 len = (child_height as usize).saturating_sub(2 * border); let fits = len <= win_height; let scroll_style = cx.editor.theme.get("ui.menu.scroll");