From af4ff80524053b28cfdaafdbbbbca25d533b71a2 Mon Sep 17 00:00:00 2001 From: "Ben Fekih, Hichem" Date: Mon, 8 Apr 2024 18:07:56 +0200 Subject: [PATCH] Improve popup position Make the popup positions more consistent. Improvements: 1. if the signature popup content is bigger than the available space, then the popup is always shown under the cursor, even if there more space above the cursor than below 2. There is no mutation anymore inside required_size. Maybe in the future we can update all widgets to have no mutations and change the trait Signed-off-by: Ben Fekih, Hichem --- helix-term/src/ui/completion.rs | 7 +- helix-term/src/ui/editor.rs | 1 - helix-term/src/ui/mod.rs | 6 +- helix-term/src/ui/popup.rs | 154 +++++++++++++++----------------- 4 files changed, 73 insertions(+), 95 deletions(-) diff --git a/helix-term/src/ui/completion.rs b/helix-term/src/ui/completion.rs index 735bc956..1fcb8e54 100644 --- a/helix-term/src/ui/completion.rs +++ b/helix-term/src/ui/completion.rs @@ -493,12 +493,7 @@ fn render(&mut self, area: Rect, surface: &mut Surface, cx: &mut Context) { 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; diff --git a/helix-term/src/ui/editor.rs b/helix-term/src/ui/editor.rs index 7b130a38..97f90f62 100644 --- a/helix-term/src/ui/editor.rs +++ b/helix-term/src/ui/editor.rs @@ -1034,7 +1034,6 @@ pub fn set_completion( 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) } diff --git a/helix-term/src/ui/mod.rs b/helix-term/src/ui/mod.rs index b5969818..5211c2e2 100644 --- a/helix-term/src/ui/mod.rs +++ b/helix-term/src/ui/mod.rs @@ -13,7 +13,7 @@ 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); }, )); diff --git a/helix-term/src/ui/popup.rs b/helix-term/src/ui/popup.rs index 124b4402..d1d7d6c5 100644 --- a/helix-term/src/ui/popup.rs +++ b/helix-term/src/ui/popup.rs @@ -15,6 +15,8 @@ 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) @@ -22,11 +24,9 @@ pub struct Popup { contents: T, position: Option, 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 @@ pub fn new(id: &'static str, contents: T) -> Self { 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,66 +93,12 @@ pub fn ignore_escape_key(mut self, ignore: bool) -> Self { 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) { - 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 - - // -- 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)); - } - - let can_put_below = viewport.height > rel_y + height; - let can_put_above = rel_y.checked_sub(height).is_some(); - let final_pos = match self.position_bias { - Open::Below => match can_put_below { - true => Open::Below, - false => Open::Above, - }, - Open::Above => match can_put_above { - true => Open::Above, - false => Open::Below, - }, - }; - - 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); - } - } - pub fn scroll_half_page_down(&mut self) { - self.scroll(self.size.1 as usize / 2, true) + self.scroll_half_pages += 1; } pub fn scroll_half_page_up(&mut self) { - self.scroll(self.size.1 as usize / 2, false) + self.scroll_half_pages = self.scroll_half_pages.saturating_sub(1); } /// Toggles the Popup's scrollbar. @@ -175,12 +119,52 @@ pub fn contents_mut(&mut self) -> &mut T { 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 (width, height) = self + .required_size((viewport.width, viewport.height)) + .expect("Component needs required_size implemented in order to be embedded in a popup"); - let (rel_x, rel_y) = self.get_rel_position(viewport, editor); + let width = width.min(viewport.width); + let height = height.min(viewport.height.saturating_sub(2)); // add some spacing in the viewport - // clip to viewport - viewport.intersection(Rect::new(rel_x, rel_y, self.size.0, self.size.1)) + 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 + + // -- 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)); + } + + 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, + false => Open::Above, + }, + Open::Above => match can_put_above { + true => Open::Above, + false => Open::Below, + }, + }; + + 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) + } + } } fn handle_mouse_event( @@ -266,38 +250,41 @@ fn handle_event(&mut self, event: &Event, cx: &mut Context) -> EventResult { // 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; - // 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 child_size = self + .contents + .required_size((area.width, area.height)) + .expect("Component needs required_size implemented in order to be embedded in a popup"); - // 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 +312,8 @@ fn render(&mut self, viewport: Rect, surface: &mut Surface, cx: &mut Context) { // 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 {