Improve popup position

helix-term/src/ui/completion.rs

@@ -493,12 +493,7 @@ impl Component for Completion {
             None => return,
         };
 
-        let popup_area = {
-            let (popup_x, popup_y) = self.popup.get_rel_position(area, cx.editor);
-            let (popup_width, popup_height) = self.popup.get_size();
-            Rect::new(popup_x, popup_y, popup_width, popup_height)
-        };
-
+        let popup_area = self.popup.area(area, cx.editor);
         let doc_width_available = area.width.saturating_sub(popup_area.right());
         let doc_area = if doc_width_available > 30 {
             let mut doc_width = doc_width_available;

helix-term/src/ui/editor.rs

@@ -1034,7 +1034,6 @@ impl EditorView {
         self.last_insert.1.push(InsertEvent::TriggerCompletion);
 
         // TODO : propagate required size on resize to completion too
-        completion.required_size((size.width, size.height));
         self.completion = Some(completion);
         Some(area)
     }

helix-term/src/ui/mod.rs

@@ -13,7 +13,7 @@ mod spinner;
 mod statusline;
 mod text;
 
-use crate::compositor::{Component, Compositor};
+use crate::compositor::Compositor;
 use crate::filter_picker_entry;
 use crate::job::{self, Callback};
 pub use completion::{Completion, CompletionItem};
@@ -143,14 +143,12 @@ pub fn raw_regex_prompt(
                                         move |_editor: &mut Editor, compositor: &mut Compositor| {
                                             let contents = Text::new(format!("{}", err));
                                             let size = compositor.size();
-                                            let mut popup = Popup::new("invalid-regex", contents)
+                                            let popup = Popup::new("invalid-regex", contents)
                                                 .position(Some(helix_core::Position::new(
                                                     size.height as usize - 2, // 2 = statusline + commandline
                                                     0,
                                                 )))
                                                 .auto_close(true);
-                                            popup.required_size((size.width, size.height));
-
                                             compositor.replace_or_push("invalid-regex", popup);
                                         },
                                     ));

helix-term/src/ui/popup.rs

@@ -15,18 +15,18 @@ use helix_view::{
     Editor,
 };
 
+const MIN_HEIGHT: u16 = 4;
+
 // 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,
-    size: (u16, u16),
-    child_size: (u16, u16),
     area: Rect,
     position_bias: Open,
-    scroll: usize,
+    scroll_half_pages: usize,
     auto_close: bool,
     ignore_escape_key: bool,
     id: &'static str,
@@ -39,11 +39,9 @@ impl<T: Component> Popup<T> {
             contents,
             position: None,
             margin: Margin::none(),
-            size: (0, 0),
             position_bias: Open::Below,
-            child_size: (0, 0),
             area: Rect::new(0, 0, 0, 0),
-            scroll: 0,
+            scroll_half_pages: 0,
             auto_close: false,
             ignore_escape_key: false,
             id,
@@ -95,15 +93,52 @@ impl<T: Component> Popup<T> {
         self
     }
 
-    /// Calculate the position where the popup should be rendered and return the coordinates of the
-    /// top left corner.
-    pub fn get_rel_position(&mut self, viewport: Rect, editor: &Editor) -> (u16, u16) {
+    pub fn scroll_half_page_down(&mut self) {
+        self.scroll_half_pages += 1;
+    }
+
+    pub fn scroll_half_page_up(&mut self) {
+        self.scroll_half_pages = self.scroll_half_pages.saturating_sub(1);
+    }
+
+    /// Toggles the Popup's scrollbar.
+    /// Consider disabling the scrollbar in case the child
+    /// already has its own.
+    pub fn with_scrollbar(mut self, enable_scrollbar: bool) -> Self {
+        self.has_scrollbar = enable_scrollbar;
+        self
+    }
+
+    pub fn contents(&self) -> &T {
+        &self.contents
+    }
+
+    pub fn contents_mut(&mut self) -> &mut T {
+        &mut self.contents
+    }
+
+    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)
+    }
+
+    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
             .position
             .get_or_insert_with(|| editor.cursor().0.unwrap_or_default());
 
-        let (width, height) = self.size;
-
         // 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
@@ -115,8 +150,8 @@ impl<T: Component> Popup<T> {
             rel_x = rel_x.saturating_sub((rel_x + width).saturating_sub(viewport.width));
         }
 
