From a062c0f1bc62dfbf0d61e15b09b5bd1a9c866cd0 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Mon, 16 Dec 2024 23:17:36 -0700 Subject: [PATCH] Improve code context menu layout position esp visual stability (#22102) * Now decides whether the menu is above or below the target position before rendering it. This causes its position to no longer vary depending on the length of completions * When the text area is height constrained (< 12) lines, now chooses the side which has the most space. Before it would always display above if height constrained below. * Misc code cleanups Release Notes: - Improved completions menu layout to be more stable and use available space better. --- crates/editor/src/code_context_menus.rs | 59 ++++++----- crates/editor/src/editor.rs | 52 +++++----- crates/editor/src/element.rs | 125 ++++++++++++++++-------- crates/ui/src/components/popover.rs | 12 ++- 4 files changed, 155 insertions(+), 93 deletions(-) diff --git a/crates/editor/src/code_context_menus.rs b/crates/editor/src/code_context_menus.rs index 9ed52a252e541..065f5897af253 100644 --- a/crates/editor/src/code_context_menus.rs +++ b/crates/editor/src/code_context_menus.rs @@ -4,8 +4,8 @@ use std::{cell::Cell, cmp::Reverse, ops::Range, rc::Rc}; use fuzzy::{StringMatch, StringMatchCandidate}; use gpui::{ div, px, uniform_list, AnyElement, BackgroundExecutor, Div, FontWeight, ListSizingBehavior, - Model, Pixels, ScrollStrategy, SharedString, StrikethroughStyle, StyledText, - UniformListScrollHandle, ViewContext, WeakView, + Model, ScrollStrategy, SharedString, StrikethroughStyle, StyledText, UniformListScrollHandle, + ViewContext, WeakView, }; use language::Buffer; use language::{CodeLabel, Documentation}; @@ -106,22 +106,24 @@ impl CodeContextMenu { } } + pub fn origin(&self, cursor_position: DisplayPoint) -> ContextMenuOrigin { + match self { + CodeContextMenu::Completions(menu) => menu.origin(cursor_position), + CodeContextMenu::CodeActions(menu) => menu.origin(cursor_position), + } + } pub fn render( &self, - cursor_position: DisplayPoint, style: &EditorStyle, - max_height: Pixels, + max_height_in_lines: u32, workspace: Option>, cx: &mut ViewContext, - ) -> (ContextMenuOrigin, AnyElement) { + ) -> AnyElement { match self { - CodeContextMenu::Completions(menu) => ( - ContextMenuOrigin::EditorPoint(cursor_position), - menu.render(style, max_height, workspace, cx), - ), - CodeContextMenu::CodeActions(menu) => { - menu.render(cursor_position, style, max_height, cx) + CodeContextMenu::Completions(menu) => { + menu.render(style, max_height_in_lines, workspace, cx) } + CodeContextMenu::CodeActions(menu) => menu.render(style, max_height_in_lines, cx), } } } @@ -322,13 +324,19 @@ impl CompletionsMenu { !self.matches.is_empty() } + fn origin(&self, cursor_position: DisplayPoint) -> ContextMenuOrigin { + ContextMenuOrigin::EditorPoint(cursor_position) + } + fn render( &self, style: &EditorStyle, - max_height: Pixels, + max_height_in_lines: u32, workspace: Option>, cx: &mut ViewContext, ) -> AnyElement { + let max_height = max_height_in_lines as f32 * cx.line_height(); + let completions = self.completions.borrow_mut(); let show_completion_documentation = self.show_completion_documentation; let widest_completion_ix = self @@ -496,7 +504,7 @@ impl CompletionsMenu { }, ) .occlude() - .max_h(max_height) + .max_h(max_height_in_lines as f32 * cx.line_height()) .track_scroll(self.scroll_handle.clone()) .with_width_from_item(widest_completion_ix) .with_sizing_behavior(ListSizingBehavior::Infer); @@ -779,13 +787,20 @@ impl CodeActionsMenu { !self.actions.is_empty() } + fn origin(&self, cursor_position: DisplayPoint) -> ContextMenuOrigin { + if let Some(row) = self.deployed_from_indicator { + ContextMenuOrigin::GutterIndicator(row) + } else { + ContextMenuOrigin::EditorPoint(cursor_position) + } + } + fn render( &self, - cursor_position: DisplayPoint, _style: &EditorStyle, - max_height: Pixels, + max_height_in_lines: u32, cx: &mut ViewContext, - ) -> (ContextMenuOrigin, AnyElement) { + ) -> AnyElement { let actions = self.actions.clone(); let selected_item = self.selected_item; let list = uniform_list( @@ -857,7 +872,7 @@ impl CodeActionsMenu { }, ) .occlude() - .max_h(max_height) + .max_h(max_height_in_lines as f32 * cx.line_height()) .track_scroll(self.scroll_handle.clone()) .with_width_from_item( self.actions @@ -873,14 +888,6 @@ impl CodeActionsMenu { ) .with_sizing_behavior(ListSizingBehavior::Infer); - let element = Popover::new().child(list).into_any_element(); - - let cursor_position = if let Some(row) = self.deployed_from_indicator { - ContextMenuOrigin::GutterIndicator(row) - } else { - ContextMenuOrigin::EditorPoint(cursor_position) - }; - - (cursor_position, element) + Popover::new().child(list).into_any_element() } } diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 39259c5eed05b..7be5bd7ab2abe 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -1382,18 +1382,16 @@ impl Editor { if self.pending_rename.is_some() { key_context.add("renaming"); } - if self.context_menu_visible() { - match self.context_menu.borrow().as_ref() { - Some(CodeContextMenu::Completions(_)) => { - key_context.add("menu"); - key_context.add("showing_completions") - } - Some(CodeContextMenu::CodeActions(_)) => { - key_context.add("menu"); - key_context.add("showing_code_actions") - } - None => {} + match self.context_menu.borrow().as_ref() { + Some(CodeContextMenu::Completions(_)) => { + key_context.add("menu"); + key_context.add("showing_completions") + } + Some(CodeContextMenu::CodeActions(_)) => { + key_context.add("menu"); + key_context.add("showing_code_actions") } + None => {} } // Disable vim contexts when a sub-editor (e.g. rename/inline assistant) is focused. @@ -4999,6 +4997,7 @@ impl Editor { })) } + #[cfg(feature = "test-support")] pub fn context_menu_visible(&self) -> bool { self.context_menu .borrow() @@ -5006,21 +5005,30 @@ impl Editor { .map_or(false, |menu| menu.visible()) } + fn context_menu_origin(&self, cursor_position: DisplayPoint) -> Option { + self.context_menu + .borrow() + .as_ref() + .map(|menu| menu.origin(cursor_position)) + } + fn render_context_menu( &self, - cursor_position: DisplayPoint, style: &EditorStyle, - max_height: Pixels, + max_height_in_lines: u32, cx: &mut ViewContext, - ) -> Option<(ContextMenuOrigin, AnyElement)> { - self.context_menu.borrow().as_ref().map(|menu| { - menu.render( - cursor_position, - style, - max_height, - self.workspace.as_ref().map(|(w, _)| w.clone()), - cx, - ) + ) -> Option { + self.context_menu.borrow().as_ref().and_then(|menu| { + if menu.visible() { + Some(menu.render( + style, + max_height_in_lines, + self.workspace.as_ref().map(|(w, _)| w.clone()), + cx, + )) + } else { + None + } }) } diff --git a/crates/editor/src/element.rs b/crates/editor/src/element.rs index be04c692e14e7..3a0cd6477fc41 100644 --- a/crates/editor/src/element.rs +++ b/crates/editor/src/element.rs @@ -70,8 +70,8 @@ use std::{ }; use sum_tree::Bias; use theme::{ActiveTheme, Appearance, PlayerColor}; -use ui::prelude::*; use ui::{h_flex, ButtonLike, ButtonStyle, ContextMenu, Tooltip}; +use ui::{prelude::*, POPOVER_Y_PADDING}; use unicode_segmentation::UnicodeSegmentation; use util::RangeExt; use util::ResultExt; @@ -2771,7 +2771,6 @@ impl EditorElement { fn layout_context_menu( &self, line_height: Pixels, - hitbox: &Hitbox, text_hitbox: &Hitbox, content_origin: gpui::Point, start_row: DisplayRow, @@ -2780,56 +2779,98 @@ impl EditorElement { newest_selection_head: DisplayPoint, gutter_overshoot: Pixels, cx: &mut WindowContext, - ) -> bool { - let max_height = cmp::min( - 12. * line_height, - cmp::max(3. * line_height, (hitbox.size.height - line_height) / 2.), - ); - let Some((position, mut context_menu)) = self.editor.update(cx, |editor, cx| { - if editor.context_menu_visible() { - editor.render_context_menu(newest_selection_head, &self.style, max_height, cx) - } else { - None - } - }) else { - return false; + ) { + let Some(context_menu_origin) = self + .editor + .read(cx) + .context_menu_origin(newest_selection_head) + else { + return; }; - - let context_menu_size = context_menu.layout_as_root(AvailableSpace::min_size(), cx); - - let (x, y) = match position { - crate::ContextMenuOrigin::EditorPoint(point) => { - let cursor_row_layout = &line_layouts[point.row().minus(start_row) as usize]; - let x = cursor_row_layout.x_for_index(point.column() as usize) - - scroll_pixel_position.x; - let y = point.row().next_row().as_f32() * line_height - scroll_pixel_position.y; - (x, y) + let target_offset = match context_menu_origin { + crate::ContextMenuOrigin::EditorPoint(display_point) => { + let cursor_row_layout = + &line_layouts[display_point.row().minus(start_row) as usize]; + gpui::Point { + x: cursor_row_layout.x_for_index(display_point.column() as usize) + - scroll_pixel_position.x, + y: display_point.row().next_row().as_f32() * line_height + - scroll_pixel_position.y, + } } crate::ContextMenuOrigin::GutterIndicator(row) => { // Context menu was spawned via a click on a gutter. Ensure it's a bit closer to the indicator than just a plain first column of the // text field. - let x = -gutter_overshoot; - let y = row.next_row().as_f32() * line_height - scroll_pixel_position.y; - (x, y) + gpui::Point { + x: -gutter_overshoot, + y: row.next_row().as_f32() * line_height - scroll_pixel_position.y, + } } }; - let mut list_origin = content_origin + point(x, y); - let list_width = context_menu_size.width; - let list_height = context_menu_size.height; + // If the context menu's max height won't fit below, then flip it above the line and display + // it in reverse order. If the available space above is less than below. + let unconstrained_max_height = line_height * 12. + POPOVER_Y_PADDING; + let min_height = line_height * 3. + POPOVER_Y_PADDING; + let target_position = content_origin + target_offset; + let y_overflows_below = target_position.y + unconstrained_max_height > text_hitbox.bottom(); + let bottom_y_when_flipped = target_position.y - line_height; + let available_above = bottom_y_when_flipped - text_hitbox.top(); + let available_below = text_hitbox.bottom() - target_position.y; + let mut y_is_flipped = y_overflows_below && available_above > available_below; + let mut max_height = cmp::min( + unconstrained_max_height, + if y_is_flipped { + available_above + } else { + available_below + }, + ); - // Snap the right edge of the list to the right edge of the window if - // its horizontal bounds overflow. - if list_origin.x + list_width > cx.viewport_size().width { - list_origin.x = (cx.viewport_size().width - list_width).max(Pixels::ZERO); + // If less than 3 lines fit within the text bounds, instead fit within the window. + if max_height < 3. * line_height { + let available_above = bottom_y_when_flipped; + let available_below = cx.viewport_size().height - target_position.y; + if available_below > 3. * line_height { + y_is_flipped = false; + max_height = min_height; + } else if available_above > 3. * line_height { + y_is_flipped = true; + max_height = min_height; + } else if available_above > available_below { + y_is_flipped = true; + max_height = available_above; + } else { + y_is_flipped = false; + max_height = available_below; + } } - if list_origin.y + list_height > text_hitbox.bottom_right().y { - list_origin.y -= line_height + list_height; - } + let max_height_in_lines = ((max_height - POPOVER_Y_PADDING) / line_height).floor() as u32; + + let Some(mut menu) = self.editor.update(cx, |editor, cx| { + editor.render_context_menu(&self.style, max_height_in_lines, cx) + }) else { + return; + }; + + let menu_size = menu.layout_as_root(AvailableSpace::min_size(), cx); + let menu_position = gpui::Point { + x: if target_position.x + menu_size.width > cx.viewport_size().width { + // Snap the right edge of the list to the right edge of the window if its horizontal bounds + // overflow. + (cx.viewport_size().width - menu_size.width).max(Pixels::ZERO) + } else { + target_position.x + }, + y: if y_is_flipped { + bottom_y_when_flipped - menu_size.height + } else { + target_position.y + }, + }; - cx.defer_draw(context_menu, list_origin, 1); - true + cx.defer_draw(menu, menu_position, 1); } #[allow(clippy::too_many_arguments)] @@ -5893,13 +5934,11 @@ impl Element for EditorElement { rows_with_hunk_bounds }, ); - let mut _context_menu_visible = false; let mut code_actions_indicator = None; if let Some(newest_selection_head) = newest_selection_head { if (start_row..end_row).contains(&newest_selection_head.row()) { - _context_menu_visible = self.layout_context_menu( + self.layout_context_menu( line_height, - &hitbox, &text_hitbox, content_origin, start_row, diff --git a/crates/ui/src/components/popover.rs b/crates/ui/src/components/popover.rs index 5bd6c1ed7ccbc..3b16b0ccac000 100644 --- a/crates/ui/src/components/popover.rs +++ b/crates/ui/src/components/popover.rs @@ -3,10 +3,13 @@ use crate::prelude::*; use crate::v_flex; use gpui::{ - div, AnyElement, Element, IntoElement, ParentElement, RenderOnce, Styled, WindowContext, + div, AnyElement, Element, IntoElement, ParentElement, Pixels, RenderOnce, Styled, WindowContext, }; use smallvec::SmallVec; +/// Y height added beyond the size of the contents. +pub const POPOVER_Y_PADDING: Pixels = px(8.); + /// A popover is used to display a menu or show some options. /// /// Clicking the element that launches the popover should not change the current view, @@ -45,7 +48,12 @@ impl RenderOnce for Popover { div() .flex() .gap_1() - .child(v_flex().elevation_2(cx).py_1().children(self.children)) + .child( + v_flex() + .elevation_2(cx) + .py(POPOVER_Y_PADDING / 2.) + .children(self.children), + ) .when_some(self.aside, |this, aside| { this.child( v_flex()