From 5d7b6141fd5481b50538b1c9e69db5eaacd4b558 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Tue, 17 Dec 2024 17:01:15 -0700 Subject: [PATCH] Improve context menu aside layout via custom logic (#22154) * Presence of the aside no longer affects position or size of the context menu. * Prefers to fit to the right, then on same side of line, then other side of line, within the following preference order: - Max possible size within text area. - Max possible size within window. - Actual size within window. This is the only case that could cause it to jump around with less stability. A further enhancement atop this might be to dynamically resize aside height to fit. Release notes are N/A as they are covered by the notes for #22102. Closes #8523 Release Notes: * N/A --- crates/editor/src/code_context_menus.rs | 168 ++++++++++++----------- crates/editor/src/editor.rs | 26 ++-- crates/editor/src/element.rs | 174 +++++++++++++++++++----- crates/gpui/src/geometry.rs | 27 ++++ 4 files changed, 276 insertions(+), 119 deletions(-) diff --git a/crates/editor/src/code_context_menus.rs b/crates/editor/src/code_context_menus.rs index cce6f6d7236936be11fee7f848b008a7bef7431a..8c017e923ca18bd51ce9c74899192b9816c3306d 100644 --- a/crates/editor/src/code_context_menus.rs +++ b/crates/editor/src/code_context_menus.rs @@ -1,5 +1,5 @@ use std::cell::RefCell; -use std::{cell::Cell, cmp::Reverse, ops::Range, rc::Rc}; +use std::{cmp::Reverse, ops::Range, rc::Rc}; use fuzzy::{StringMatch, StringMatchCandidate}; use gpui::{ @@ -14,8 +14,8 @@ use multi_buffer::{Anchor, ExcerptId}; use ordered_float::OrderedFloat; use project::{CodeAction, Completion, TaskSourceKind}; use task::ResolvedTask; -use ui::{prelude::*, Color, IntoElement, ListItem, Popover, Styled}; -use util::ResultExt as _; +use ui::{prelude::*, Color, IntoElement, ListItem, Pixels, Popover, Styled}; +use util::ResultExt; use workspace::Workspace; use crate::{ @@ -26,6 +26,8 @@ use crate::{ }; use crate::{AcceptInlineCompletion, InlineCompletionMenuHint, InlineCompletionText}; +pub const MAX_COMPLETIONS_ASIDE_WIDTH: Pixels = px(500.); + pub enum CodeContextMenu { Completions(CompletionsMenu), CodeActions(CodeActionsMenu), @@ -109,18 +111,31 @@ impl CodeContextMenu { CodeContextMenu::CodeActions(menu) => menu.origin(cursor_position), } } + pub fn render( &self, style: &EditorStyle, max_height_in_lines: u32, - workspace: Option>, cx: &mut ViewContext, ) -> AnyElement { + match self { + CodeContextMenu::Completions(menu) => menu.render(style, max_height_in_lines, cx), + CodeContextMenu::CodeActions(menu) => menu.render(style, max_height_in_lines, cx), + } + } + + pub fn render_aside( + &self, + style: &EditorStyle, + max_height: Pixels, + workspace: Option>, + cx: &mut ViewContext, + ) -> Option { match self { CodeContextMenu::Completions(menu) => { - menu.render(style, max_height_in_lines, workspace, cx) + menu.render_aside(style, max_height, workspace, cx) } - CodeContextMenu::CodeActions(menu) => menu.render(style, max_height_in_lines, cx), + CodeContextMenu::CodeActions(_) => None, } } } @@ -142,7 +157,6 @@ pub struct CompletionsMenu { pub selected_item: usize, scroll_handle: UniformListScrollHandle, resolve_completions: bool, - pub aside_was_displayed: Cell, show_completion_documentation: bool, } @@ -160,7 +174,6 @@ impl CompletionsMenu { initial_position: Anchor, buffer: Model, completions: Box<[Completion]>, - aside_was_displayed: bool, ) -> Self { let match_candidates = completions .iter() @@ -180,7 +193,6 @@ impl CompletionsMenu { selected_item: 0, scroll_handle: UniformListScrollHandle::new(), resolve_completions: true, - aside_was_displayed: Cell::new(aside_was_displayed), } } @@ -236,7 +248,6 @@ impl CompletionsMenu { selected_item: 0, scroll_handle: UniformListScrollHandle::new(), resolve_completions: false, - aside_was_displayed: Cell::new(false), show_completion_documentation: false, } } @@ -362,11 +373,8 @@ impl CompletionsMenu { &self, style: &EditorStyle, 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 @@ -393,71 +401,12 @@ impl CompletionsMenu { }) => provider_name.len(), }) .map(|(ix, _)| ix); + drop(completions); let selected_item = self.selected_item; - let style = style.clone(); - - let multiline_docs = match &self.entries[selected_item] { - CompletionEntry::Match(mat) if show_completion_documentation => { - match &completions[mat.candidate_id].documentation { - Some(Documentation::MultiLinePlainText(text)) => { - Some(div().child(SharedString::from(text.clone()))) - } - Some(Documentation::MultiLineMarkdown(parsed)) if !parsed.text.is_empty() => { - Some(div().child(render_parsed_markdown( - "completions_markdown", - parsed, - &style, - workspace, - cx, - ))) - } - Some(Documentation::Undocumented) if self.aside_was_displayed.get() => { - Some(div().child("No documentation")) - } - _ => None, - } - } - CompletionEntry::InlineCompletionHint(hint) => Some(match &hint.text { - InlineCompletionText::Edit { text, highlights } => div() - .my_1() - .rounded(px(6.)) - .bg(cx.theme().colors().editor_background) - .border_1() - .border_color(cx.theme().colors().border_variant) - .child( - gpui::StyledText::new(text.clone()) - .with_highlights(&style.text, highlights.clone()), - ), - InlineCompletionText::Move(text) => div().child(text.clone()), - }), - _ => None, - }; - - let aside_contents = if let Some(multiline_docs) = multiline_docs { - Some(multiline_docs) - } else if show_completion_documentation && self.aside_was_displayed.get() { - Some(div().child("Fetching documentation...")) - } else { - None - }; - self.aside_was_displayed.set(aside_contents.is_some()); - - let aside_contents = aside_contents.map(|div| { - div.id("multiline_docs") - .max_h(max_height) - .flex_1() - .px_1p5() - .py_1() - .max_w(px(640.)) - .w(px(450.)) - .overflow_y_scroll() - .occlude() - }); - - drop(completions); let completions = self.completions.clone(); let matches = self.entries.clone(); + let style = style.clone(); let list = uniform_list( cx.view().clone(), "completions", @@ -588,12 +537,71 @@ impl CompletionsMenu { .with_width_from_item(widest_completion_ix) .with_sizing_behavior(ListSizingBehavior::Infer); - Popover::new() - .child(list) - .when_some(aside_contents, |popover, aside_contents| { - popover.aside(aside_contents) - }) - .into_any_element() + Popover::new().child(list).into_any_element() + } + + fn render_aside( + &self, + style: &EditorStyle, + max_height: Pixels, + workspace: Option>, + cx: &mut ViewContext, + ) -> Option { + if !self.show_completion_documentation { + return None; + } + + let multiline_docs = match &self.entries[self.selected_item] { + CompletionEntry::Match(mat) => { + match self.completions.borrow_mut()[mat.candidate_id] + .documentation + .as_ref()? + { + Documentation::MultiLinePlainText(text) => { + div().child(SharedString::from(text.clone())) + } + Documentation::MultiLineMarkdown(parsed) if !parsed.text.is_empty() => div() + .child(render_parsed_markdown( + "completions_markdown", + parsed, + &style, + workspace, + cx, + )), + Documentation::MultiLineMarkdown(_) => return None, + Documentation::SingleLine(_) => return None, + Documentation::Undocumented => return None, + } + } + CompletionEntry::InlineCompletionHint(hint) => match &hint.text { + InlineCompletionText::Edit { text, highlights } => div() + .my_1() + .rounded(px(6.)) + .bg(cx.theme().colors().editor_background) + .border_1() + .border_color(cx.theme().colors().border_variant) + .child( + gpui::StyledText::new(text.clone()) + .with_highlights(&style.text, highlights.clone()), + ), + InlineCompletionText::Move(text) => div().child(text.clone()), + }, + }; + + Some( + Popover::new() + .child( + multiline_docs + .id("multiline_docs") + .max_h(max_height) + .px_0p5() + .min_w(px(260.)) + .max_w(MAX_COMPLETIONS_ASIDE_WIDTH) + .overflow_y_scroll() + .occlude(), + ) + .into_any_element(), + ) } pub async fn filter(&mut self, query: Option<&str>, executor: BackgroundExecutor) { diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 4265a5760c933f3d45f0b8c15e04d70028df8da2..7c7bb81a19d5248924b269a9843d336a97044d53 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -3682,10 +3682,6 @@ impl Editor { let query = Self::completion_query(&self.buffer.read(cx).read(cx), position); - let aside_was_displayed = match self.context_menu.borrow().deref() { - Some(CodeContextMenu::Completions(menu)) => menu.aside_was_displayed.get(), - _ => false, - }; let trigger_kind = match &options.trigger { Some(trigger) if buffer.read(cx).completion_triggers().contains(trigger) => { CompletionTriggerKind::TRIGGER_CHARACTER @@ -3720,7 +3716,6 @@ impl Editor { position, buffer.clone(), completions.into(), - aside_was_displayed, ); menu.filter(query.as_deref(), cx.background_executor().clone()) @@ -5118,12 +5113,27 @@ impl Editor { ) -> Option { self.context_menu.borrow().as_ref().and_then(|menu| { if menu.visible() { - Some(menu.render( + Some(menu.render(style, max_height_in_lines, cx)) + } else { + None + } + }) + } + + fn render_context_menu_aside( + &self, + style: &EditorStyle, + max_height: Pixels, + cx: &mut ViewContext, + ) -> Option { + self.context_menu.borrow().as_ref().and_then(|menu| { + if menu.visible() { + menu.render_aside( style, - max_height_in_lines, + max_height, 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 b75338c718fc0187fb76cfd6b5f6d2eee799efa6..1daaacbcb389fb25d48d79ec3e570a2a673e1c4b 100644 --- a/crates/editor/src/element.rs +++ b/crates/editor/src/element.rs @@ -1,6 +1,6 @@ use crate::{ blame_entry_tooltip::{blame_entry_relative_timestamp, BlameEntryTooltip}, - code_context_menus::CodeActionsMenu, + code_context_menus::{CodeActionsMenu, MAX_COMPLETIONS_ASIDE_WIDTH}, display_map::{ Block, BlockContext, BlockStyle, DisplaySnapshot, HighlightedChunk, ToDisplayPoint, }, @@ -2901,38 +2901,44 @@ impl EditorElement { else { return; }; - 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, + let target_position = content_origin + + 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: cmp::max( + px(0.), + cursor_row_layout.x_for_index(display_point.column() as usize) + - scroll_pixel_position.x, + ), + y: cmp::max( + px(0.), + 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. - gpui::Point { - x: -gutter_overshoot, - y: 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. + gpui::Point { + x: -gutter_overshoot, + y: row.next_row().as_f32() * line_height - scroll_pixel_position.y, + } } - } - }; + }; // 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 y_overflows_below = unconstrained_max_height > available_below; let mut y_is_flipped = y_overflows_below && available_above > available_below; - let mut max_height = cmp::min( + let mut height = cmp::min( unconstrained_max_height, if y_is_flipped { available_above @@ -2942,33 +2948,33 @@ impl EditorElement { ); // If less than 3 lines fit within the text bounds, instead fit within the window. - if max_height < 3. * line_height { + if height < min_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; + height = min_height; } else if available_above > 3. * line_height { y_is_flipped = true; - max_height = min_height; + height = min_height; } else if available_above > available_below { y_is_flipped = true; - max_height = available_above; + height = available_above; } else { y_is_flipped = false; - max_height = available_below; + height = available_below; } } - let max_height_in_lines = ((max_height - POPOVER_Y_PADDING) / line_height).floor() as u32; + let max_height_in_lines = ((height - POPOVER_Y_PADDING) / line_height).floor() as u32; - let Some(mut menu) = self.editor.update(cx, |editor, cx| { + let Some(mut menu_element) = 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_size = menu_element.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 @@ -2983,8 +2989,114 @@ impl EditorElement { target_position.y }, }; + cx.defer_draw(menu_element, menu_position, 1); + + let aside_element = self.editor.update(cx, |editor, cx| { + editor.render_context_menu_aside(&self.style, unconstrained_max_height, cx) + }); + if let Some(aside_element) = aside_element { + let menu_bounds = Bounds::new(menu_position, menu_size); + let max_menu_size = size(menu_size.width, unconstrained_max_height); + let max_menu_bounds = if y_is_flipped { + Bounds::new( + point( + menu_position.x, + bottom_y_when_flipped - max_menu_size.height, + ), + max_menu_size, + ) + } else { + Bounds::new(target_position, max_menu_size) + }; - cx.defer_draw(menu, menu_position, 1); + // Pad the target by 4 pixels to create a gap. + let mut extend_amount = Edges::all(px(4.)); + // Extend to include the cursored line to avoid overlapping it. + if y_is_flipped { + extend_amount.bottom = line_height; + } else { + extend_amount.top = line_height; + } + self.layout_context_menu_aside( + text_hitbox, + y_is_flipped, + menu_position, + menu_bounds.extend(extend_amount), + max_menu_bounds.extend(extend_amount), + unconstrained_max_height, + aside_element, + cx, + ); + } + } + + #[allow(clippy::too_many_arguments)] + fn layout_context_menu_aside( + &self, + text_hitbox: &Hitbox, + y_is_flipped: bool, + menu_position: gpui::Point, + target_bounds: Bounds, + max_target_bounds: Bounds, + max_height: Pixels, + aside: AnyElement, + cx: &mut WindowContext, + ) { + let mut aside = aside; + let actual_size = aside.layout_as_root(AvailableSpace::min_size(), cx); + + // Snap to right side of window if it would overflow. + let aside_x = cmp::min( + menu_position.x, + cx.viewport_size().width - actual_size.width, + ); + if aside_x < px(0.) { + // Not enough space, skip drawing. + return; + } + + let top_position = point(aside_x, target_bounds.top() - actual_size.height); + let bottom_position = point(aside_x, target_bounds.bottom()); + let right_position = point(target_bounds.right(), menu_position.y); + + let fit_horizontally_within = |available: Edges, wanted: Size| { + // Prefer to fit to the right, then on the same side of the line as the menu, then on + // the other side of the line. + if wanted.width < available.right { + Some(right_position) + } else if !y_is_flipped && wanted.height < available.bottom { + Some(bottom_position) + } else if !y_is_flipped && wanted.height < available.top { + Some(top_position) + } else if y_is_flipped && wanted.height < available.top { + Some(top_position) + } else if y_is_flipped && wanted.height < available.bottom { + Some(bottom_position) + } else { + None + } + }; + + // Prefer choosing a direction using max sizes rather than actual size for stability. + let mut available = max_target_bounds.space_within(&text_hitbox.bounds); + let mut wanted = size(MAX_COMPLETIONS_ASIDE_WIDTH, max_height); + let aside_position = fit_horizontally_within(available, wanted) + .or_else(|| { + // Fallback: fit max size in window. + available = max_target_bounds + .space_within(&Bounds::new(Default::default(), cx.viewport_size())); + fit_horizontally_within(available, wanted) + }) + .or_else(|| { + // Fallback: fit actual size in window. + wanted = actual_size; + fit_horizontally_within(available, wanted) + }); + + // Skip drawing if it doesn't fit anywhere. + if let Some(aside_position) = aside_position { + cx.defer_draw(aside, aside_position, 1); + } } #[allow(clippy::too_many_arguments)] diff --git a/crates/gpui/src/geometry.rs b/crates/gpui/src/geometry.rs index 7bf79fee7c1cd49ef4576775240c37bb3bc6e5b8..11d03b2f7a4f4d11fb0e2b6c55278618d5fc03e1 100644 --- a/crates/gpui/src/geometry.rs +++ b/crates/gpui/src/geometry.rs @@ -1003,6 +1003,18 @@ where size: self.size.clone() + size(double_amount.clone(), double_amount), } } + + /// Extends the bounds different amounts in each direction. + pub fn extend(&self, amount: Edges) -> Bounds { + Bounds { + origin: self.origin.clone() - point(amount.left.clone(), amount.top.clone()), + size: self.size.clone() + + size( + amount.left.clone() + amount.right.clone(), + amount.top.clone() + amount.bottom.clone(), + ), + } + } } impl Bounds @@ -1097,6 +1109,21 @@ impl + Sub Bounds +where + T: Clone + Debug + Add + Sub + Default, +{ + /// Computes the space available within outer bounds. + pub fn space_within(&self, outer: &Self) -> Edges { + Edges { + top: self.top().clone() - outer.top().clone(), + right: outer.right().clone() - self.right().clone(), + bottom: outer.bottom().clone() - self.bottom().clone(), + left: self.left().clone() - outer.left().clone(), + } + } +} + impl Mul for Bounds where T: Mul + Clone + Default + Debug,