on master help sign in
core: Improve changeset composition behavior.

It would fail to combine with an empty set.

commitf00cb15137fdff72c2f08c4c6f43bfa96933433f authorBlaž Hrastnik <blaz@mxxn.io> dateTue 6 Apr 2021, 19:01 (6 years ago) parentcf7b19d
1 file changed+69−32
helix-core/src/transaction.rs+69−32
helix-core/src/transaction.rs+69−32
@@ -27,15 +27,35 @@ pub struct ChangeSet {
2727 pub(crate) changes: Vec<Operation>,
2828 /// The required document length. Will refuse to apply changes unless it matches.
2929 len: usize,
30+ len_after: usize,
31+}
32+
33+impl Default for ChangeSet {
34+ fn default() -> Self {
35+ Self {
36+ changes: Vec::new(),
37+ len: 0,
38+ len_after: 0,
39+ }
40+ }
3041}
3142
3243impl ChangeSet {
44+ pub fn with_capacity(capacity: usize) -> Self {
45+ Self {
46+ changes: Vec::with_capacity(capacity),
47+ len: 0,
48+ len_after: 0,
49+ }
50+ }
51+
3352 #[must_use]
3453 pub fn new(doc: &Rope) -> Self {
3554 let len = doc.len_chars();
3655 Self {
37− changes: vec![Operation::Retain(len)],
56+ changes: Vec::new(),
3857 len,
58+ len_after: len,
3959 }
4060 }
4161
@@ -47,21 +67,6 @@ impl ChangeSet {
4767 &self.changes
4868 }
4969
50− #[must_use]
51− fn len_after(&self) -> usize {
52− use Operation::*;
53−
54− let mut len = 0;
55− for change in &self.changes {
56− match change {
57− Retain(i) => len += i,
58− Insert(s) => len += s.chars().count(),
59− Delete(_) => (),
60− }
61− }
62− len
63− }
64−
6570 // Changeset builder operations: delete/insert/retain
6671 fn delete(&mut self, n: usize) {
6772 use Operation::*;
@@ -69,6 +74,8 @@ impl ChangeSet {
6974 return;
7075 }
7176
77+ self.len += n;
78+
7279 if let Some(Delete(count)) = self.changes.last_mut() {
7380 *count += n;
7481 } else {
@@ -83,6 +90,8 @@ impl ChangeSet {
8390 return;
8491 }
8592
93+ self.len_after += fragment.len();
94+
8695 let new_last = match self.changes.as_mut_slice() {
8796 [.., Insert(prev)] | [.., Insert(prev), Delete(_)] => {
8897 prev.push_tendril(&fragment);
@@ -101,6 +110,9 @@ impl ChangeSet {
101110 return;
102111 }
103112
113+ self.len += n;
114+ self.len_after += n;
115+
104116 if let Some(Retain(count)) = self.changes.last_mut() {
105117 *count += n;
106118 } else {
@@ -112,7 +124,13 @@ impl ChangeSet {
112124 /// In other words, If `this` goes `docA` → `docB` and `other` represents `docB` → `docC`, the
113125 /// returned value will represent the change `docA` → `docC`.
114126 pub fn compose(self, other: Self) -> Self {
115− debug_assert!(self.len_after() == other.len);
127+ debug_assert!(self.len_after == other.len);
128+
129+ // composing fails in weird ways if one of the sets is empty
130+ // a: [] len: 0 len_after: 1 | b: [Insert(Tendril<UTF8>(inline: "\n")), Retain(1)] len 1
131+ if self.changes.is_empty() {
132+ return other;
133+ }
116134
117135 let len = self.changes.len();
118136
@@ -122,10 +140,7 @@ impl ChangeSet {
122140 let mut head_a = changes_a.next();
123141 let mut head_b = changes_b.next();
124142
125− let mut changes = Self {
126− len: self.len,
127− changes: Vec::with_capacity(len), // TODO: max(a, b), shrink_to_fit() afterwards
128− };
143+ let mut changes = Self::with_capacity(len); // TODO: max(a, b), shrink_to_fit() afterwards
129144
130145 loop {
131146 use std::cmp::Ordering;
@@ -228,6 +243,9 @@ impl ChangeSet {
228243 };
229244 }
230245
246+ // starting len should still equal original starting len
247+ debug_assert!(changes.len == self.len);
248+
231249 changes
232250 }
233251
@@ -251,7 +269,8 @@ impl ChangeSet {
251269 pub fn invert(&self, original_doc: &Rope) -> Self {
252270 assert!(original_doc.len_chars() == self.len);
253271
254− let mut changes = Vec::with_capacity(self.changes.len());
272+ let mut changes = Self::with_capacity(self.changes.len());
273+
255274 let mut pos = 0;
256275 let mut len = 0;
257276
@@ -259,24 +278,22 @@ impl ChangeSet {
259278 use Operation::*;
260279 match change {
261280 Retain(n) => {
262− changes.push(Retain(*n));
281+ changes.retain(*n);
263282 pos += n;
264− len += n;
265283 }
266284 Delete(n) => {
267285 let text = Cow::from(original_doc.slice(pos..pos + *n));
268− changes.push(Insert(Tendril::from_slice(&text)));
286+ changes.insert(Tendril::from_slice(&text));
269287 pos += n;
270288 }
271289 Insert(s) => {
272290 let chars = s.chars().count();
273− changes.push(Delete(chars));
274− len += chars;
291+ changes.delete(chars);
275292 }
276293 }
277294 }
278295
279− Self { changes, len }
296+ changes
280297 }
281298
282299 /// Returns true if applied successfully.
@@ -309,7 +326,7 @@ impl ChangeSet {
309326 /// `true` when the set is empty.
310327 #[inline]
311328 pub fn is_empty(&self) -> bool {
312− matches!(self.changes.as_slice(), [] | [Operation::Retain(_)])
329+ self.changes.is_empty()
313330 }
314331
315332 /// Map a position through the changes.
@@ -458,8 +475,8 @@ impl Transaction {
458475 I: IntoIterator<Item = Change> + ExactSizeIterator,
459476 {
460477 let len = doc.len_chars();
461− let acc = Vec::with_capacity(2 * changes.len() + 1); // rough estimate
462− let mut changeset = ChangeSet { changes: acc, len };
478+
479+ let mut changeset = ChangeSet::with_capacity(2 * changes.len() + 1); // rough estimate
463480
464481 // TODO: verify ranges are ordered and not overlapping or change will panic.
465482
@@ -576,11 +593,13 @@ mod test {
576593 Insert("abc".into()),
577594 ],
578595 len: 8,
596+ len_after: 15,
579597 };
580598
581599 let b = ChangeSet {
582600 changes: vec![Delete(10), Insert("world".into()), Retain(5)],
583601 len: 15,
602+ len_after: 10,
584603 };
585604
586605 let mut text = Rope::from("hello xz");
@@ -597,8 +616,9 @@ mod test {
597616 use Operation::*;
598617
599618 let changes = ChangeSet {
600− changes: vec![Retain(4), Delete(5), Insert("test".into()), Retain(3)],
619+ changes: vec![Retain(4), Insert("test".into()), Delete(5), Retain(3)],
601620 len: 12,
621+ len_after: 11,
602622 };
603623
604624 let doc = Rope::from("世界3 hello xz");
@@ -627,6 +647,7 @@ mod test {
627647 let cs = ChangeSet {
628648 changes: vec![Retain(4), Insert("!!".into()), Retain(4)],
629649 len: 8,
650+ len_after: 10,
630651 };
631652
632653 assert_eq!(cs.map_pos(0, Assoc::Before), 0); // before insert region
@@ -638,6 +659,7 @@ mod test {
638659 let cs = ChangeSet {
639660 changes: vec![Retain(4), Delete(4), Retain(4)],
640661 len: 12,
662+ len_after: 8,
641663 };
642664 assert_eq!(cs.map_pos(0, Assoc::Before), 0); // at start
643665 assert_eq!(cs.map_pos(4, Assoc::Before), 4); // before a delete
@@ -655,6 +677,7 @@ mod test {
655677 Delete(2),
656678 ],
657679 len: 4,
680+ len_after: 4,
658681 };
659682 assert_eq!(cs.map_pos(2, Assoc::Before), 2);
660683 assert_eq!(cs.map_pos(2, Assoc::After), 2);
@@ -717,4 +740,18 @@ mod test {
717740 assert_eq!(changes.changes, &[Insert("hello".into())]);
718741 // instead of insert h, insert e, insert l, insert l, insert o
719742 }
743+
744+ #[test]
745+ fn combine_with_empty() {
746+ let empty = Rope::from("");
747+ let mut a = ChangeSet::new(&empty);
748+
749+ let mut b = ChangeSet::new(&empty);
750+ b.insert("a".into());
751+
752+ let changes = a.compose(b);
753+
754+ use Operation::*;
755+ assert_eq!(changes.changes, &[Insert("a".into())]);
756+ }
720757}