fix view anchors not at start of a visual line

The top of a view is marked by a char idx anchor. That char idx is
usually the first character of the visual line it's on. We use a char
index instead of a line index because the view may start in the middle
of a line with soft wrapping. However, it's possible to temporarily
endup in a state where this anchor is not the first character of the
first visual line. This is pretty rare because edits usually happen
inside/after the view. In most cases we handle this case correctly.

However, if the cursor is before the anchor (but still in view)
there can be crashes or visual artifacts. This is caused by the fact
that visual_offset_from_anchor (and the positioning code in view.rs)
incorrectly assumed that the (cursor) position is always after the
view anchor if the cursor is in view. But if the anchor is not the
first character of the first visual line this is not the case anymore.

In that case crashes and visual artifacts are possible. This commit
fixes that problem by changing `visual_offset_from_anchor` (and
callsites) to properly consider that case.
pull/6188/merge
Pascal Kuthe 2 years ago committed by Blaž Hrastnik
parent 0ab96cc257
commit 72b9311678

@ -98,7 +98,7 @@ pub use {regex, tree_sitter};
pub use graphemes::RopeGraphemes; pub use graphemes::RopeGraphemes;
pub use position::{ pub use position::{
char_idx_at_visual_offset, coords_at_pos, pos_at_coords, visual_offset_from_anchor, char_idx_at_visual_offset, coords_at_pos, pos_at_coords, visual_offset_from_anchor,
visual_offset_from_block, Position, visual_offset_from_block, Position, VisualOffsetError,
}; };
#[allow(deprecated)] #[allow(deprecated)]
pub use position::{pos_at_visual_coords, visual_coords_at_pos}; pub use position::{pos_at_visual_coords, visual_coords_at_pos};

