From 2f1af2ab694a9a273605b8aeaf5e8f5a816628a1 Mon Sep 17 00:00:00 2001 From: Michael Sloan Date: Tue, 21 Jan 2025 22:47:56 -0700 Subject: [PATCH] When completions menu is displayed above cursor, reverse order (#23446) Considered doing this when previously working on completions menu layout, as it brings the default selection position next to the cursor position, and is generally more symmetrical. With #23445 there is now a more compelling reason, as the "translucent, cropped bottom" display doesn't make sense when displayed above. Release Notes: - N/A --- crates/editor/src/code_context_menus.rs | 103 ++++++++++++++++++----- crates/editor/src/editor.rs | 3 +- crates/editor/src/element.rs | 32 +++---- crates/gpui/src/elements/uniform_list.rs | 57 +++++++++++-- crates/gpui/src/geometry.rs | 2 + 5 files changed, 152 insertions(+), 45 deletions(-) diff --git a/crates/editor/src/code_context_menus.rs b/crates/editor/src/code_context_menus.rs index 7feda719ea204857182de1779f3a51bc4f4d59ab..ed84a6cf56e67e3baee9c7d7e029c9cafc71eb1b 100644 --- a/crates/editor/src/code_context_menus.rs +++ b/crates/editor/src/code_context_menus.rs @@ -125,11 +125,16 @@ impl CodeContextMenu { &self, style: &EditorStyle, max_height_in_lines: u32, + y_flipped: bool, 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), + CodeContextMenu::Completions(menu) => { + menu.render(style, max_height_in_lines, y_flipped, cx) + } + CodeContextMenu::CodeActions(menu) => { + menu.render(style, max_height_in_lines, y_flipped, cx) + } } } @@ -268,31 +273,50 @@ impl CompletionsMenu { provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - self.update_selection_index(0, provider, cx); + let index = if self.scroll_handle.y_flipped() { + self.entries.borrow().len() - 1 + } else { + 0 + }; + self.update_selection_index(index, provider, cx); } - fn select_prev( + fn select_last( &mut self, provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - self.update_selection_index(self.prev_match_index(), provider, cx); + let index = if self.scroll_handle.y_flipped() { + 0 + } else { + self.entries.borrow().len() - 1 + }; + self.update_selection_index(index, provider, cx); } - fn select_next( + fn select_prev( &mut self, provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - self.update_selection_index(self.next_match_index(), provider, cx); + let index = if self.scroll_handle.y_flipped() { + self.next_match_index() + } else { + self.prev_match_index() + }; + self.update_selection_index(index, provider, cx); } - fn select_last( + fn select_next( &mut self, provider: Option<&dyn CompletionProvider>, cx: &mut ViewContext, ) { - let index = self.entries.borrow().len() - 1; + let index = if self.scroll_handle.y_flipped() { + self.prev_match_index() + } else { + self.next_match_index() + }; self.update_selection_index(index, provider, cx); } @@ -336,6 +360,11 @@ impl CompletionsMenu { } _ => { entries.insert(0, hint); + // When `y_flipped`, need to scroll to bring it into view. + if self.selected_item == 0 { + self.scroll_handle + .scroll_to_item(self.selected_item, ScrollStrategy::Top); + } } } } @@ -439,6 +468,7 @@ impl CompletionsMenu { &self, style: &EditorStyle, max_height_in_lines: u32, + y_flipped: bool, cx: &mut ViewContext, ) -> AnyElement { let completions = self.completions.borrow_mut(); @@ -664,6 +694,7 @@ impl CompletionsMenu { .occlude() .max_h(max_height_in_lines as f32 * cx.line_height()) .track_scroll(self.scroll_handle.clone()) + .y_flipped(y_flipped) .with_width_from_item(widest_completion_ix) .with_sizing_behavior(ListSizingBehavior::Infer); @@ -981,39 +1012,63 @@ pub struct CodeActionsMenu { impl CodeActionsMenu { fn select_first(&mut self, cx: &mut ViewContext) { - self.selected_item = 0; + self.selected_item = if self.scroll_handle.y_flipped() { + self.actions.len() - 1 + } else { + 0 + }; + self.scroll_handle + .scroll_to_item(self.selected_item, ScrollStrategy::Top); + cx.notify() + } + + fn select_last(&mut self, cx: &mut ViewContext) { + self.selected_item = if self.scroll_handle.y_flipped() { + 0 + } else { + self.actions.len() - 1 + }; self.scroll_handle .scroll_to_item(self.selected_item, ScrollStrategy::Top); cx.notify() } fn select_prev(&mut self, cx: &mut ViewContext) { - if self.selected_item > 0 { - self.selected_item -= 1; + self.selected_item = if self.scroll_handle.y_flipped() { + self.next_match_index() } else { - self.selected_item = self.actions.len() - 1; - } + self.prev_match_index() + }; self.scroll_handle .scroll_to_item(self.selected_item, ScrollStrategy::Top); cx.notify(); } fn select_next(&mut self, cx: &mut ViewContext) { - if self.selected_item + 1 < self.actions.len() { - self.selected_item += 1; + self.selected_item = if self.scroll_handle.y_flipped() { + self.prev_match_index() } else { - self.selected_item = 0; - } + self.next_match_index() + }; self.scroll_handle .scroll_to_item(self.selected_item, ScrollStrategy::Top); cx.notify(); } - fn select_last(&mut self, cx: &mut ViewContext) { - self.selected_item = self.actions.len() - 1; - self.scroll_handle - .scroll_to_item(self.selected_item, ScrollStrategy::Top); - cx.notify() + fn prev_match_index(&self) -> usize { + if self.selected_item > 0 { + self.selected_item - 1 + } else { + self.actions.len() - 1 + } + } + + fn next_match_index(&self) -> usize { + if self.selected_item + 1 < self.actions.len() { + self.selected_item + 1 + } else { + 0 + } } fn visible(&self) -> bool { @@ -1032,6 +1087,7 @@ impl CodeActionsMenu { &self, _style: &EditorStyle, max_height_in_lines: u32, + y_flipped: bool, cx: &mut ViewContext, ) -> AnyElement { let actions = self.actions.clone(); @@ -1107,6 +1163,7 @@ impl CodeActionsMenu { .occlude() .max_h(max_height_in_lines as f32 * cx.line_height()) .track_scroll(self.scroll_handle.clone()) + .y_flipped(y_flipped) .with_width_from_item( self.actions .iter() diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index fbc74b3d0607043373cde8981c53d6d07562b8f5..923e61754ca70e7f43dab2dac13f5ea5c84743a6 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -5246,11 +5246,12 @@ impl Editor { &self, style: &EditorStyle, max_height_in_lines: u32, + y_flipped: bool, cx: &mut ViewContext, ) -> Option { self.context_menu.borrow().as_ref().and_then(|menu| { if menu.visible() { - Some(menu.render(style, max_height_in_lines, cx)) + Some(menu.render(style, max_height_in_lines, y_flipped, cx)) } else { None } diff --git a/crates/editor/src/element.rs b/crates/editor/src/element.rs index e9485aa447f7f581a42b57da1f5cccc3b811ac9e..e8a7098cd4ef496e146f8f43ccb7bb3821755fc4 100644 --- a/crates/editor/src/element.rs +++ b/crates/editor/src/element.rs @@ -3136,10 +3136,10 @@ impl EditorElement { 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 y_flipped = y_overflows_below && available_above > available_below; let mut height = cmp::min( unconstrained_max_height, - if y_is_flipped { + if y_flipped { available_above } else { available_below @@ -3151,16 +3151,16 @@ impl EditorElement { let available_above = bottom_y_when_flipped; let available_below = viewport_bounds.bottom() - target_position.y; if available_below > 3. * line_height { - y_is_flipped = false; + y_flipped = false; height = min_height; } else if available_above > 3. * line_height { - y_is_flipped = true; + y_flipped = true; height = min_height; } else if available_above > available_below { - y_is_flipped = true; + y_flipped = true; height = available_above; } else { - y_is_flipped = false; + y_flipped = false; height = available_below; } } @@ -3169,7 +3169,7 @@ impl EditorElement { // TODO(mgsloan): use viewport_bounds.width as a max width when rendering menu. let Some(mut menu_element) = self.editor.update(cx, |editor, cx| { - editor.render_context_menu(&self.style, max_height_in_lines, cx) + editor.render_context_menu(&self.style, max_height_in_lines, y_flipped, cx) }) else { return; }; @@ -3181,7 +3181,7 @@ impl EditorElement { x: target_position .x .min((viewport_bounds.right() - menu_size.width).max(Pixels::ZERO)), - y: if y_is_flipped { + y: if y_flipped { bottom_y_when_flipped - menu_size.height } else { target_position.y @@ -3192,7 +3192,7 @@ impl EditorElement { // Layout documentation aside 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 { + let max_menu_bounds = if y_flipped { Bounds::new( point( menu_position.x, @@ -3205,7 +3205,7 @@ impl EditorElement { }; self.layout_context_menu_aside( text_hitbox, - y_is_flipped, + y_flipped, menu_position, menu_bounds, max_menu_bounds, @@ -3220,7 +3220,7 @@ impl EditorElement { fn layout_context_menu_aside( &self, text_hitbox: &Hitbox, - y_is_flipped: bool, + y_flipped: bool, menu_position: gpui::Point, menu_bounds: Bounds, max_menu_bounds: Bounds, @@ -3231,7 +3231,7 @@ impl EditorElement { ) { let mut extend_amount = Edges::all(MENU_GAP); // Extend to include the cursored line to avoid overlapping it. - if y_is_flipped { + if y_flipped { extend_amount.bottom = line_height; } else { extend_amount.top = line_height; @@ -3280,13 +3280,13 @@ impl EditorElement { let fit_within = |available: Edges, wanted: Size| { // Prefer to fit on the same side of the line as the menu, then on the other side of // the line. - if !y_is_flipped && wanted.height < available.bottom { + if !y_flipped && wanted.height < available.bottom { Some(bottom_position) - } else if !y_is_flipped && wanted.height < available.top { + } else if !y_flipped && wanted.height < available.top { Some(top_position) - } else if y_is_flipped && wanted.height < available.top { + } else if y_flipped && wanted.height < available.top { Some(top_position) - } else if y_is_flipped && wanted.height < available.bottom { + } else if y_flipped && wanted.height < available.bottom { Some(bottom_position) } else { None diff --git a/crates/gpui/src/elements/uniform_list.rs b/crates/gpui/src/elements/uniform_list.rs index 4c2b449b41090174b68c458a2fdff8e8864b6619..a95ac8f8475a550c3932a22e2cbe5841127bff80 100644 --- a/crates/gpui/src/elements/uniform_list.rs +++ b/crates/gpui/src/elements/uniform_list.rs @@ -106,6 +106,8 @@ pub struct UniformListScrollState { pub deferred_scroll_to_item: Option<(usize, ScrollStrategy)>, /// Size of the item, captured during last layout. pub last_item_size: Option, + /// Whether the list was vertically flipped during last layout. + pub y_flipped: bool, } #[derive(Copy, Clone, Debug, Default)] @@ -125,6 +127,7 @@ impl UniformListScrollHandle { base_handle: ScrollHandle::new(), deferred_scroll_to_item: None, last_item_size: None, + y_flipped: false, }))) } @@ -133,6 +136,11 @@ impl UniformListScrollHandle { self.0.borrow_mut().deferred_scroll_to_item = Some((ix, strategy)); } + /// Check if the list is flipped vertically. + pub fn y_flipped(&self) -> bool { + self.0.borrow().y_flipped + } + /// Get the index of the topmost visible child. #[cfg(any(test, feature = "test-support"))] pub fn logical_scroll_top_index(&self) -> usize { @@ -264,9 +272,13 @@ impl Element for UniformList { bounds.bottom_right() - point(border.right + padding.right, border.bottom), ); - if let Some(handle) = self.scroll_handle.as_mut() { - handle.0.borrow_mut().base_handle.set_bounds(bounds); - } + let y_flipped = if let Some(scroll_handle) = self.scroll_handle.as_mut() { + let mut scroll_state = scroll_handle.0.borrow_mut(); + scroll_state.base_handle.set_bounds(bounds); + scroll_state.y_flipped + } else { + false + }; if self.item_count > 0 { let content_height = @@ -286,7 +298,10 @@ impl Element for UniformList { scroll_offset.x = Pixels::ZERO; } - if let Some((ix, scroll_strategy)) = shared_scroll_to_item { + if let Some((mut ix, scroll_strategy)) = shared_scroll_to_item { + if y_flipped { + ix = self.item_count.saturating_sub(ix + 1); + } let list_height = padded_bounds.size.height; let mut updated_scroll_offset = shared_scroll_offset.borrow_mut(); let item_top = item_height * ix + padding.top; @@ -330,7 +345,15 @@ impl Element for UniformList { let visible_range = first_visible_element_ix ..cmp::min(last_visible_element_ix, self.item_count); - let mut items = (self.render_items)(visible_range.clone(), cx); + let items = if y_flipped { + let flipped_range = self.item_count.saturating_sub(visible_range.end) + ..self.item_count.saturating_sub(visible_range.start); + let mut items = (self.render_items)(flipped_range, cx); + items.reverse(); + items + } else { + (self.render_items)(visible_range.clone(), cx) + }; let content_mask = ContentMask { bounds }; cx.with_content_mask(Some(content_mask), |cx| { @@ -500,6 +523,30 @@ impl UniformList { self.scroll_handle = Some(handle); self } + + /// Sets whether the list is flipped vertically, such that item 0 appears at the bottom. + pub fn y_flipped(mut self, y_flipped: bool) -> Self { + if let Some(ref scroll_handle) = self.scroll_handle { + let mut scroll_state = scroll_handle.0.borrow_mut(); + let mut base_handle = &scroll_state.base_handle; + let offset = base_handle.offset(); + match scroll_state.last_item_size { + Some(last_size) if scroll_state.y_flipped != y_flipped => { + let new_y_offset = + -(offset.y + last_size.contents.height - last_size.item.height); + base_handle.set_offset(point(offset.x, new_y_offset)); + scroll_state.y_flipped = y_flipped; + } + // Handle case where list is initially flipped. + None if y_flipped => { + base_handle.set_offset(point(offset.x, Pixels::MIN)); + scroll_state.y_flipped = y_flipped; + } + _ => {} + } + } + self + } } impl InteractiveElement for UniformList { diff --git a/crates/gpui/src/geometry.rs b/crates/gpui/src/geometry.rs index 8d726f6d2891afe05a96748d0f076d21e916f5d6..b461b7f28d2acdd5d467fd3847e8310e49c64bce 100644 --- a/crates/gpui/src/geometry.rs +++ b/crates/gpui/src/geometry.rs @@ -2500,6 +2500,8 @@ impl Pixels { pub const ZERO: Pixels = Pixels(0.0); /// The maximum value that can be represented by `Pixels`. pub const MAX: Pixels = Pixels(f32::MAX); + /// The minimum value that can be represented by `Pixels`. + pub const MIN: Pixels = Pixels(f32::MIN); /// Floors the `Pixels` value to the nearest whole number. ///