Dino and Conrad Irwin created
This Pull Request introduces various changes to the editor's horizontal
scrolling, mostly focused on vim mode's horizontal scroll motions (`z
l`, `z h`, `z shift-l`, `z shift-h`). In order to make it easier to
review, the logical changes have been split into different sections.
## Cursor Position Update
Changes introduced on https://github.com/zed-industries/zed/pull/32558
added both `z l` and `z h` to vim mode but it only scrolled the editor's
content, without changing the cursor position. This doesn't reflect the
actual behavior of those motions in vim, so these two commits tackled
that, ensuring that the cursor position is updated, only when the cursor
is on the left or right edges of the editor:
-
https://github.com/zed-industries/zed/commit/ea3b866a763ba0bcfc12999ee1741c6528c895b7
-
https://github.com/zed-industries/zed/commit/805f41a913c6e86ef8be550d363a3cc2caeccbe9
## Horizontal Autoscroll Fix
After introducing the cursor position update to both `z l` and `z h` it
was noted that there was a bug with using `z l`, followed by `0` and
then `z l` again, as on the second use `z l` the cursor would not be
updated. This would only happen on the first line in the editor, and it
was concluded that it was because the
`editor::scroll::autoscroll::Editor.autoscroll_horizontally` method was
directly updating the scroll manager's anchor offset, instead of using
the `editor::scroll::Editor.set_scroll_position_internal` method, like
is being done by the vertical autoscroll
(`editor::scroll::autoscroll::Editor.autoscroll_vertically`).
This wouldn't update the scroll manager's anchor, which would still
think it was at `(0, 1)` so the cursor position would not be updated.
The changes in [this
commit](https://github.com/zed-industries/zed/commit/3957f02e189018ef559cd28516f0b872026b0ce1)
updated the horizontal autoscrolling method to also leverage
`set_scroll_position_internal`.
## Visible Column Count & Page Width Scroll Amount
The changes in
https://github.com/zed-industries/zed/commit/d83652c3ae1d0356fd7dca7b8922a0116de39ff0
add a `visible_column_count` field to `editor::scroll::ScrollManager`
struct, which allowed the introduction of the `ScrollAmount::PageWidth`
enum.
With these changes, two new actions are introduced,
`vim::normal::scroll::HalfPageRight` and
`vim::normal::scroll::HalfPageLeft` (in
https://github.com/zed-industries/zed/commit/7f344304d56337654a34b1b461a1dc69defd2e4e),
which move the editor half page to the right and half page to the left,
as well as the cursor position, which have also been mapped to `z
shift-l` and `z shift-h`, respectively.
Closes #17219
Release Notes:
- Improved `z l` and `z h` to actually move the cursor position, similar
to vim's behavior
- Added `z shift-l` and `z shift-h` to scroll half of the page width's
to the right or to the left, respectively
---------
Co-authored-by: Conrad Irwin <conrad.irwin@gmail.com>assets/keymaps/vim.json | 2
crates/editor/src/element.rs | 3
crates/editor/src/scroll.rs | 82 ++++++++++++-
crates/editor/src/scroll/autoscroll.rs | 21 ++-
crates/editor/src/scroll/scroll_amount.rs | 7 +
crates/vim/src/normal/scroll.rs | 105 ++++++++++++++++-
crates/vim/test_data/test_horizontal_scroll.json | 16 ++
7 files changed, 209 insertions(+), 27 deletions(-)
@@ -189,6 +189,8 @@
"z shift-r": "editor::UnfoldAll",
"z l": "vim::ColumnRight",
"z h": "vim::ColumnLeft",
+ "z shift-l": "vim::HalfPageRight",
+ "z shift-h": "vim::HalfPageLeft",
"shift-z shift-q": ["pane::CloseActiveItem", { "save_intent": "skip" }],
"shift-z shift-z": ["pane::CloseActiveItem", { "save_intent": "save_all" }],
// Count support
@@ -7944,6 +7944,7 @@ impl Element for EditorElement {
editor.last_bounds = Some(bounds);
editor.gutter_dimensions = gutter_dimensions;
editor.set_visible_line_count(bounds.size.height / line_height, window, cx);
+ editor.set_visible_column_count(editor_content_width / em_advance);
if matches!(
editor.mode,
@@ -8449,6 +8450,7 @@ impl Element for EditorElement {
scroll_width,
em_advance,
&line_layouts,
+ window,
cx,
)
} else {
@@ -8603,6 +8605,7 @@ impl Element for EditorElement {
scroll_width,
em_advance,
&line_layouts,
+ window,
cx,
)
} else {
@@ -13,6 +13,7 @@ use crate::{
pub use autoscroll::{Autoscroll, AutoscrollStrategy};
use core::fmt::Debug;
use gpui::{App, Axis, Context, Global, Pixels, Task, Window, point, px};
+use language::language_settings::{AllLanguageSettings, SoftWrap};
use language::{Bias, Point};
pub use scroll_amount::ScrollAmount;
use settings::Settings;
@@ -151,12 +152,16 @@ pub struct ScrollManager {
pub(crate) vertical_scroll_margin: f32,
anchor: ScrollAnchor,
ongoing: OngoingScroll,
+ /// The second element indicates whether the autoscroll request is local
+ /// (true) or remote (false). Local requests are initiated by user actions,
+ /// while remote requests come from external sources.
autoscroll_request: Option<(Autoscroll, bool)>,
last_autoscroll: Option<(gpui::Point<f32>, f32, f32, AutoscrollStrategy)>,
show_scrollbars: bool,
hide_scrollbar_task: Option<Task<()>>,
active_scrollbar: Option<ActiveScrollbarState>,
visible_line_count: Option<f32>,
+ visible_column_count: Option<f32>,
forbid_vertical_scroll: bool,
minimap_thumb_state: Option<ScrollbarThumbState>,
}
@@ -173,6 +178,7 @@ impl ScrollManager {
active_scrollbar: None,
last_autoscroll: None,
visible_line_count: None,
+ visible_column_count: None,
forbid_vertical_scroll: false,
minimap_thumb_state: None,
}
@@ -210,7 +216,7 @@ impl ScrollManager {
window: &mut Window,
cx: &mut Context<Editor>,
) {
- let (new_anchor, top_row) = if scroll_position.y <= 0. {
+ let (new_anchor, top_row) = if scroll_position.y <= 0. && scroll_position.x <= 0. {
(
ScrollAnchor {
anchor: Anchor::min(),
@@ -218,6 +224,22 @@ impl ScrollManager {
},
0,
)
+ } else if scroll_position.y <= 0. {
+ let buffer_point = map
+ .clip_point(
+ DisplayPoint::new(DisplayRow(0), scroll_position.x as u32),
+ Bias::Left,
+ )
+ .to_point(map);
+ let anchor = map.buffer_snapshot.anchor_at(buffer_point, Bias::Right);
+
+ (
+ ScrollAnchor {
+ anchor: anchor,
+ offset: scroll_position.max(&gpui::Point::default()),
+ },
+ 0,
+ )
} else {
let scroll_top = scroll_position.y;
let scroll_top = match EditorSettings::get_global(cx).scroll_beyond_last_line {
@@ -242,8 +264,13 @@ impl ScrollManager {
}
};
- let scroll_top_buffer_point =
- DisplayPoint::new(DisplayRow(scroll_top as u32), 0).to_point(map);
+ let scroll_top_row = DisplayRow(scroll_top as u32);
+ let scroll_top_buffer_point = map
+ .clip_point(
+ DisplayPoint::new(scroll_top_row, scroll_position.x as u32),
+ Bias::Left,
+ )
+ .to_point(map);
let top_anchor = map
.buffer_snapshot
.anchor_at(scroll_top_buffer_point, Bias::Right);
@@ -476,6 +503,10 @@ impl Editor {
.map(|line_count| line_count as u32 - 1)
}
+ pub fn visible_column_count(&self) -> Option<f32> {
+ self.scroll_manager.visible_column_count
+ }
+
pub(crate) fn set_visible_line_count(
&mut self,
lines: f32,
@@ -497,6 +528,10 @@ impl Editor {
}
}
+ pub(crate) fn set_visible_column_count(&mut self, columns: f32) {
+ self.scroll_manager.visible_column_count = Some(columns);
+ }
+
pub fn apply_scroll_delta(
&mut self,
scroll_delta: gpui::Point<f32>,
@@ -675,25 +710,48 @@ impl Editor {
let Some(visible_line_count) = self.visible_line_count() else {
return;
};
+ let Some(mut visible_column_count) = self.visible_column_count() else {
+ return;
+ };
+
+ // If the user has a preferred line length, and has the editor
+ // configured to wrap at the preferred line length, or bounded to it,
+ // use that value over the visible column count. This was mostly done so
+ // that tests could actually be written for vim's `z l`, `z h`, `z
+ // shift-l` and `z shift-h` commands, as there wasn't a good way to
+ // configure the editor to only display a certain number of columns. If
+ // that ever happens, this could probably be removed.
+ let settings = AllLanguageSettings::get_global(cx);
+ if matches!(
+ settings.defaults.soft_wrap,
+ SoftWrap::PreferredLineLength | SoftWrap::Bounded
+ ) {
+ if (settings.defaults.preferred_line_length as f32) < visible_column_count {
+ visible_column_count = settings.defaults.preferred_line_length as f32;
+ }
+ }
// If the scroll position is currently at the left edge of the document
// (x == 0.0) and the intent is to scroll right, the gutter's margin
// should first be added to the current position, otherwise the cursor
// will end at the column position minus the margin, which looks off.
- if current_position.x == 0.0 && amount.columns() > 0. {
+ if current_position.x == 0.0 && amount.columns(visible_column_count) > 0. {
if let Some(last_position_map) = &self.last_position_map {
current_position.x += self.gutter_dimensions.margin / last_position_map.em_advance;
}
}
- let new_position =
- current_position + point(amount.columns(), amount.lines(visible_line_count));
+ let new_position = current_position
+ + point(
+ amount.columns(visible_column_count),
+ amount.lines(visible_line_count),
+ );
self.set_scroll_position(new_position, window, cx);
}
/// Returns an ordering. The newest selection is:
/// Ordering::Equal => on screen
- /// Ordering::Less => above the screen
- /// Ordering::Greater => below the screen
+ /// Ordering::Less => above or to the left of the screen
+ /// Ordering::Greater => below or to the right of the screen
pub fn newest_selection_on_screen(&self, cx: &mut App) -> Ordering {
let snapshot = self.display_map.update(cx, |map, cx| map.snapshot(cx));
let newest_head = self
@@ -711,8 +769,12 @@ impl Editor {
return Ordering::Less;
}
- if let Some(visible_lines) = self.visible_line_count() {
- if newest_head.row() <= DisplayRow(screen_top.row().0 + visible_lines as u32) {
+ if let (Some(visible_lines), Some(visible_columns)) =
+ (self.visible_line_count(), self.visible_column_count())
+ {
+ if newest_head.row() <= DisplayRow(screen_top.row().0 + visible_lines as u32)
+ && newest_head.column() <= screen_top.column() + visible_columns as u32
+ {
return Ordering::Equal;
}
}
@@ -274,12 +274,14 @@ impl Editor {
start_row: DisplayRow,
viewport_width: Pixels,
scroll_width: Pixels,
- max_glyph_width: Pixels,
+ em_advance: Pixels,
layouts: &[LineWithInvisibles],
+ window: &mut Window,
cx: &mut Context<Self>,
) -> bool {
let display_map = self.display_map.update(cx, |map, cx| map.snapshot(cx));
let selections = self.selections.all::<Point>(cx);
+ let mut scroll_position = self.scroll_manager.scroll_position(&display_map);
let mut target_left;
let mut target_right;
@@ -295,16 +297,17 @@ impl Editor {
if head.row() >= start_row
&& head.row() < DisplayRow(start_row.0 + layouts.len() as u32)
{
- let start_column = head.column().saturating_sub(3);
- let end_column = cmp::min(display_map.line_len(head.row()), head.column() + 3);
+ let start_column = head.column();
+ let end_column = cmp::min(display_map.line_len(head.row()), head.column());
target_left = target_left.min(
layouts[head.row().minus(start_row) as usize]
- .x_for_index(start_column as usize),
+ .x_for_index(start_column as usize)
+ + self.gutter_dimensions.margin,
);
target_right = target_right.max(
layouts[head.row().minus(start_row) as usize]
.x_for_index(end_column as usize)
- + max_glyph_width,
+ + em_advance,
);
}
}
@@ -319,14 +322,16 @@ impl Editor {
return false;
}
- let scroll_left = self.scroll_manager.anchor.offset.x * max_glyph_width;
+ let scroll_left = self.scroll_manager.anchor.offset.x * em_advance;
let scroll_right = scroll_left + viewport_width;
if target_left < scroll_left {
- self.scroll_manager.anchor.offset.x = target_left / max_glyph_width;
+ scroll_position.x = target_left / em_advance;
+ self.set_scroll_position_internal(scroll_position, true, true, window, cx);
true
} else if target_right > scroll_right {
- self.scroll_manager.anchor.offset.x = (target_right - viewport_width) / max_glyph_width;
+ scroll_position.x = (target_right - viewport_width) / em_advance;
+ self.set_scroll_position_internal(scroll_position, true, true, window, cx);
true
} else {
false
@@ -23,6 +23,8 @@ pub enum ScrollAmount {
Page(f32),
// Scroll N columns (positive is towards the right of the document)
Column(f32),
+ // Scroll N page width (positive is towards the right of the document)
+ PageWidth(f32),
}
impl ScrollAmount {
@@ -37,14 +39,16 @@ impl ScrollAmount {
(visible_line_count * count).trunc()
}
Self::Column(_count) => 0.0,
+ Self::PageWidth(_count) => 0.0,
}
}
- pub fn columns(&self) -> f32 {
+ pub fn columns(&self, visible_column_count: f32) -> f32 {
match self {
Self::Line(_count) => 0.0,
Self::Page(_count) => 0.0,
Self::Column(count) => *count,
+ Self::PageWidth(count) => (visible_column_count * count).trunc(),
}
}
@@ -58,6 +62,7 @@ impl ScrollAmount {
// so I'm leaving this at 0.0 for now to try and make it clear that
// this should not have an impact on that?
ScrollAmount::Column(_) => px(0.0),
+ ScrollAmount::PageWidth(_) => px(0.0),
}
}
@@ -7,6 +7,7 @@ use editor::{
use gpui::{Context, Window, actions};
use language::Bias;
use settings::Settings;
+use text::SelectionGoal;
actions!(
vim,
@@ -26,7 +27,11 @@ actions!(
/// Scrolls up by one page.
PageUp,
/// Scrolls down by one page.
- PageDown
+ PageDown,
+ /// Scrolls right by half a page's width.
+ HalfPageRight,
+ /// Scrolls left by half a page's width.
+ HalfPageLeft,
]
);
@@ -51,6 +56,16 @@ pub fn register(editor: &mut Editor, cx: &mut Context<Vim>) {
Vim::action(editor, cx, |vim, _: &PageUp, window, cx| {
vim.scroll(false, window, cx, |c| ScrollAmount::Page(-c.unwrap_or(1.)))
});
+ Vim::action(editor, cx, |vim, _: &HalfPageRight, window, cx| {
+ vim.scroll(false, window, cx, |c| {
+ ScrollAmount::PageWidth(c.unwrap_or(0.5))
+ })
+ });
+ Vim::action(editor, cx, |vim, _: &HalfPageLeft, window, cx| {
+ vim.scroll(false, window, cx, |c| {
+ ScrollAmount::PageWidth(-c.unwrap_or(0.5))
+ })
+ });
Vim::action(editor, cx, |vim, _: &ScrollDown, window, cx| {
vim.scroll(true, window, cx, |c| {
if let Some(c) = c {
@@ -123,6 +138,10 @@ fn scroll_editor(
return;
};
+ let Some(visible_column_count) = editor.visible_column_count() else {
+ return;
+ };
+
let top_anchor = editor.scroll_manager.anchor().anchor;
let vertical_scroll_margin = EditorSettings::get_global(cx).vertical_scroll_margin;
@@ -132,8 +151,14 @@ fn scroll_editor(
cx,
|s| {
s.move_with(|map, selection| {
+ // TODO: Improve the logic and function calls below to be dependent on
+ // the `amount`. If the amount is vertical, we don't care about
+ // columns, while if it's horizontal, we don't care about rows,
+ // so we don't need to calculate both and deal with logic for
+ // both.
let mut head = selection.head();
let top = top_anchor.to_display_point(map);
+ let max_point = map.max_point();
let starting_column = head.column();
let vertical_scroll_margin =
@@ -163,9 +188,8 @@ fn scroll_editor(
(visible_line_count as u32).saturating_sub(1 + vertical_scroll_margin),
);
// scroll off the end.
- let max_row = if top.row().0 + visible_line_count as u32 >= map.max_point().row().0
- {
- map.max_point().row()
+ let max_row = if top.row().0 + visible_line_count as u32 >= max_point.row().0 {
+ max_point.row()
} else {
DisplayRow(
(top.row().0 + visible_line_count as u32)
@@ -185,13 +209,52 @@ fn scroll_editor(
} else {
head.row()
};
- let new_head =
- map.clip_point(DisplayPoint::new(new_row, starting_column), Bias::Left);
+
+ // The minimum column position that the cursor position can be
+ // at is either the scroll manager's anchor column, which is the
+ // left-most column in the visible area, or the scroll manager's
+ // old anchor column, in case the cursor position is being
+ // preserved. This is necessary for motions like `ctrl-d` in
+ // case there's not enough content to scroll half page down, in
+ // which case the scroll manager's anchor column will be the
+ // maximum column for the current line, so the minimum column
+ // would end up being the same as the maximum column.
+ let min_column = match preserve_cursor_position {
+ true => old_top_anchor.to_display_point(map).column(),
+ false => top.column(),
+ };
+
+ // As for the maximum column position, that should be either the
+ // right-most column in the visible area, which we can easily
+ // calculate by adding the visible column count to the minimum
+ // column position, or the right-most column in the current
+ // line, seeing as the cursor might be in a short line, in which
+ // case we don't want to go past its last column.
+ let max_row_column = map.line_len(new_row);
+ let max_column = match min_column + visible_column_count as u32 {
+ max_column if max_column >= max_row_column => max_row_column,
+ max_column => max_column,
+ };
+
+ // Ensure that the cursor's column stays within the visible
+ // area, otherwise clip it at either the left or right edge of
+ // the visible area.
+ let new_column = match (min_column, max_column) {
+ (min_column, _) if starting_column < min_column => min_column,
+ (_, max_column) if starting_column > max_column => max_column,
+ _ => starting_column,
+ };
+
+ let new_head = map.clip_point(DisplayPoint::new(new_row, new_column), Bias::Left);
+ let goal = match amount {
+ ScrollAmount::Column(_) | ScrollAmount::PageWidth(_) => SelectionGoal::None,
+ _ => selection.goal,
+ };
if selection.is_empty() {
- selection.collapse_to(new_head, selection.goal)
+ selection.collapse_to(new_head, goal)
} else {
- selection.set_head(new_head, selection.goal)
+ selection.set_head(new_head, goal)
};
})
},
@@ -472,4 +535,30 @@ mod test {
cx.simulate_shared_keystrokes("ctrl-o").await;
cx.shared_state().await.assert_matches();
}
+
+ #[gpui::test]
+ async fn test_horizontal_scroll(cx: &mut gpui::TestAppContext) {
+ let mut cx = NeovimBackedTestContext::new(cx).await;
+
+ cx.set_scroll_height(20).await;
+ cx.set_shared_wrap(12).await;
+ cx.set_neovim_option("nowrap").await;
+
+ let content = "ˇ01234567890123456789";
+ cx.set_shared_state(&content).await;
+
+ cx.simulate_shared_keystrokes("z shift-l").await;
+ cx.shared_state().await.assert_eq("012345ˇ67890123456789");
+
+ // At this point, `z h` should not move the cursor as it should still be
+ // visible within the 12 column width.
+ cx.simulate_shared_keystrokes("z h").await;
+ cx.shared_state().await.assert_eq("012345ˇ67890123456789");
+
+ let content = "ˇ01234567890123456789";
+ cx.set_shared_state(&content).await;
+
+ cx.simulate_shared_keystrokes("z l").await;
+ cx.shared_state().await.assert_eq("0ˇ1234567890123456789");
+ }
}
@@ -0,0 +1,16 @@
+{"SetOption":{"value":"scrolloff=3"}}
+{"SetOption":{"value":"lines=22"}}
+{"SetOption":{"value":"wrap"}}
+{"SetOption":{"value":"columns=12"}}
+{"SetOption":{"value":"nowrap"}}
+{"Put":{"state":"ˇ01234567890123456789"}}
+{"Key":"z"}
+{"Key":"shift-l"}
+{"Get":{"state":"012345ˇ67890123456789","mode":"Normal"}}
+{"Key":"z"}
+{"Key":"h"}
+{"Get":{"state":"012345ˇ67890123456789","mode":"Normal"}}
+{"Put":{"state":"ˇ01234567890123456789"}}
+{"Key":"z"}
+{"Key":"l"}
+{"Get":{"state":"0ˇ1234567890123456789","mode":"Normal"}}