Skip to content

Commit

Permalink
Improve code context menu layout position esp visual stability
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@mgsloan
mgsloan committed Dec 17, 2024
1 parent fc5a810 commit 2788636
Show file tree
Hide file tree
Showing 4 changed files with 152 additions and 90 deletions.
53 changes: 30 additions & 23 deletions crates/editor/src/code_context_menus.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ 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,
Model, MouseButton, Pixels, ScrollStrategy, SharedString, StrikethroughStyle, StyledText,
UniformListScrollHandle, ViewContext, WeakView,
};
use language::Buffer;
Expand Down Expand Up @@ -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<WeakView<Workspace>>,
cx: &mut ViewContext<Editor>,
) -> (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),
}
}
}
Expand Down Expand Up @@ -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<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
Expand Down Expand Up @@ -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<Editor>,
) -> (ContextMenuOrigin, AnyElement) {
) -> AnyElement {
let actions = self.actions.clone();
let selected_item = self.selected_item;
let list = uniform_list(
Expand Down Expand Up @@ -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()
}
}
52 changes: 30 additions & 22 deletions crates/editor/src/editor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -4999,28 +4997,38 @@ impl Editor {
}))
}

#[cfg(feature = "test-support")]
pub fn context_menu_visible(&self) -> bool {
self.context_menu
.borrow()
.as_ref()
.map_or(false, |menu| menu.visible())
}

fn context_menu_origin(&self, cursor_position: DisplayPoint) -> Option<ContextMenuOrigin> {
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<Editor>,
) -> 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<AnyElement> {
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
}
})
}

Expand Down
125 changes: 82 additions & 43 deletions crates/editor/src/element.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -2771,7 +2771,6 @@ impl EditorElement {
fn layout_context_menu(
&self,
line_height: Pixels,
hitbox: &Hitbox,
text_hitbox: &Hitbox,
content_origin: gpui::Point<Pixels>,
start_row: DisplayRow,
Expand All @@ -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)]
Expand Down Expand Up @@ -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,
Expand Down
12 changes: 10 additions & 2 deletions crates/ui/src/components/popover.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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()
Expand Down

0 comments on commit 2788636

Please sign in to comment.