From 38b78151012147942ecdb2f2303743751e88511c 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 3. completion logic doesn't have any dependencies to SignatureHelp Popup Signed-off-by: Ben Fekih, Hichem --- helix-term/src/handlers/completion.rs | 13 +- helix-term/src/handlers/signature_help.rs | 28 +--- helix-term/src/ui/completion.rs | 19 +-- helix-term/src/ui/editor.rs | 12 +- helix-term/src/ui/mod.rs | 8 +- helix-term/src/ui/popup.rs | 152 ++++++++---------- .../src/ui/{lsp.rs => signature_help.rs} | 54 ++++++- helix-view/src/editor.rs | 2 + 8 files changed, 139 insertions(+), 149 deletions(-) rename helix-term/src/ui/{lsp.rs => signature_help.rs} (76%) diff --git a/helix-term/src/handlers/completion.rs b/helix-term/src/handlers/completion.rs index 491ca56389c38..0b61b70e10dfb 100644 --- a/helix-term/src/handlers/completion.rs +++ b/helix-term/src/handlers/completion.rs @@ -26,8 +26,7 @@ use crate::events::{OnModeSwitch, PostCommand, PostInsertChar}; use crate::job::{dispatch, dispatch_blocking}; use crate::keymap::MappableCommand; use crate::ui::editor::InsertEvent; -use crate::ui::lsp::SignatureHelp; -use crate::ui::{self, CompletionItem, Popup}; +use crate::ui::{self, CompletionItem}; use super::Handlers; @@ -306,20 +305,12 @@ fn show_completion( return; } - let size = compositor.size(); let ui = compositor.find::().unwrap(); if ui.completion.is_some() { return; } - let completion_area = ui.set_completion(editor, savepoint, items, trigger.pos, size); - let signature_help_area = compositor - .find_id::>(SignatureHelp::ID) - .map(|signature_help| signature_help.area(size, editor)); - // Delete the signature help popup if they intersect. - if matches!((completion_area, signature_help_area),(Some(a), Some(b)) if a.intersects(b)) { - compositor.remove(SignatureHelp::ID); - } + ui.set_completion(editor, savepoint, items, trigger.pos); } pub fn trigger_auto_completion( diff --git a/helix-term/src/handlers/signature_help.rs b/helix-term/src/handlers/signature_help.rs index 3c746548ac8c4..e4b093130eeb8 100644 --- a/helix-term/src/handlers/signature_help.rs +++ b/helix-term/src/handlers/signature_help.rs @@ -14,13 +14,11 @@ use helix_view::Editor; use tokio::sync::mpsc::Sender; use tokio::time::Instant; -use crate::commands::Open; use crate::compositor::Compositor; use crate::events::{OnModeSwitch, PostInsertChar}; use crate::handlers::Handlers; -use crate::ui::lsp::SignatureHelp; -use crate::ui::Popup; -use crate::{job, ui}; +use crate::job; +use crate::ui::signature_help::{SignatureHelp, SignaturePopup}; #[derive(Debug)] enum State { @@ -231,26 +229,8 @@ pub fn show_signature_help( }; contents.set_active_param_range(active_param_range()); - let old_popup = compositor.find_id::>(SignatureHelp::ID); - let mut popup = Popup::new(SignatureHelp::ID, contents) - .position(old_popup.and_then(|p| p.get_position())) - .position_bias(Open::Above) - .ignore_escape_key(true); - - // Don't create a popup if it intersects the auto-complete menu. - let size = compositor.size(); - if compositor - .find::() - .unwrap() - .completion - .as_mut() - .map(|completion| completion.area(size, editor)) - .filter(|area| area.intersects(popup.area(size, editor))) - .is_some() - { - return; - } - + let old_popup = compositor.find_id::(SignatureHelp::ID); + let popup = SignaturePopup::new(contents, old_popup.and_then(|p| p.get_position())); compositor.replace_or_push(SignatureHelp::ID, popup); } diff --git a/helix-term/src/ui/completion.rs b/helix-term/src/ui/completion.rs index 735bc956adcf7..ba74bf9303544 100644 --- a/helix-term/src/ui/completion.rs +++ b/helix-term/src/ui/completion.rs @@ -8,7 +8,6 @@ use helix_view::{ document::SavePoint, editor::CompleteAction, graphics::Margin, - handlers::lsp::SignatureHelpInvoked, theme::{Modifier, Style}, ViewId, }; @@ -329,13 +328,6 @@ impl Completion { trigger_auto_completion(&editor.handlers.completions, editor, true); } }; - - // In case the popup was deleted because of an intersection w/ the auto-complete menu. - if event != PromptEvent::Update { - editor - .handlers - .trigger_signature_help(SignatureHelpInvoked::Automatic, editor); - } }); let margin = if editor.menu_border() { @@ -436,6 +428,10 @@ impl Component for Completion { } fn render(&mut self, area: Rect, surface: &mut Surface, cx: &mut Context) { + cx.editor.completion_areas.clear(); + let popup_area = self.popup.area(area, cx.editor); + cx.editor.completion_areas.push(popup_area); + self.popup.render(area, surface, cx); // if we have a selection, render a markdown popup on top/below with info @@ -493,12 +489,6 @@ 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 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; @@ -544,6 +534,7 @@ impl Component for Completion { Widget::render(Block::default().borders(Borders::ALL), doc_area, surface); } + cx.editor.completion_areas.push(doc_area); markdown_doc.render(doc_area, surface, cx); } } diff --git a/helix-term/src/ui/editor.rs b/helix-term/src/ui/editor.rs index 7b130a38ae623..0965aed705d4f 100644 --- a/helix-term/src/ui/editor.rs +++ b/helix-term/src/ui/editor.rs @@ -1020,27 +1020,23 @@ impl EditorView { savepoint: Arc, items: Vec, trigger_offset: usize, - size: Rect, - ) -> Option { - let mut completion = Completion::new(editor, savepoint, items, trigger_offset); - + ) { + let completion = Completion::new(editor, savepoint, items, trigger_offset); if completion.is_empty() { // skip if we got no completion results - return None; + return; } - let area = completion.area(size, editor); editor.last_completion = Some(CompleteAction::Triggered); 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) } pub fn clear_completion(&mut self, editor: &mut Editor) { self.completion = None; + editor.completion_areas.clear(); if let Some(last_completion) = editor.last_completion.take() { match last_completion { CompleteAction::Triggered => (), diff --git a/helix-term/src/ui/mod.rs b/helix-term/src/ui/mod.rs index b5969818cf563..930dab0091f95 100644 --- a/helix-term/src/ui/mod.rs +++ b/helix-term/src/ui/mod.rs @@ -2,18 +2,18 @@ mod completion; mod document; pub(crate) mod editor; mod info; -pub mod lsp; mod markdown; pub mod menu; pub mod overlay; pub mod picker; pub mod popup; mod prompt; +pub mod signature_help; 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); }, )); diff --git a/helix-term/src/ui/popup.rs b/helix-term/src/ui/popup.rs index 124b4402fdb1f..ab971fb0b2c2f 100644 --- a/helix-term/src/ui/popup.rs +++ b/helix-term/src/ui/popup.rs @@ -15,6 +15,8 @@ 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) @@ -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 @@ impl Popup { 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,43 @@ impl Popup { 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 { + // trigger required_size so we recalculate if the child changed + 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 width = width.min(viewport.width); + let height = height.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 +141,8 @@ impl Popup { 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 +154,19 @@ impl Popup { }, }; - 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 +250,41 @@ 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)> { - 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 = 120u16; + const MAX_HEIGHT: u16 = 26u16; - 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 @@ impl Component for Popup { // 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 { diff --git a/helix-term/src/ui/lsp.rs b/helix-term/src/ui/signature_help.rs similarity index 76% rename from helix-term/src/ui/lsp.rs rename to helix-term/src/ui/signature_help.rs index a3698e38d8795..b50b234f9f5a3 100644 --- a/helix-term/src/ui/lsp.rs +++ b/helix-term/src/ui/signature_help.rs @@ -1,17 +1,63 @@ use std::sync::Arc; use arc_swap::ArcSwap; -use helix_core::syntax; +use helix_core::{syntax, Position}; use helix_view::graphics::{Margin, Rect, Style}; +use helix_view::input::Event; use tui::buffer::Buffer; use tui::widgets::{BorderType, Paragraph, Widget, Wrap}; -use crate::compositor::{Component, Compositor, Context}; +use crate::commands::Open; +use crate::compositor::{Component, Compositor, Context, EventResult}; use crate::ui::Markdown; use super::Popup; +pub struct SignaturePopup { + popup: Popup, +} + +impl SignaturePopup { + pub fn new(contents: SignatureHelp, pos: Option) -> Self { + Self { + popup: Popup::new(SignatureHelp::ID, contents) + .position(pos) + .position_bias(Open::Above) + .ignore_escape_key(true), + } + } + + pub fn get_position(&self) -> Option { + self.popup.get_position() + } +} + +impl Component for SignaturePopup { + fn handle_event(&mut self, event: &Event, ctx: &mut Context) -> EventResult { + self.popup.handle_event(event, ctx) + } + + fn render(&mut self, area: Rect, frame: &mut Buffer, ctx: &mut Context) { + let popup_area = self.popup.area(area, ctx.editor); + + for comp_area in ctx.editor.completion_areas.iter() { + if comp_area.intersects(popup_area) { + return; + } + } + self.popup.render(area, frame, ctx); + } + + fn required_size(&mut self, viewport: (u16, u16)) -> Option<(u16, u16)> { + self.popup.required_size(viewport) + } + + fn id(&self) -> Option<&'static str> { + self.popup.id() + } +} + pub struct SignatureHelp { signature: String, signature_doc: Option, @@ -47,8 +93,8 @@ impl SignatureHelp { self.active_param_range = offset; } - pub fn visible_popup(compositor: &mut Compositor) -> Option<&mut Popup> { - compositor.find_id::>(Self::ID) + pub fn visible_popup(compositor: &mut Compositor) -> Option<&mut SignaturePopup> { + compositor.find_id::(Self::ID) } } diff --git a/helix-view/src/editor.rs b/helix-view/src/editor.rs index d7058d3ef0221..20088dcdf9742 100644 --- a/helix-view/src/editor.rs +++ b/helix-view/src/editor.rs @@ -991,6 +991,7 @@ pub struct Editor { redraw_timer: Pin>, last_motion: Option, pub last_completion: Option, + pub completion_areas: Vec, pub exit_code: i32, @@ -1121,6 +1122,7 @@ impl Editor { redraw_timer: Box::pin(sleep(Duration::MAX)), last_motion: None, last_completion: None, + completion_areas: Vec::new(), config, auto_pairs, exit_code: 0,