-        let can_put_below = viewport.height > rel_y + height;
-        let can_put_above = rel_y.checked_sub(height).is_some();
+        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 {
             Open::Below => match can_put_below {
                 true => Open::Below,
@@ -128,61 +163,19 @@ impl<T: Component> Popup<T> {
             },
         };
 
-        rel_y = match final_pos {
-            Open::Above => rel_y.saturating_sub(height),
-            Open::Below => rel_y + 1,
-        };
-
-        (rel_x, rel_y)
-    }
-
-    pub fn get_size(&self) -> (u16, u16) {
-        (self.size.0, self.size.1)
-    }
-
-    pub fn scroll(&mut self, offset: usize, direction: bool) {
-        if direction {
-            let max_offset = self.child_size.1.saturating_sub(self.size.1);
-            self.scroll = (self.scroll + offset).min(max_offset as usize);
-        } else {
-            self.scroll = self.scroll.saturating_sub(offset);
+        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)
+            }
+            Open::Below => {
+                rel_y += 1;
+                let y_max = viewport.bottom().min(height + rel_y);
+                Rect::new(rel_x, rel_y, width, y_max - rel_y)
+            }
         }
     }
 
-    pub fn scroll_half_page_down(&mut self) {
-        self.scroll(self.size.1 as usize / 2, true)
-    }
-
-    pub fn scroll_half_page_up(&mut self) {
-        self.scroll(self.size.1 as usize / 2, false)
-    }
-
-    /// Toggles the Popup's scrollbar.
-    /// Consider disabling the scrollbar in case the child
-    /// already has its own.
-    pub fn with_scrollbar(mut self, enable_scrollbar: bool) -> Self {
-        self.has_scrollbar = enable_scrollbar;
-        self
-    }
-
-    pub fn contents(&self) -> &T {
-        &self.contents
-    }
-
-    pub fn contents_mut(&mut self) -> &mut T {
-        &mut self.contents
-    }
-
-    pub fn area(&mut self, viewport: Rect, editor: &Editor) -> Rect {
-        // trigger required_size so we recalculate if the child changed
-        self.required_size((viewport.width, viewport.height));
-
-        let (rel_x, rel_y) = self.get_rel_position(viewport, editor);
-
-        // clip to viewport
-        viewport.intersection(Rect::new(rel_x, rel_y, self.size.0, self.size.1))
-    }
-
     fn handle_mouse_event(
         &mut self,
         &MouseEvent {
@@ -266,38 +259,41 @@ 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)> {
-        let max_width = 120.min(viewport.0);
-        let max_height = 26.min(viewport.1.saturating_sub(2)); // add some spacing in the viewport
+    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 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");
 
-        self.child_size = (width, height);
-        self.size = (
-            (width + self.margin.width()).min(max_width),
-            (height + self.margin.height()).min(max_height),
+        let size = (
+            (width + self.margin.width()).min(MAX_WIDTH),
+            (height + self.margin.height()).min(MAX_HEIGHT),
         );
 
-        Some(self.size)
+        Some(size)
     }
 
     fn render(&mut self, viewport: Rect, surface: &mut Surface, cx: &mut Context) {
-        let area = self.area(viewport, cx.editor);
-        self.area = area;
+        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");
 
-        // required_size() calculates the popup size without taking account of self.position
-        // so we need to correct the popup height to correctly calculate the scroll
-        self.size.1 = area.height;
+        let area = self.area_internal(viewport, cx.editor, child_size);
+        self.area = area;
 
-        // re-clamp scroll offset
-        let max_offset = self.child_size.1.saturating_sub(self.size.1);
-        self.scroll = self.scroll.min(max_offset as usize);
-        cx.scroll = Some(self.scroll);
+        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");
@@ -325,9 +321,8 @@ impl<T: Component> Component for Popup<T> {
         // render scrollbar if contents do not fit
         if self.has_scrollbar {
             let win_height = (inner.height as usize).saturating_sub(2 * border);
-            let len = (self.child_size.1 as usize).saturating_sub(2 * border);
+            let len = (child_size.1 as usize).saturating_sub(2 * border);
             let fits = len <= win_height;
-            let scroll = self.scroll;
             let scroll_style = cx.editor.theme.get("ui.menu.scroll");
 
             const fn div_ceil(a: usize, b: usize) -> usize {