Vasyl Protsiv created
This PR introduces `Nearest` scrolling strategy to `UniformList`. This
is now used in completions menu and the picker to choose the appropriate
scrolling strategy depending on movement direction. Previously,
selecting the next element after the last visible item caused the menu
to scroll with `ScrollStrategy::Top`, which scrolled the whole page and
placed the next element at the top. This behavior is inconsistent,
because using `ScrollStrategy::Top` when moving up only scrolls one
element, not the whole page.
https://github.com/user-attachments/assets/ccfb238f-8f76-4a18-a18d-bbcb63340c5a
The solution is to introduce the `Nearest` scrolling strategy which will
internally choose the scrolling strategy depending on whether the new
selected item is below or above currently visible items. This ensures a
single-item scroll regardless of movement direction.
https://github.com/user-attachments/assets/8502efb8-e2c0-4ab1-bd8d-93103841a9c4
I also noticed that some functions in the file have different logic
depending on `y_flipped`. This appears related to reversing the order of
elements in the list when the completion menu appears above the cursor.
This was a feature suggested in #11200 and implemented in #23446. It
looks like this feature was reverted in #27765 and there currently seem
to be no way to have `y_flipped` to be set to `true`.
My understanding is that the opposite scroll strategy should be used if
`y_flipped`, but since there is no way to enable this feature to test it
and I don't know if the feature is ever going to be reintroduced I
decided not to include it in this PR.
Release Notes:
- gpui: Add 'Nearest' scrolling strategy to 'UniformList'crates/editor/src/code_context_menus.rs | 2
crates/gpui/src/elements/uniform_list.rs | 219 +++++++++++++++++++++----
crates/picker/src/picker.rs | 2
3 files changed, 187 insertions(+), 36 deletions(-)
@@ -506,7 +506,7 @@ impl CompletionsMenu {
cx: &mut Context<Editor>,
) {
self.scroll_handle
- .scroll_to_item(self.selected_item, ScrollStrategy::Top);
+ .scroll_to_item(self.selected_item, ScrollStrategy::Nearest);
if let Some(provider) = provider {
let entries = self.entries.borrow();
let entry = if self.selected_item < entries.len() {
@@ -92,6 +92,10 @@ pub enum ScrollStrategy {
/// May not be possible if there's not enough list items above the item scrolled to:
/// in this case, the element will be placed at the closest possible position.
Bottom,
+ /// If the element is not visible attempt to place it at:
+ /// - The top of the list's viewport if the target element is above currently visible elements.
+ /// - The bottom of the list's viewport if the target element is above currently visible elements.
+ Nearest,
}
#[derive(Clone, Copy, Debug)]
@@ -391,39 +395,42 @@ impl Element for UniformList {
scroll_offset.x = Pixels::ZERO;
}
- if let Some(deferred_scroll) = shared_scroll_to_item {
- let mut ix = deferred_scroll.item_index;
+ if let Some(DeferredScrollToItem {
+ mut item_index,
+ mut strategy,
+ offset,
+ scroll_strict,
+ }) = shared_scroll_to_item
+ {
if y_flipped {
- ix = self.item_count.saturating_sub(ix + 1);
+ item_index = self.item_count.saturating_sub(item_index + 1);
}
let list_height = padded_bounds.size.height;
let mut updated_scroll_offset = shared_scroll_offset.borrow_mut();
- let item_top = item_height * ix;
+ let item_top = item_height * item_index;
let item_bottom = item_top + item_height;
let scroll_top = -updated_scroll_offset.y;
- let offset_pixels = item_height * deferred_scroll.offset;
- let mut scrolled_to_top = false;
-
- if item_top < scroll_top + offset_pixels {
- scrolled_to_top = true;
- // todo: using the padding here is wrong - this only works well for few scenarios
- updated_scroll_offset.y = -item_top + padding.top + offset_pixels;
- } else if item_bottom > scroll_top + list_height {
- scrolled_to_top = true;
- updated_scroll_offset.y = -(item_bottom - list_height);
- }
+ let offset_pixels = item_height * offset;
+
+ // is the selected item above/below currently visible items
+ let is_above = item_top < scroll_top + offset_pixels;
+ let is_below = item_bottom > scroll_top + list_height;
+
+ if scroll_strict || is_above || is_below {
+ if strategy == ScrollStrategy::Nearest {
+ if is_above {
+ strategy = ScrollStrategy::Top;
+ } else if is_below {
+ strategy = ScrollStrategy::Bottom;
+ }
+ }
- if deferred_scroll.scroll_strict
- || (scrolled_to_top
- && (item_top < scroll_top + offset_pixels
- || item_bottom > scroll_top + list_height))
- {
- match deferred_scroll.strategy {
+ let max_scroll_offset =
+ (content_height - list_height).max(Pixels::ZERO);
+ match strategy {
ScrollStrategy::Top => {
updated_scroll_offset.y = -(item_top - offset_pixels)
- .max(Pixels::ZERO)
- .min(content_height - list_height)
- .max(Pixels::ZERO);
+ .clamp(Pixels::ZERO, max_scroll_offset);
}
ScrollStrategy::Center => {
let item_center = item_top + item_height / 2.0;
@@ -431,18 +438,15 @@ impl Element for UniformList {
let viewport_height = list_height - offset_pixels;
let viewport_center = offset_pixels + viewport_height / 2.0;
let target_scroll_top = item_center - viewport_center;
-
- updated_scroll_offset.y = -target_scroll_top
- .max(Pixels::ZERO)
- .min(content_height - list_height)
- .max(Pixels::ZERO);
+ updated_scroll_offset.y =
+ -target_scroll_top.clamp(Pixels::ZERO, max_scroll_offset);
}
ScrollStrategy::Bottom => {
- updated_scroll_offset.y = -(item_bottom - list_height
- + offset_pixels)
- .max(Pixels::ZERO)
- .min(content_height - list_height)
- .max(Pixels::ZERO);
+ updated_scroll_offset.y = -(item_bottom - list_height)
+ .clamp(Pixels::ZERO, max_scroll_offset);
+ }
+ ScrollStrategy::Nearest => {
+ // Nearest, but the item is visible -> no scroll is required
}
}
}
@@ -695,3 +699,150 @@ impl InteractiveElement for UniformList {
&mut self.interactivity
}
}
+
+#[cfg(test)]
+mod test {
+ use crate::TestAppContext;
+
+ #[gpui::test]
+ fn test_scroll_strategy_nearest(cx: &mut TestAppContext) {
+ use crate::{
+ Context, FocusHandle, ScrollStrategy, UniformListScrollHandle, Window, actions, div,
+ prelude::*, px, uniform_list,
+ };
+ use std::ops::Range;
+
+ actions!(example, [SelectNext, SelectPrev]);
+
+ struct TestView {
+ index: usize,
+ length: usize,
+ scroll_handle: UniformListScrollHandle,
+ focus_handle: FocusHandle,
+ visible_range: Range<usize>,
+ }
+
+ impl TestView {
+ pub fn select_next(
+ &mut self,
+ _: &SelectNext,
+ window: &mut Window,
+ _: &mut Context<Self>,
+ ) {
+ if self.index + 1 == self.length {
+ self.index = 0
+ } else {
+ self.index += 1;
+ }
+ self.scroll_handle
+ .scroll_to_item(self.index, ScrollStrategy::Nearest);
+ window.refresh();
+ }
+
+ pub fn select_previous(
+ &mut self,
+ _: &SelectPrev,
+ window: &mut Window,
+ _: &mut Context<Self>,
+ ) {
+ if self.index == 0 {
+ self.index = self.length - 1
+ } else {
+ self.index -= 1;
+ }
+ self.scroll_handle
+ .scroll_to_item(self.index, ScrollStrategy::Nearest);
+ window.refresh();
+ }
+ }
+
+ impl Render for TestView {
+ fn render(&mut self, _window: &mut Window, cx: &mut Context<Self>) -> impl IntoElement {
+ div()
+ .id("list-example")
+ .track_focus(&self.focus_handle)
+ .on_action(cx.listener(Self::select_next))
+ .on_action(cx.listener(Self::select_previous))
+ .size_full()
+ .child(
+ uniform_list(
+ "entries",
+ self.length,
+ cx.processor(|this, range: Range<usize>, _window, _cx| {
+ this.visible_range = range.clone();
+ range
+ .map(|ix| div().id(ix).h(px(20.0)).child(format!("Item {ix}")))
+ .collect()
+ }),
+ )
+ .track_scroll(self.scroll_handle.clone())
+ .h(px(200.0)),
+ )
+ }
+ }
+
+ let (view, cx) = cx.add_window_view(|window, cx| {
+ let focus_handle = cx.focus_handle();
+ window.focus(&focus_handle);
+ TestView {
+ scroll_handle: UniformListScrollHandle::new(),
+ index: 0,
+ focus_handle,
+ length: 47,
+ visible_range: 0..0,
+ }
+ });
+
+ // 10 out of 47 items are visible
+
+ // First 9 times selecting next item does not scroll
+ for ix in 1..10 {
+ cx.dispatch_action(SelectNext);
+ view.read_with(cx, |view, _| {
+ assert_eq!(view.index, ix);
+ assert_eq!(view.visible_range, 0..10);
+ })
+ }
+
+ // Now each time the list scrolls down by 1
+ for ix in 10..47 {
+ cx.dispatch_action(SelectNext);
+ view.read_with(cx, |view, _| {
+ assert_eq!(view.index, ix);
+ assert_eq!(view.visible_range, ix - 9..ix + 1);
+ })
+ }
+
+ // After the last item we move back to the start
+ cx.dispatch_action(SelectNext);
+ view.read_with(cx, |view, _| {
+ assert_eq!(view.index, 0);
+ assert_eq!(view.visible_range, 0..10);
+ });
+
+ // Return to the last element
+ cx.dispatch_action(SelectPrev);
+ view.read_with(cx, |view, _| {
+ assert_eq!(view.index, 46);
+ assert_eq!(view.visible_range, 37..47);
+ });
+
+ // First 9 times selecting previous does not scroll
+ for ix in (37..46).rev() {
+ cx.dispatch_action(SelectPrev);
+ view.read_with(cx, |view, _| {
+ assert_eq!(view.index, ix);
+ assert_eq!(view.visible_range, 37..47);
+ })
+ }
+
+ // Now each time the list scrolls up by 1
+ for ix in (0..37).rev() {
+ cx.dispatch_action(SelectPrev);
+ view.read_with(cx, |view, _| {
+ assert_eq!(view.index, ix);
+ assert_eq!(view.visible_range, ix..ix + 10);
+ })
+ }
+ }
+}
@@ -709,7 +709,7 @@ impl<D: PickerDelegate> Picker<D> {
match &mut self.element_container {
ElementContainer::List(state) => state.scroll_to_reveal_item(ix),
ElementContainer::UniformList(scroll_handle) => {
- scroll_handle.scroll_to_item(ix, ScrollStrategy::Top)
+ scroll_handle.scroll_to_item(ix, ScrollStrategy::Nearest)
}
}
}