@ -137,6 +137,12 @@ pub fn visual_offset_from_block(
(last_pos, block_start) (last_pos, block_start)
} }
#[derive(Debug, PartialEq, Eq, Clone, Copy)]
pub enum VisualOffsetError {
PosBeforeAnchorRow,
PosAfterMaxRow,
}
/// Returns the visual offset from the start of the visual line /// Returns the visual offset from the start of the visual line
/// that contains anchor. /// that contains anchor.
pub fn visual_offset_from_anchor( pub fn visual_offset_from_anchor(
@ -146,28 +152,46 @@ pub fn visual_offset_from_anchor(
text_fmt: &TextFormat, text_fmt: &TextFormat,
annotations: &TextAnnotations, annotations: &TextAnnotations,
max_rows: usize, max_rows: usize,
) -> Option<(Position, usize)> { ) -> Result<(Position, usize), VisualOffsetError> {
let (formatter, block_start) = let (formatter, block_start) =
DocumentFormatter::new_at_prev_checkpoint(text, text_fmt, annotations, anchor); DocumentFormatter::new_at_prev_checkpoint(text, text_fmt, annotations, anchor);
let mut char_pos = block_start; let mut char_pos = block_start;
let mut anchor_line = None; let mut anchor_line = None;
let mut found_pos = None;
let mut last_pos = Position::default(); let mut last_pos = Position::default();
if pos < block_start {
return Err(VisualOffsetError::PosBeforeAnchorRow);
}
for (grapheme, vpos) in formatter { for (grapheme, vpos) in formatter {
last_pos = vpos; last_pos = vpos;
char_pos += grapheme.doc_chars(); char_pos += grapheme.doc_chars();
if char_pos > anchor && anchor_line.is_none() {
anchor_line = Some(last_pos.row);
}
if char_pos > pos { if char_pos > pos {
last_pos.row -= anchor_line.unwrap(); if let Some(anchor_line) = anchor_line {
return Some((last_pos, block_start)); last_pos.row -= anchor_line;
return Ok((last_pos, block_start));
} else {
found_pos = Some(last_pos);
}
}
if char_pos > anchor && anchor_line.is_none() {
if let Some(mut found_pos) = found_pos {
return if found_pos.row == last_pos.row {
found_pos.row = 0;
Ok((found_pos, block_start))
} else {
Err(VisualOffsetError::PosBeforeAnchorRow)
};
} else {
anchor_line = Some(last_pos.row);
}
} }
if let Some(anchor_line) = anchor_line { if let Some(anchor_line) = anchor_line {
if vpos.row >= anchor_line + max_rows { if vpos.row >= anchor_line + max_rows {
return None; return Err(VisualOffsetError::PosAfterMaxRow);
} }
} }
} }
@ -175,7 +199,7 @@ pub fn visual_offset_from_anchor(
let anchor_line = anchor_line.unwrap_or(last_pos.row); let anchor_line = anchor_line.unwrap_or(last_pos.row);
last_pos.row -= anchor_line; last_pos.row -= anchor_line;
Some((last_pos, block_start)) Ok((last_pos, block_start))
} }
/// Convert (line, column) coordinates to a character index. /// Convert (line, column) coordinates to a character index.

@ -7,9 +7,13 @@ use crate::{
}; };
use helix_core::{ use helix_core::{
char_idx_at_visual_offset, doc_formatter::TextFormat, syntax::Highlight, char_idx_at_visual_offset,
text_annotations::TextAnnotations, visual_offset_from_anchor, visual_offset_from_block, doc_formatter::TextFormat,
Position, RopeSlice, Selection, Transaction, syntax::Highlight,
text_annotations::TextAnnotations,
visual_offset_from_anchor, visual_offset_from_block, Position, RopeSlice, Selection,
Transaction,
VisualOffsetError::{PosAfterMaxRow, PosBeforeAnchorRow},
}; };
use std::{ use std::{
@ -213,46 +217,38 @@ impl View {
// - 1 so we have at least one gap in the middle. // - 1 so we have at least one gap in the middle.
// a height of 6 with padding of 3 on each side will keep shifting the view back and forth // a height of 6 with padding of 3 on each side will keep shifting the view back and forth
// as we type // as we type
let scrolloff = scrolloff.min(viewport.height.saturating_sub(1) as usize / 2); let scrolloff = if CENTERING {
0
} else {
scrolloff.min(viewport.height.saturating_sub(1) as usize / 2)
};
let cursor = doc.selection(self.id).primary().cursor(doc_text); let cursor = doc.selection(self.id).primary().cursor(doc_text);
let mut offset = self.offset; let mut offset = self.offset;
let off = visual_offset_from_anchor(
doc_text,
offset.anchor,
cursor,
&text_fmt,
&annotations,
vertical_viewport_end,
);
let (visual_off, mut at_top) = if cursor >= offset.anchor { let (new_anchor, at_top) = match off {
let off = visual_offset_from_anchor( Ok((visual_pos, _)) if visual_pos.row < scrolloff + offset.vertical_offset => {
doc_text, if CENTERING {
offset.anchor,
cursor,
&text_fmt,
&annotations,
vertical_viewport_end,
);
(off, false)
} else if CENTERING {
// cursor out of view
return None;
} else {
(None, true)
};
let new_anchor = match visual_off {
Some((visual_pos, _)) if visual_pos.row < scrolloff + offset.vertical_offset => {
if CENTERING && visual_pos.row < offset.vertical_offset {
// cursor out of view // cursor out of view
return None; return None;
} }
at_top = true; (true, true)
true
} }
Some((visual_pos, _)) if visual_pos.row + scrolloff + 1 >= vertical_viewport_end => { Ok((visual_pos, _)) if visual_pos.row + scrolloff >= vertical_viewport_end => {
if CENTERING && visual_pos.row >= vertical_viewport_end { (true, false)
// cursor out of view
return None;
}
true
} }
Some(_) => false, Ok((_, _)) => (false, false),
None => true, Err(_) if CENTERING => return None,
Err(PosBeforeAnchorRow) => (true, true),
Err(PosAfterMaxRow) => (true, false),
}; };
if new_anchor { if new_anchor {
@ -269,8 +265,8 @@ impl View {
offset.horizontal_offset = 0; offset.horizontal_offset = 0;
} else { } else {
// determine the current visual column of the text // determine the current visual column of the text
let col = visual_off let col = off
.unwrap_or_else(|| { .unwrap_or_else(|_| {
visual_offset_from_block( visual_offset_from_block(
doc_text, doc_text,
offset.anchor, offset.anchor,
@ -360,8 +356,9 @@ impl View {
); );
match pos { match pos {
Some((Position { row, .. }, _)) => row.saturating_sub(self.offset.vertical_offset), Ok((Position { row, .. }, _)) => row.saturating_sub(self.offset.vertical_offset),
None => visual_height.saturating_sub(1), Err(PosAfterMaxRow) => visual_height.saturating_sub(1),
Err(PosBeforeAnchorRow) => 0,
} }
} }
@ -390,7 +387,8 @@ impl View {
&text_fmt, &text_fmt,
&annotations, &annotations,
viewport.height as usize, viewport.height as usize,
)? )
.ok()?
.0; .0;
if pos.row < self.offset.vertical_offset { if pos.row < self.offset.vertical_offset {
return None; return None;

Loading…
Cancel
Save