on master help sign in
correctly map unsorted positions (#7471)

* correctly map unsorted positions * Fix typo Co-authored-by: Michael Davis <mcarsondavis@gmail.com> --------- Co-authored-by: Michael Davis <mcarsondavis@gmail.com>

commit4a2337d828c6c2fa7c0e46052e4c8a62dbd4737d authorPascal Kuthe <pascal.kuthe@semimod.de> dateWed 28 Jun 2023, 16:35 (3 years ago) parentd8f9b90
2 files changed+95−37
helix-core/src/transaction.rs+94−36 helix-view/src/document.rs+1−1
helix-core/src/transaction.rs+94−36
@@ -326,7 +326,7 @@ impl ChangeSet {
326326 self.changes.is_empty() || self.changes == [Operation::Retain(self.len)]
327327 }
328328
329− /// Map a *sorted* list of positions through the changes.
329+ /// Map a (mostly) *sorted* list of positions through the changes.
330330 ///
331331 /// This is equivalent to updating each position with `map_pos`:
332332 ///
@@ -335,27 +335,63 @@ impl ChangeSet {
335335 /// *pos = changes.map_pos(*pos, assoc);
336336 /// }
337337 /// ```
338− /// However this function is significantly faster running in `O(N+M)` instead of `O(NM)`
338+ /// However this function is significantly faster for sorted lists running
339+ /// in `O(N+M)` instead of `O(NM)`. This function also handles unsorted/
340+ /// partially sorted lists. However, in that case worst case complexity is
341+ /// again `O(MN)`. For lists that are often/mostly sorted (like the end of diagnostic ranges)
342+ /// performance is usally close to `O(N + M)`
339343 pub fn update_positions<'a>(&self, positions: impl Iterator<Item = (&'a mut usize, Assoc)>) {
340344 use Operation::*;
341345
342346 let mut positions = positions.peekable();
343− macro_rules! map {
344− ($map: expr) => {
345− loop {
346− let Some((pos, assoc)) = positions.peek_mut() else { return; };
347− let Some(new_pos) = $map(**pos, *assoc) else { break; };
348− **pos = new_pos;
349− positions.next();
350− }
351− };
352− }
353347
354348 let mut old_pos = 0;
355349 let mut new_pos = 0;
356− let mut iter = self.changes.iter().peekable();
350+ let mut iter = self.changes.iter().enumerate().peekable();
351+
352+ 'outer: loop {
353+ macro_rules! map {
354+ ($map: expr, $i: expr) => {
355+ loop {
356+ let Some((pos, assoc)) = positions.peek_mut() else { return; };
357+ if **pos < old_pos {
358+ // Positions are not sorted, revert to the last Operation that
359+ // contains this position and continue iterating from there.
360+ // We can unwrap here since `pos` can not be negative
361+ // (unsigned integer) and iterating backwards to the start
362+ // should always move us back to the start
363+ for (i, change) in self.changes[..$i].iter().enumerate().rev() {
364+ match change {
365+ Retain(i) => {
366+ old_pos -= i;
367+ new_pos -= i;
368+ }
369+ Delete(i) => {
370+ old_pos -= i;
371+ }
372+ Insert(ins) => {
373+ new_pos -= ins.chars().count();
374+ }
375+ }
376+ if old_pos <= **pos {
377+ iter = self.changes[i..].iter().enumerate().peekable();
378+ }
379+ }
380+ debug_assert!(old_pos <= **pos, "Reverse Iter across changeset works");
381+ continue 'outer;
382+ }
383+ let Some(new_pos) = $map(**pos, *assoc) else { break; };
384+ **pos = new_pos;
385+ positions.next();
386+ }
387+ };
388+ }
389+
390+ let Some((i, change)) = iter.next() else {
391+ map!(|pos, _| (old_pos == pos).then_some(new_pos), self.changes.len());
392+ break;
393+ };
357394
358− while let Some(change) = iter.next() {
359395 let len = match change {
360396 Delete(i) | Retain(i) => *i,
361397 Insert(_) => 0,
@@ -364,42 +400,51 @@ impl ChangeSet {
364400
365401 match change {
366402 Retain(_) => {
367− map!(|pos, _| (old_end > pos).then_some(new_pos + (pos - old_pos)));
403+ map!(
404+ |pos, _| (old_end > pos).then_some(new_pos + (pos - old_pos)),
405+ i
406+ );
368407 new_pos += len;
369408 }
370409 Delete(_) => {
371410 // in range
372− map!(|pos, _| (old_end > pos).then_some(new_pos));
411+ map!(|pos, _| (old_end > pos).then_some(new_pos), i);
373412 }
374413 Insert(s) => {
375414 let ins = s.chars().count();
376415
377416 // a subsequent delete means a replace, consume it
378− if let Some(Delete(len)) = iter.peek() {
417+ if let Some((_, Delete(len))) = iter.peek() {
379418 iter.next();
380419
381420 old_end = old_pos + len;
382421 // in range of replaced text
383− map!(|pos, assoc| (old_end > pos).then(|| {
384− // at point or tracking before
385− if pos == old_pos || assoc == Assoc::Before {
386− new_pos
387− } else {
388− // place to end of insert
389− new_pos + ins
390− }
391− }));
422+ map!(
423+ |pos, assoc| (old_end > pos).then(|| {
424+ // at point or tracking before
425+ if pos == old_pos || assoc == Assoc::Before {
426+ new_pos
427+ } else {
428+ // place to end of insert
429+ new_pos + ins
430+ }
431+ }),
432+ i
433+ );
392434 } else {
393435 // at insert point
394− map!(|pos, assoc| (old_pos == pos).then(|| {
395− // return position before inserted text
396− if assoc == Assoc::Before {
397− new_pos
398− } else {
399− // after text
400− new_pos + ins
401− }
402− }));
436+ map!(
437+ |pos, assoc| (old_pos == pos).then(|| {
438+ // return position before inserted text
439+ if assoc == Assoc::Before {
440+ new_pos
441+ } else {
442+ // after text
443+ new_pos + ins
444+ }
445+ }),
446+ i
447+ );
403448 }
404449
405450 new_pos += ins;
@@ -407,7 +452,6 @@ impl ChangeSet {
407452 }
408453 old_pos = old_end;
409454 }
410− map!(|pos, _| (old_pos == pos).then_some(new_pos));
411455 let out_of_bounds: Vec<_> = positions.collect();
412456
413457 panic!("Positions {out_of_bounds:?} are out of range for changeset len {old_pos}!",)
@@ -822,6 +866,20 @@ mod test {
822866 };
823867 assert_eq!(cs.map_pos(2, Assoc::Before), 2);
824868 assert_eq!(cs.map_pos(2, Assoc::After), 2);
869+ // unsorted selection
870+ let cs = ChangeSet {
871+ changes: vec![
872+ Insert("ab".into()),
873+ Delete(2),
874+ Insert("cd".into()),
875+ Delete(2),
876+ ],
877+ len: 4,
878+ len_after: 4,
879+ };
880+ let mut positions = [4, 2];
881+ cs.update_positions(positions.iter_mut().map(|pos| (pos, Assoc::After)));
882+ assert_eq!(positions, [4, 2]);
825883 }
826884
827885 #[test]
helix-view/src/document.rs+1−1
@@ -1197,7 +1197,7 @@ impl Document {
11971197 if let Some(data) = Rc::get_mut(annotations) {
11981198 changes.update_positions(
11991199 data.iter_mut()
1200− .map(|diagnostic| (&mut diagnostic.char_idx, Assoc::After)),
1200+ .map(|annotation| (&mut annotation.char_idx, Assoc::After)),
12011201 );
12021202 }
12031203 };