Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

correctly map unsorted positions #7471

Merged
merged 2 commits into from
Jun 28, 2023
Merged

correctly map unsorted positions #7471

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a29d03
correctly map unsorted positions
pascalkuthe Jun 27, 2023
40afd4d
Fix typo
pascalkuthe Jun 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 94 additions & 36 deletions helix-core/src/transaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ impl ChangeSet {
self.changes.is_empty() || self.changes == [Operation::Retain(self.len)]
}

/// Map a *sorted* list of positions through the changes.
/// Map a (mostly) *sorted* list of positions through the changes.
///
/// This is equivalent to updating each position with `map_pos`:
///
Expand All @@ -335,27 +335,63 @@ impl ChangeSet {
/// *pos = changes.map_pos(*pos, assoc);
/// }
/// ```
/// However this function is significantly faster running in `O(N+M)` instead of `O(NM)`
/// However this function is significantly faster for sorted lists running
/// in `O(N+M)` instead of `O(NM)`. This function also handles unsorted/
/// partially sorted lists. However, in that case worst case complexity is
/// again `O(MN)`. For lists that are often/mostly sorted (like the end of diagnostic ranges)
/// performanc is usally close to `O(N + M)`
pascalkuthe marked this conversation as resolved.
Show resolved Hide resolved
pub fn update_positions<'a>(&self, positions: impl Iterator<Item = (&'a mut usize, Assoc)>) {
use Operation::*;

let mut positions = positions.peekable();
macro_rules! map {
($map: expr) => {
loop {
let Some((pos, assoc)) = positions.peek_mut() else { return; };
let Some(new_pos) = $map(**pos, *assoc) else { break; };
**pos = new_pos;
positions.next();
}
};
}

let mut old_pos = 0;
let mut new_pos = 0;
let mut iter = self.changes.iter().peekable();
let mut iter = self.changes.iter().enumerate().peekable();

'outer: loop {
macro_rules! map {
($map: expr, $i: expr) => {
loop {
let Some((pos, assoc)) = positions.peek_mut() else { return; };
if **pos < old_pos {
// Positions are not sorted, revert to the last Operation that
// contains this position and continue iterating from there.
// We can unwrap here since `pos` can not be negative
// (unsigned integer) and iterating backwards to the start
// should always move us back to the start
for (i, change) in self.changes[..$i].iter().enumerate().rev() {
match change {
Retain(i) => {
old_pos -= i;
new_pos -= i;
}
Delete(i) => {
old_pos -= i;
}
Insert(ins) => {
new_pos -= ins.chars().count();
}
}
if old_pos <= **pos {
iter = self.changes[i..].iter().enumerate().peekable();
}
}
debug_assert!(old_pos <= **pos, "Reverse Iter across changeset works");
continue 'outer;
}
let Some(new_pos) = $map(**pos, *assoc) else { break; };
**pos = new_pos;
positions.next();
}
};
}

let Some((i, change)) = iter.next() else {
map!(|pos, _| (old_pos == pos).then_some(new_pos), self.changes.len());
break;
};

while let Some(change) = iter.next() {
let len = match change {
Delete(i) | Retain(i) => *i,
Insert(_) => 0,
Expand All @@ -364,50 +400,58 @@ impl ChangeSet {

match change {
Retain(_) => {
map!(|pos, _| (old_end > pos).then_some(new_pos + (pos - old_pos)));
map!(
|pos, _| (old_end > pos).then_some(new_pos + (pos - old_pos)),
i
);
new_pos += len;
}
Delete(_) => {
// in range
map!(|pos, _| (old_end > pos).then_some(new_pos));
map!(|pos, _| (old_end > pos).then_some(new_pos), i);
}
Insert(s) => {
let ins = s.chars().count();

// a subsequent delete means a replace, consume it
if let Some(Delete(len)) = iter.peek() {
if let Some((_, Delete(len))) = iter.peek() {
iter.next();

old_end = old_pos + len;
// in range of replaced text
map!(|pos, assoc| (old_end > pos).then(|| {
// at point or tracking before
if pos == old_pos || assoc == Assoc::Before {
new_pos
} else {
// place to end of insert
new_pos + ins
}
}));
map!(
|pos, assoc| (old_end > pos).then(|| {
// at point or tracking before
if pos == old_pos || assoc == Assoc::Before {
new_pos
} else {
// place to end of insert
new_pos + ins
}
}),
i
);
} else {
// at insert point
map!(|pos, assoc| (old_pos == pos).then(|| {
// return position before inserted text
if assoc == Assoc::Before {
new_pos
} else {
// after text
new_pos + ins
}
}));
map!(
|pos, assoc| (old_pos == pos).then(|| {
// return position before inserted text
if assoc == Assoc::Before {
new_pos
} else {
// after text
new_pos + ins
}
}),
i
);
}

new_pos += ins;
}
}
old_pos = old_end;
}
map!(|pos, _| (old_pos == pos).then_some(new_pos));
let out_of_bounds: Vec<_> = positions.collect();

panic!("Positions {out_of_bounds:?} are out of range for changeset len {old_pos}!",)
Expand Down Expand Up @@ -822,6 +866,20 @@ mod test {
};
assert_eq!(cs.map_pos(2, Assoc::Before), 2);
assert_eq!(cs.map_pos(2, Assoc::After), 2);
// unsorted selection
let cs = ChangeSet {
changes: vec![
Insert("ab".into()),
Delete(2),
Insert("cd".into()),
Delete(2),
],
len: 4,
len_after: 4,
};
let mut positions = [4, 2];
cs.update_positions(positions.iter_mut().map(|pos| (pos, Assoc::After)));
assert_eq!(positions, [4, 2]);
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion helix-view/src/document.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1197,7 +1197,7 @@ impl Document {
if let Some(data) = Rc::get_mut(annotations) {
changes.update_positions(
data.iter_mut()
.map(|diagnostic| (&mut diagnostic.char_idx, Assoc::After)),
.map(|annotation| (&mut annotation.char_idx, Assoc::After)),
);
}
};
Expand Down