Skip to content

Commit

Permalink
Improve popup position
Browse files Browse the repository at this point in the history 
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. Below the curson there is always enough (5 lines) for the completion
   popup. Now it will be always shown below the cursor
3. 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 <[email protected]>
  • Loading branch information
@karthago1
karthago1 committed Apr 7, 2024
1 parent 97afd67 commit e7e7099
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 98 deletions.
7 changes: 1 addition & 6 deletions helix-term/src/ui/completion.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -498,12 +498,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;
Expand Down
13 changes: 0 additions & 13 deletions helix-term/src/ui/menu.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -394,19 +394,6 @@ impl<T: Item + 'static> Component for Menu<T> {

let render_borders = cx.editor.menu_border();

if !render_borders {
if let Some(cursor) = self.cursor {
let offset_from_top = cursor - scroll;
let left = &mut surface[(area.left(), area.y + offset_from_top as u16)];
left.set_style(selected);
let right = &mut surface[(
area.right().saturating_sub(1),
area.y + offset_from_top as u16,
)];
right.set_style(selected);
}
}

let fits = len <= win_height;

let scroll_style = theme.get("ui.menu.scroll");
Expand Down
6 changes: 2 additions & 4 deletions helix-term/src/ui/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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);
},
));
Expand Down
141 changes: 66 additions & 75 deletions helix-term/src/ui/popup.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,17 @@ 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,
Expand All @@ -39,11 +38,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,
Expand Down Expand Up @@ -95,14 +92,39 @@ 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 {
// 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 position = self
.position
.get_or_insert_with(|| editor.cursor().0.unwrap_or_default());

let (width, height) = self.size;
.unwrap_or_else(|| 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
Expand All @@ -115,8 +137,10 @@ 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();
// TODO can_put_below should be useless since MIN_HEIGHT is 4 and there is always 4 lines
// below the cursor (ask a maintainer and remove the code related to can_put_below).
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,
Expand All @@ -128,59 +152,20 @@ impl<T: Component> Popup<T> {
},
};

rel_y = match final_pos {
Open::Above => rel_y.saturating_sub(height),
Open::Below => rel_y + 1,
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)
}
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)
}
};

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

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))
log::error!("{area:?}");
area
}

fn handle_mouse_event(
Expand Down Expand Up @@ -277,23 +262,30 @@ impl<T: Component> Component for Popup<T> {
.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 = (
let size = (
(width + self.margin.width()).min(max_width),
(height + self.margin.height()).min(max_height),
);

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

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;
cx.scroll = Some(self.scroll);

let child_size = self
.contents
.required_size((area.width, area.width))
.expect("Component needs required_size implemented in order to be embedded in a popup");

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");
Expand Down Expand Up @@ -321,9 +313,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 {
Expand Down

0 comments on commit e7e7099

Please sign in to comment.