Yehowshua/ZIm
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

editor: Prevent overlapping of signature/hover popovers and context menus (#31090)

Browse source 
Closes #29358

If hover popovers or signature popovers ever clash with the context menu
(like code completion or code actions), they find the best spot by
trying different directions around the context menu to show the popover.
If they can’t find a good spot, they just overlap with the context menu.

Not overlapping state:
<img width="350" alt="image"
src="https://github.com/user-attachments/assets/2f1bdc4c-eb01-405c-b5fb-eb28eadc9957"
/>

Overlapping case, moves popover to bottom of context menu:
<img width="350" alt="image"
src="https://github.com/user-attachments/assets/3ce4be23-7701-4711-b604-5e29682360e1"
/>

Overlapping case, moves popover to right of context menu:
<img width="350" alt="image"
src="https://github.com/user-attachments/assets/60d47518-e412-4d64-9d17-a69a17248bdf"
/> <img width="350" alt="image"
src="https://github.com/user-attachments/assets/2a3de176-7443-46d8-99d1-b2973a0ffaa6"
/>

Overlapping case, moves popover to left of context menu:
<img width="350" alt="image"
src="https://github.com/user-attachments/assets/015b799b-8c6e-4405-aee6-e205d4caebec"
/>

Overlapping case, moves popover to top of context menu:
<img width="350" alt="image"
src="https://github.com/user-attachments/assets/fbd03d84-9a49-44eb-846b-a9852d2ff43e"
/>

Release Notes:

- Fixed an issue where hover popovers or signature popovers would
overlap with existing opened completion or code actions context menus.
This commit is contained in:
smit 2025-05-21 18:45:00 +05:30 • committed by GitHub
parent 0c03519393
commit 7450b788f3
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 322 additions and 71 deletions
Download patch file Download diff file Expand all files Collapse all files

355
crates/editor/src/element.rs
View file

@ -3597,7 +3597,7 @@ impl EditorElement {
style: &EditorStyle,
window: &mut Window,
cx: &mut App,
) {
) -> Option<ContextMenuLayout> {
let mut min_menu_height = Pixels::ZERO;
let mut max_menu_height = Pixels::ZERO;
let mut height_above_menu = Pixels::ZERO;
@ -3638,7 +3638,7 @@ impl EditorElement {
let visible = edit_prediction_popover_visible || context_menu_visible;
if !visible {
return;
return None;
}
let cursor_row_layout = &line_layouts[cursor.row().minus(start_row) as usize];
@ -3663,7 +3663,7 @@ impl EditorElement {
let min_height = height_above_menu + min_menu_height + height_below_menu;
let max_height = height_above_menu + max_menu_height + height_below_menu;
let Some((laid_out_popovers, y_flipped)) = self.layout_popovers_above_or_below_line(
let (laid_out_popovers, y_flipped) = self.layout_popovers_above_or_below_line(
target_position,
line_height,
min_height,
@ -3721,16 +3721,11 @@ impl EditorElement {
.flatten()
.collect::<Vec<_>>()
},
) else {
return;
};
)?;
let Some((menu_ix, (_, menu_bounds))) = laid_out_popovers
let (menu_ix, (_, menu_bounds)) = laid_out_popovers
.iter()
.find_position(|(x, _)| matches!(x, CursorPopoverType::CodeContextMenu))
else {
return;
};
.find_position(|(x, _)| matches!(x, CursorPopoverType::CodeContextMenu))?;
let last_ix = laid_out_popovers.len() - 1;
let menu_is_last = menu_ix == last_ix;
let first_popover_bounds = laid_out_popovers[0].1;
@ -3771,7 +3766,7 @@ impl EditorElement {
false
};
self.layout_context_menu_aside(
let aside_bounds = self.layout_context_menu_aside(
y_flipped,
*menu_bounds,
target_bounds,
@ -3783,6 +3778,23 @@ impl EditorElement {
window,
cx,
);
if let Some(menu_bounds) = laid_out_popovers.iter().find_map(|(popover_type, bounds)| {
if matches!(popover_type, CursorPopoverType::CodeContextMenu) {
Some(*bounds)
} else {
None
}
}) {
let bounds = if let Some(aside_bounds) = aside_bounds {
menu_bounds.union(&aside_bounds)
} else {
menu_bounds
};
return Some(ContextMenuLayout { y_flipped, bounds });
}
None
}
fn layout_gutter_menu(
@ -3988,7 +4000,7 @@ impl EditorElement {
viewport_bounds: Bounds<Pixels>,
window: &mut Window,
cx: &mut App,
) {
) -> Option<Bounds<Pixels>> {
let available_within_viewport = target_bounds.space_within(&viewport_bounds);
let positioned_aside = if available_within_viewport.right >= MENU_ASIDE_MIN_WIDTH
&& !must_place_above_or_below
@ -3997,16 +4009,14 @@ impl EditorElement {
available_within_viewport.right - px(1.),
MENU_ASIDE_MAX_WIDTH,
);
let Some(mut aside) = self.render_context_menu_aside(
let mut aside = self.render_context_menu_aside(
size(max_width, max_height - POPOVER_Y_PADDING),
window,
cx,
) else {
return;
};
aside.layout_as_root(AvailableSpace::min_size(), window, cx);
)?;
let size = aside.layout_as_root(AvailableSpace::min_size(), window, cx);
let right_position = point(target_bounds.right(), menu_bounds.origin.y);
Some((aside, right_position))
Some((aside, right_position, size))
} else {
let max_size = size(
// TODO(mgsloan): Once the menu is bounded by viewport width the bound on viewport
@ -4023,9 +4033,7 @@ impl EditorElement {
),
) - POPOVER_Y_PADDING,
);
let Some(mut aside) = self.render_context_menu_aside(max_size, window, cx) else {
return;
};
let mut aside = self.render_context_menu_aside(max_size, window, cx)?;
let actual_size = aside.layout_as_root(AvailableSpace::min_size(), window, cx);
let top_position = point(
@ -4059,13 +4067,17 @@ impl EditorElement {
// Fallback: fit actual size in window.
.or_else(|| fit_within(available_within_viewport, actual_size));
aside_position.map(|position| (aside, position))
aside_position.map(|position| (aside, position, actual_size))
};
// Skip drawing if it doesn't fit anywhere.
if let Some((aside, position)) = positioned_aside {
if let Some((aside, position, size)) = positioned_aside {
let aside_bounds = Bounds::new(position, size);
window.defer_draw(aside, position, 2);
return Some(aside_bounds);
}
None
}
fn render_context_menu(
@ -4174,13 +4186,13 @@ impl EditorElement {
&self,
snapshot: &EditorSnapshot,
hitbox: &Hitbox,
text_hitbox: &Hitbox,
visible_display_row_range: Range<DisplayRow>,
content_origin: gpui::Point<Pixels>,
scroll_pixel_position: gpui::Point<Pixels>,
line_layouts: &[LineWithInvisibles],
line_height: Pixels,
em_width: Pixels,
context_menu_layout: Option<ContextMenuLayout>,
window: &mut Window,
cx: &mut App,
) {
@ -4224,21 +4236,24 @@ impl EditorElement {
let mut overall_height = Pixels::ZERO;
let mut measured_hover_popovers = Vec::new();
for mut hover_popover in hover_popovers {
for (position, mut hover_popover) in hover_popovers.into_iter().with_position() {
let size = hover_popover.layout_as_root(AvailableSpace::min_size(), window, cx);
let horizontal_offset =
(text_hitbox.top_right().x - POPOVER_RIGHT_OFFSET - (hovered_point.x + size.width))
(hitbox.top_right().x - POPOVER_RIGHT_OFFSET - (hovered_point.x + size.width))
.min(Pixels::ZERO);
overall_height += HOVER_POPOVER_GAP + size.height;
match position {
itertools::Position::Middle | itertools::Position::Last => {
overall_height += HOVER_POPOVER_GAP
}
_ => {}
}
overall_height += size.height;
measured_hover_popovers.push(MeasuredHoverPopover {
element: hover_popover,
size,
horizontal_offset,
});
}
overall_height += HOVER_POPOVER_GAP;
fn draw_occluder(
width: Pixels,
@ -4255,8 +4270,12 @@ impl EditorElement {
window.defer_draw(occlusion, origin, 2);
}
if hovered_point.y > overall_height {
// There is enough space above. Render popovers above the hovered point
fn place_popovers_above(
hovered_point: gpui::Point<Pixels>,
measured_hover_popovers: Vec<MeasuredHoverPopover>,
window: &mut Window,
cx: &mut App,
) {
let mut current_y = hovered_point.y;
for (position, popover) in measured_hover_popovers.into_iter().with_position() {
let size = popover.size;
@ -4273,8 +4292,15 @@ impl EditorElement {
current_y = popover_origin.y - HOVER_POPOVER_GAP;
}
} else {
// There is not enough space above. Render popovers below the hovered point
}
fn place_popovers_below(
hovered_point: gpui::Point<Pixels>,
measured_hover_popovers: Vec<MeasuredHoverPopover>,
line_height: Pixels,
window: &mut Window,
cx: &mut App,
) {
let mut current_y = hovered_point.y + line_height;
for (position, popover) in measured_hover_popovers.into_iter().with_position() {
let size = popover.size;
@ -4289,6 +4315,123 @@ impl EditorElement {
current_y = popover_origin.y + size.height + HOVER_POPOVER_GAP;
}
}
let intersects_menu = |bounds: Bounds<Pixels>| -> bool {
context_menu_layout
.as_ref()
.map_or(false, |menu| bounds.intersects(&menu.bounds))
};
let can_place_above = {
let mut bounds_above = Vec::new();
let mut current_y = hovered_point.y;
for popover in &measured_hover_popovers {
let size = popover.size;
let popover_origin = point(
hovered_point.x + popover.horizontal_offset,
current_y - size.height,
);
bounds_above.push(Bounds::new(popover_origin, size));
current_y = popover_origin.y - HOVER_POPOVER_GAP;
}
bounds_above
.iter()
.all(|b| b.is_contained_within(hitbox) && !intersects_menu(*b))
};
let can_place_below = || {
let mut bounds_below = Vec::new();
let mut current_y = hovered_point.y + line_height;
for popover in &measured_hover_popovers {
let size = popover.size;
let popover_origin = point(hovered_point.x + popover.horizontal_offset, current_y);
bounds_below.push(Bounds::new(popover_origin, size));
current_y = popover_origin.y + size.height + HOVER_POPOVER_GAP;
}
bounds_below
.iter()
.all(|b| b.is_contained_within(hitbox) && !intersects_menu(*b))
};
if can_place_above {
// try placing above hovered point
place_popovers_above(hovered_point, measured_hover_popovers, window, cx);
} else if can_place_below() {
// try placing below hovered point
place_popovers_below(
hovered_point,
measured_hover_popovers,
line_height,
window,
cx,
);
} else {
// try to place popovers around the context menu
let origin_surrounding_menu = context_menu_layout.as_ref().and_then(|menu| {
let total_width = measured_hover_popovers
.iter()
.map(|p| p.size.width)
.max()
.unwrap_or(Pixels::ZERO);
let y_for_horizontal_positioning = if menu.y_flipped {
menu.bounds.bottom() - overall_height
} else {
menu.bounds.top()
};
let possible_origins = vec![
// left of context menu
point(
menu.bounds.left() - total_width - HOVER_POPOVER_GAP,
y_for_horizontal_positioning,
),
// right of context menu
point(
menu.bounds.right() + HOVER_POPOVER_GAP,
y_for_horizontal_positioning,
),
// top of context menu
point(
menu.bounds.left(),
menu.bounds.top() - overall_height - HOVER_POPOVER_GAP,
),
// bottom of context menu
point(menu.bounds.left(), menu.bounds.bottom() + HOVER_POPOVER_GAP),
];
possible_origins.into_iter().find(|&origin| {
Bounds::new(origin, size(total_width, overall_height))
.is_contained_within(hitbox)
})
});
if let Some(origin) = origin_surrounding_menu {
let mut current_y = origin.y;
for (position, popover) in measured_hover_popovers.into_iter().with_position() {
let size = popover.size;
let popover_origin = point(origin.x, current_y);
window.defer_draw(popover.element, popover_origin, 2);
if position != itertools::Position::Last {
let origin = point(popover_origin.x, popover_origin.y + size.height);
draw_occluder(size.width, origin, window, cx);
}
current_y = popover_origin.y + size.height + HOVER_POPOVER_GAP;
}
} else {
// fallback to existing above/below cursor logic
// this might overlap menu or overflow in rare case
if can_place_above {
place_popovers_above(hovered_point, measured_hover_popovers, window, cx);
} else {
place_popovers_below(
hovered_point,
measured_hover_popovers,
line_height,
window,
cx,
);
}
}
}
}
fn layout_diff_hunk_controls(
@ -4395,7 +4538,6 @@ impl EditorElement {
fn layout_signature_help(
&self,
hitbox: &Hitbox,
text_hitbox: &Hitbox,
content_origin: gpui::Point<Pixels>,
scroll_pixel_position: gpui::Point<Pixels>,
newest_selection_head: Option<DisplayPoint>,
@ -4403,6 +4545,7 @@ impl EditorElement {
line_layouts: &[LineWithInvisibles],
line_height: Pixels,
em_width: Pixels,
context_menu_layout: Option<ContextMenuLayout>,
window: &mut Window,
cx: &mut App,
) {
@ -4448,22 +4591,82 @@ impl EditorElement {
let target_point = content_origin + point(target_x, target_y);
let actual_size = element.layout_as_root(Size::<AvailableSpace>::default(), window, cx);
let overall_height = actual_size.height + HOVER_POPOVER_GAP;
let popover_origin = if target_point.y > overall_height {
point(target_point.x, target_point.y - actual_size.height)
} else {
point(
target_point.x,
target_point.y + line_height + HOVER_POPOVER_GAP,
let (popover_bounds_above, popover_bounds_below) = {
let horizontal_offset = (hitbox.top_right().x
- POPOVER_RIGHT_OFFSET
- (target_point.x + actual_size.width))
.min(Pixels::ZERO);
let initial_x = target_point.x + horizontal_offset;
(
Bounds::new(
point(initial_x, target_point.y - actual_size.height),
actual_size,
),
Bounds::new(
point(initial_x, target_point.y + line_height + HOVER_POPOVER_GAP),
actual_size,
),
)
};
let horizontal_offset = (text_hitbox.top_right().x
- POPOVER_RIGHT_OFFSET
- (popover_origin.x + actual_size.width))
.min(Pixels::ZERO);
let final_origin = point(popover_origin.x + horizontal_offset, popover_origin.y);
let intersects_menu = |bounds: Bounds<Pixels>| -> bool {
context_menu_layout
.as_ref()
.map_or(false, |menu| bounds.intersects(&menu.bounds))
};
let final_origin = if popover_bounds_above.is_contained_within(hitbox)
&& !intersects_menu(popover_bounds_above)
{
// try placing above cursor
popover_bounds_above.origin
} else if popover_bounds_below.is_contained_within(hitbox)
&& !intersects_menu(popover_bounds_below)
{
// try placing below cursor
popover_bounds_below.origin
} else {
// try surrounding context menu if exists
let origin_surrounding_menu = context_menu_layout.as_ref().and_then(|menu| {
let y_for_horizontal_positioning = if menu.y_flipped {
menu.bounds.bottom() - actual_size.height
} else {
menu.bounds.top()
};
let possible_origins = vec![
// left of context menu
point(
menu.bounds.left() - actual_size.width - HOVER_POPOVER_GAP,
y_for_horizontal_positioning,
),
// right of context menu
point(
menu.bounds.right() + HOVER_POPOVER_GAP,
y_for_horizontal_positioning,
),
// top of context menu
point(
menu.bounds.left(),
menu.bounds.top() - actual_size.height - HOVER_POPOVER_GAP,
),
// bottom of context menu
point(menu.bounds.left(), menu.bounds.bottom() + HOVER_POPOVER_GAP),
];
possible_origins
.into_iter()
.find(|&origin| Bounds::new(origin, actual_size).is_contained_within(hitbox))
});
origin_surrounding_menu.unwrap_or_else(|| {
// fallback to existing above/below cursor logic
// this might overlap menu or overflow in rare case
if popover_bounds_above.is_contained_within(hitbox) {
popover_bounds_above.origin
} else {
popover_bounds_below.origin
}
})
};
window.defer_draw(element, final_origin, 2);
}
@ -7884,27 +8087,31 @@ impl Element for EditorElement {
let gutter_settings = EditorSettings::get_global(cx).gutter;
if let Some(newest_selection_head) = newest_selection_head {
let newest_selection_point =
newest_selection_head.to_point(&snapshot.display_snapshot);
if (start_row..end_row).contains(&newest_selection_head.row()) {
self.layout_cursor_popovers(
line_height,
&text_hitbox,
content_origin,
right_margin,
start_row,
scroll_pixel_position,
&line_layouts,
newest_selection_head,
newest_selection_point,
&style,
window,
cx,
);
}
}
let context_menu_layout =
if let Some(newest_selection_head) = newest_selection_head {
let newest_selection_point =
newest_selection_head.to_point(&snapshot.display_snapshot);
if (start_row..end_row).contains(&newest_selection_head.row()) {
self.layout_cursor_popovers(
line_height,
&text_hitbox,
content_origin,
right_margin,
start_row,
scroll_pixel_position,
&line_layouts,
newest_selection_head,
newest_selection_point,
&style,
window,
cx,
)
} else {
None
}
} else {
None
};
self.layout_gutter_menu(
line_height,
@ -7958,7 +8165,6 @@ impl Element for EditorElement {
self.layout_signature_help(
&hitbox,
&text_hitbox,
content_origin,
scroll_pixel_position,
newest_selection_head,
@ -7966,6 +8172,7 @@ impl Element for EditorElement {
&line_layouts,
line_height,
em_width,
context_menu_layout,
window,
cx,
);
@ -7974,13 +8181,13 @@ impl Element for EditorElement {
self.layout_hover_popovers(
&snapshot,
&hitbox,
&text_hitbox,
start_row..end_row,
content_origin,
scroll_pixel_position,
&line_layouts,
line_height,
em_width,
context_menu_layout,
window,
cx,
);
@ -8212,6 +8419,12 @@ pub(super) fn gutter_bounds(
}
}
#[derive(Clone, Copy)]
struct ContextMenuLayout {
y_flipped: bool,
bounds: Bounds<Pixels>,
}
/// Holds information required for layouting the editor scrollbars.
struct ScrollbarLayoutInformation {
/// The bounds of the editor area (excluding the content offset).

38
crates/gpui/src/geometry.rs
View file

@ -1388,6 +1388,44 @@ where
&& point.y <= self.origin.y.clone() + self.size.height.clone()
}
/// Checks if this bounds is completely contained within another bounds.
///
/// This method determines whether the current bounds is entirely enclosed by the given bounds.
/// A bounds is considered to be contained within another if its origin (top-left corner) and
/// its bottom-right corner are both contained within the other bounds.
///
/// # Arguments
///
/// * `other` - A reference to another `Bounds` that might contain this bounds.
///
/// # Returns
///
/// Returns `true` if this bounds is completely inside the other bounds, `false` otherwise.
///
/// # Examples
///
/// ```
/// # use gpui::{Bounds, Point, Size};
/// let outer_bounds = Bounds {
/// origin: Point { x: 0, y: 0 },
/// size: Size { width: 20, height: 20 },
/// };
/// let inner_bounds = Bounds {
/// origin: Point { x: 5, y: 5 },
/// size: Size { width: 10, height: 10 },
/// };
/// let overlapping_bounds = Bounds {
/// origin: Point { x: 15, y: 15 },
/// size: Size { width: 10, height: 10 },
/// };
///
/// assert!(inner_bounds.is_contained_within(&outer_bounds));
/// assert!(!overlapping_bounds.is_contained_within(&outer_bounds));
/// ```
pub fn is_contained_within(&self, other: &Self) -> bool {
other.contains(&self.origin) && other.contains(&self.bottom_right())
}
/// Applies a function to the origin and size of the bounds, producing a new `Bounds<U>`.
///
/// This method allows for converting a `Bounds<T>` to a `Bounds<U>` by specifying a closure