fix popup size calculation

Co-authored-by: ath3 <[email protected]>

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);
 }
 

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);
         };

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(..);

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<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;
 

helix-term/src/ui/popup.rs

@@ -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,
@@ -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,
@@ -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
@@ -118,38 +120,32 @@ 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
-
+    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<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");
+
+        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<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,
         }
     }
 
@@ -259,35 +294,16 @@ 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 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<T: Component> Component for Popup<T> {
         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");