Improve context menu aside layout via custom logic (#22154)

Michael Sloan created

* 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

Change summary

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

Detailed changes

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<WeakView<Workspace>>,
         cx: &mut ViewContext<Editor>,
     ) -> 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<WeakView<Workspace>>,
+        cx: &mut ViewContext<Editor>,
+    ) -> Option<AnyElement> {
         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<bool>,
     show_completion_documentation: bool,
 }
 
@@ -160,7 +174,6 @@ impl CompletionsMenu {
         initial_position: Anchor,
         buffer: Model<Buffer>,
         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<WeakView<Workspace>>,
         cx: &mut ViewContext<Editor>,
     ) -> 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<WeakView<Workspace>>,
+        cx: &mut ViewContext<Editor>,
+    ) -> Option<AnyElement> {
+        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) {

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<AnyElement> {
         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<Editor>,
+    ) -> Option<AnyElement> {
+        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
             }

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<Pixels>,
+        target_bounds: Bounds<Pixels>,
+        max_target_bounds: Bounds<Pixels>,
+        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<Pixels>, wanted: Size<Pixels>| {
+            // 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)]

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<T>) -> Bounds<T> {
+        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<T> Bounds<T>
@@ -1097,6 +1109,21 @@ impl<T: Clone + Default + Debug + PartialOrd + Add<T, Output = T> + Sub<Output =
     }
 }
 
+impl<T> Bounds<T>
+where
+    T: Clone + Debug + Add<T, Output = T> + Sub<T, Output = T> + Default,
+{
+    /// Computes the space available within outer bounds.
+    pub fn space_within(&self, outer: &Self) -> Edges<T> {
+        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<T, Rhs> Mul<Rhs> for Bounds<T>
 where
     T: Mul<Rhs, Output = Rhs> + Clone + Default + Debug,