From f95bd4324018930652996ef1ea321387fe0cf945 Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sun, 22 Jun 2025 12:02:18 +0100 Subject: [PATCH] Works without indexes --- src/operation_transformation/edited_text.rs | 119 +++++++--- src/operation_transformation/operation.rs | 222 ++++++++---------- ...ted_text__tests__calculate_operations.snap | 16 +- ...ts__calculate_operations_with_no_diff.snap | 8 +- .../find_longest_prefix_contained_within.rs | 6 +- src/utils/string_builder.rs | 22 +- 6 files changed, 203 insertions(+), 190 deletions(-) diff --git a/src/operation_transformation/edited_text.rs b/src/operation_transformation/edited_text.rs index 4190157..10a0133 100644 --- a/src/operation_transformation/edited_text.rs +++ b/src/operation_transformation/edited_text.rs @@ -115,25 +115,42 @@ where let mut seen_right_length: usize = 0; let mut merged_length: usize = 0; - loop { - let (side, OrderedOperation { operation, order }, maybe_other_operation) = - match (maybe_left_op.clone(), maybe_right_op.clone()) { - (Some(left_op), Some(right_op)) => { - if left_op - .get_sort_key(seen_left_length) - .partial_cmp(&right_op.get_sort_key(seen_right_length)) - == Some(std::cmp::Ordering::Less) - { - (Side::Left, left_op, maybe_right_op.clone()) - } else { - (Side::Right, right_op, maybe_left_op.clone()) - } - } + let mut last_left_op = None; + let mut last_right_op = None; - (Some(left_op), None) => (Side::Left, left_op, None), - (None, Some(right_op)) => (Side::Right, right_op, None), - (None, None) => break, - }; + loop { + let ( + side, + OrderedOperation { operation, order }, + maybe_other_operation, + mut last_other_op, + ) = match (maybe_left_op.clone(), maybe_right_op.clone()) { + (Some(left_op), Some(right_op)) => { + if left_op + .get_sort_key(seen_left_length) + .partial_cmp(&right_op.get_sort_key(seen_right_length)) + == Some(std::cmp::Ordering::Less) + { + ( + Side::Left, + left_op, + maybe_right_op.clone(), + last_right_op.clone(), + ) + } else { + ( + Side::Right, + right_op, + maybe_left_op.clone(), + last_left_op.clone(), + ) + } + } + + (Some(left_op), None) => (Side::Left, left_op, None, last_right_op.clone()), + (None, Some(right_op)) => (Side::Right, right_op, None, last_left_op.clone()), + (None, None) => break, + }; let is_advancing_operation = matches!( operation, @@ -144,15 +161,32 @@ where let original_length = operation.len() as i64; let result = match side { Side::Left => { - let result = - operation.merge_operations_with_context(order, maybe_other_operation); + let result = operation.merge_operations_with_context( + order, + &mut last_other_op, + maybe_other_operation, + ); - if let Some(ref op @ (Operation::Insert { .. } | Operation::Equal { .. })) = - result + if let Some( + ref op @ (OrderedOperation { + operation: Operation::Insert { .. }, + .. + } + | OrderedOperation { + operation: Operation::Equal { .. }, + .. + }), + ) = result { let mut shift = merged_length as i64 - seen_left_length as i64; - if !matches!(op, Operation::Equal { .. }) { - shift += op.len() as i64 - original_length; + if !matches!( + op, + OrderedOperation { + operation: Operation::Equal { .. }, + .. + } + ) { + shift += op.operation.len() as i64 - original_length; } while let Some(cursor) = left_cursors.next_if(|cursor| { @@ -170,19 +204,37 @@ where } maybe_left_op = left_iter.next(); + last_left_op = result.clone(); result } Side::Right => { - let result = - operation.merge_operations_with_context(order, maybe_other_operation); + let result = operation.merge_operations_with_context( + order, + &mut last_other_op, + maybe_other_operation, + ); - if let Some(ref op @ (Operation::Insert { .. } | Operation::Equal { .. })) = - result + if let Some( + ref op @ (OrderedOperation { + operation: Operation::Insert { .. }, + .. + } + | OrderedOperation { + operation: Operation::Equal { .. }, + .. + }), + ) = result { let mut shift = merged_length as i64 - seen_right_length as i64; - if !matches!(op, Operation::Equal { .. }) { - shift += op.len() as i64 - original_length; + if !matches!( + op, + OrderedOperation { + operation: Operation::Equal { .. }, + .. + } + ) { + shift += op.operation.len() as i64 - original_length; } while let Some(cursor) = right_cursors.next_if(|cursor| { @@ -200,6 +252,7 @@ where } maybe_right_op = right_iter.next(); + last_right_op = result.clone(); result } @@ -208,15 +261,15 @@ where println!(" = {result:?}"); if let Some(operation) = result { - if operation.len() == 0 { + if operation.operation.len() == 0 { continue; } if is_advancing_operation { - merged_length += operation.len(); + merged_length += operation.operation.len(); } - merged_operations.push(OrderedOperation { order, operation }); + merged_operations.push(operation); } } diff --git a/src/operation_transformation/operation.rs b/src/operation_transformation/operation.rs index 653020f..fbaf786 100644 --- a/src/operation_transformation/operation.rs +++ b/src/operation_transformation/operation.rs @@ -4,7 +4,6 @@ use std::ops::Range; #[cfg(feature = "serde")] use serde::{Deserialize, Serialize}; -use super::merge_context::MergeContext; use crate::{ Token, operation_transformation::ordered_operation::OrderedOperation, @@ -40,8 +39,6 @@ where }, } - - impl Operation where T: PartialEq + Clone + std::fmt::Debug, @@ -49,7 +46,7 @@ where /// Creates an equal operation with the given index. /// This operation is used to indicate that the text at the given index /// is unchanged. - pub fn create_equal( length: usize) -> Option { + pub fn create_equal(length: usize) -> Option { Some(Operation::Equal { length, @@ -58,7 +55,7 @@ where }) } - pub fn create_equal_with_text( text: String) -> Option { + pub fn create_equal_with_text(text: String) -> Option { if text.is_empty() { return None; } @@ -127,11 +124,11 @@ where #[cfg(debug_assertions)] debug_assert!( text.as_ref() - .is_none_or(|text| builder.get_slice(self.range()) == *text), + .is_none_or(|text| builder.get_slice_from_remaining(self.len()) == *text), "Text (`{}`) which is supposed to be equal does not match the text in the \ range: `{}`", text.as_ref().unwrap_or(&"".to_owned()), - builder.get_slice(self.range()) + builder.get_slice_from_remaining(self.len()) ); builder.retain(*length) @@ -149,10 +146,10 @@ where debug_assert!( deleted_text .as_ref() - .is_none_or(|text| builder.get_slice(self.range()) == *text), + .is_none_or(|text| builder.get_slice_from_remaining(self.len()) == *text), "Text to-be-deleted `{}` does not match the text in the range: `{}`", deleted_text.as_ref().unwrap_or(&"".to_owned()), - builder.get_slice(self.range()) + builder.get_slice_from_remaining(self.len()) ); builder.delete(*deleted_character_count) @@ -162,7 +159,6 @@ where builder } - /// Returns the number of affected characters. It is always greater than 0 /// because empty operations cannot be created. pub fn len(&self) -> usize { @@ -176,9 +172,6 @@ where } } - - - /// Merges the operation with the given context, producing a new operation /// and updating the context. This implements a comples FSM that handles /// the merging of operations in a way that is consistent with the text. @@ -187,24 +180,24 @@ where pub fn merge_operations_with_context( self, order: usize, - affecting_context: &mut MergeContext, - produced_context: &mut MergeContext, + previous_operation: &mut Option>, other_operation: Option>, - ) -> Option> { - affecting_context.consume_last_operation_if_it_is_too_behind(self.start_index() as i64); - let operation = self.with_shifted_index(affecting_context.shift); - - match (operation, affecting_context.last_operation()) { - (operation @ Operation::Insert { .. }, None | Some(Operation::Equal { .. })) => { - produced_context.shift += operation.len() as i64; - produced_context.consume_and_replace_last_operation(Some(operation.clone())); - Some(operation) - } + ) -> Option> { + println!( + "mergin: {self} (order {order}) - previous: {previous_operation:?} - other: \ + {other_operation:?}" + ); + let operation = self; + match (operation, previous_operation) { ( - Operation::Insert { text, index }, - Some(Operation::Insert { - text: previous_inserted_text, + Operation::Insert { text }, + Some(OrderedOperation { + operation: + Operation::Insert { + text: previous_inserted_text, + .. + }, .. }), ) => { @@ -213,86 +206,54 @@ where // This way, we don't end up duplicating text. let offset_in_tokens = find_longest_prefix_contained_within(previous_inserted_text, &text); - let offset_in_length = text - .iter() - .take(offset_in_tokens) - .map(Token::get_original_length) - .sum::(); - let trimmed_operation = - Operation::create_insert(index, text[offset_in_tokens..].to_vec()); - affecting_context.shift -= offset_in_length as i64; - produced_context.shift += trimmed_operation - .as_ref() - .map(Operation::len) - .unwrap_or_default() as i64; - produced_context.consume_and_replace_last_operation(trimmed_operation.clone()); + let trimmed_operation = Operation::create_insert(text[offset_in_tokens..].to_vec()); - trimmed_operation + trimmed_operation.map(|operation| OrderedOperation { order, operation }) } ( - operation @ Operation::Delete { .. }, - None | Some(Operation::Insert { .. } | Operation::Equal { .. }), + Operation::Delete { + #[cfg(debug_assertions)] + deleted_text, + deleted_character_count, + }, + Some( + last_delete @ OrderedOperation { + operation: Operation::Delete { .. }, + .. + }, + ), ) => { - produced_context.consume_and_replace_last_operation(Some(operation.clone())); - Some(operation) - } + let operation_end_index = order + deleted_character_count; + let last_delete_end_index = last_delete.order + last_delete.operation.len(); - ( - operation @ Operation::Insert { .. }, - Some(last_delete @ Operation::Delete { .. }), - ) => { - produced_context.shift += operation.len() as i64; + let new_length = deleted_character_count + .min(0.max(operation_end_index as i64 - last_delete_end_index as i64) as usize); - debug_assert!( - last_delete.range().contains(&operation.start_index()), - "There is a last delete ({last_delete}) but the operation ({operation}) is \ - not contained in it" + let overlap = deleted_character_count - new_length; + + #[cfg(debug_assertions)] + let updated_delete = deleted_text.as_ref().map_or_else( + || Operation::create_delete(new_length), + |text| { + Operation::create_delete_with_text( + text.chars() + .skip((deleted_character_count - new_length) as usize) + .collect::(), + ) + }, ); - let difference = operation.start_index() as i64 - last_delete.start_index() as i64; + #[cfg(not(debug_assertions))] + let updated_delete = Operation::create_delete(new_length); - let moved_operation = operation.with_index(last_delete.start_index()); - - affecting_context.replace_last_operation(Operation::create_delete( - moved_operation.end_index(), - (last_delete.len() as i64 - difference) as usize, - )); - affecting_context.shift -= difference; - - produced_context.consume_and_replace_last_operation(Some(moved_operation.clone())); - - Some(moved_operation) + updated_delete.map(|operation| OrderedOperation { + order: order + overlap, + operation, + }) } - ( - operation @ Operation::Delete { .. }, - Some(last_delete @ Operation::Delete { .. }), - ) => { - debug_assert!( - last_delete.range().contains(&operation.start_index()), - "There is a last delete ({last_delete}) but the operation ({operation}) is \ - not contained in it" - ); - - let difference = operation.start_index() as i64 - last_delete.start_index() as i64; - - let updated_delete = Operation::create_delete( - last_delete.start_index(), - 0.max(operation.end_index() as i64 - last_delete.end_index() as i64) as usize, - ); - - affecting_context.replace_last_operation(Operation::create_delete( - last_delete.start_index(), - 0.max(last_delete.end_index() as i64 - operation.end_index() as i64) as usize, - )); - affecting_context.shift -= difference; - - produced_context.consume_and_replace_last_operation(updated_delete.clone()); - - updated_delete - } ( ref operation @ Operation::Equal { length, @@ -300,61 +261,68 @@ where ref text, .. }, - Some(last_delete @ Operation::Delete { .. }), + Some( + last_delete @ OrderedOperation { + operation: Operation::Delete { .. }, + .. + }, + ), ) => { - debug_assert!( - last_delete.range().contains(&operation.start_index()), - "There is a last delete ({last_delete}) but the operation ({operation}) is \ - not contained in it" - ); + if let Some(other_operation) = other_operation { + if matches!(other_operation.operation, Operation::Equal { .. }) + && operation.len() == other_operation.operation.len() + && order == other_operation.order + { + println!(" !!!equal to next"); + return Some(OrderedOperation { + order, + operation: Operation::create_equal(0).unwrap(), + }); + } + } - let overlap = (length as i64) - .min(last_delete.end_index() as i64 - operation.start_index() as i64); + let last_delete_end_index = last_delete.order + last_delete.operation.len(); + + let overlap = + 0.max((length as i64).min(last_delete_end_index as i64 - order as i64)); #[cfg(debug_assertions)] - let result = text.as_ref().map_or_else( - || { - Operation::create_equal( - operation.end_index().min(last_delete.end_index()) - 1, - (length as i64 - overlap) as usize, - ) - }, + let updated_equal = text.as_ref().map_or_else( + || Operation::create_equal((length as i64 - overlap) as usize), |text| { Operation::create_equal_with_text( - operation.end_index().min(last_delete.end_index()) - 1, text.chars().skip(overlap as usize).collect::(), ) }, ); #[cfg(not(debug_assertions))] - let result = Operation::create_equal( - operation.end_index().min(last_delete.end_index()), - (length as i64 - overlap) as usize, - ); + let updated_equal = Operation::create_equal((length as i64 - overlap) as usize); - result + updated_equal.map(|operation| OrderedOperation { + order: order + overlap as usize, + operation, + }) } (operation @ Operation::Equal { .. }, _) => { - let result = if let Some(other_operation) = other_operation { + if let Some(other_operation) = other_operation { if matches!(other_operation.operation, Operation::Equal { .. }) && operation.len() == other_operation.operation.len() && order == other_operation.order { println!(" !!!equal to next"); - Operation::create_equal(operation.start_index(), 0) + Operation::create_equal(0) } else { Some(operation) } } else { Some(operation) - }; - - produced_context.consume_and_replace_last_operation(result.clone()); - - result + } + .map(|operation| OrderedOperation { order, operation }) } + + (operation, _) => Some(OrderedOperation { order, operation }), } } } @@ -412,10 +380,7 @@ where )?; #[cfg(not(debug_assertions))] - write!( - f, - "", - )?; + write!(f, "",)?; Ok(()) } @@ -436,13 +401,12 @@ mod tests { use super::*; - #[test] fn test_apply_delete_with_create() { let builder = StringBuilder::new("hello world"); let delete_operation = Operation::<()>::create_delete_with_text("hello ".to_owned()).unwrap(); - let retain_operation = Operation::<()>::create_equal( 5).unwrap(); + let retain_operation = Operation::<()>::create_equal(5).unwrap(); let mut builder = delete_operation.apply(builder); builder = retain_operation.apply(builder); diff --git a/src/operation_transformation/snapshots/reconcile__operation_transformation__edited_text__tests__calculate_operations.snap b/src/operation_transformation/snapshots/reconcile__operation_transformation__edited_text__tests__calculate_operations.snap index cd775b2..dc186e0 100644 --- a/src/operation_transformation/snapshots/reconcile__operation_transformation__edited_text__tests__calculate_operations.snap +++ b/src/operation_transformation/snapshots/reconcile__operation_transformation__edited_text__tests__calculate_operations.snap @@ -8,35 +8,35 @@ EditedText { operations: [ OrderedOperation { order: 0, - operation: , + operation: , }, OrderedOperation { order: 12, - operation: , + operation: , }, OrderedOperation { order: 12, - operation: , + operation: , }, OrderedOperation { order: 13, - operation: , + operation: , }, OrderedOperation { order: 16, - operation: , + operation: , }, OrderedOperation { order: 17, - operation: , + operation: , }, OrderedOperation { order: 20, - operation: , + operation: , }, OrderedOperation { order: 31, - operation: , + operation: , }, ], cursors: [], diff --git a/src/operation_transformation/snapshots/reconcile__operation_transformation__edited_text__tests__calculate_operations_with_no_diff.snap b/src/operation_transformation/snapshots/reconcile__operation_transformation__edited_text__tests__calculate_operations_with_no_diff.snap index 33414f8..4dad79d 100644 --- a/src/operation_transformation/snapshots/reconcile__operation_transformation__edited_text__tests__calculate_operations_with_no_diff.snap +++ b/src/operation_transformation/snapshots/reconcile__operation_transformation__edited_text__tests__calculate_operations_with_no_diff.snap @@ -1,5 +1,5 @@ --- -source: reconcile/src/operation_transformation/edited_text.rs +source: src/operation_transformation/edited_text.rs expression: operations snapshot_kind: text --- @@ -8,15 +8,15 @@ EditedText { operations: [ OrderedOperation { order: 0, - operation: , + operation: , }, OrderedOperation { order: 5, - operation: , + operation: , }, OrderedOperation { order: 6, - operation: , + operation: , }, ], cursors: [], diff --git a/src/utils/find_longest_prefix_contained_within.rs b/src/utils/find_longest_prefix_contained_within.rs index eb4b826..a04da4e 100644 --- a/src/utils/find_longest_prefix_contained_within.rs +++ b/src/utils/find_longest_prefix_contained_within.rs @@ -9,20 +9,20 @@ use crate::Token; /// old: [0, 1, 9, 0, 2, 5] /// new: [9, 0, 2, 5, 1] /// ``` -/// > results in an length of 4 +/// > results in a length of 4 /// /// /// ```not_rust /// old: [0, 1, 9, 0, 2, 5] /// new: [0, 2] /// ``` -/// > results in an length of 2 +/// > results in a length of 2 /// /// ```not_rust /// old: [0, 1, 9, 0, 2, 5] /// new: [0, 4] /// ``` -/// > results in an length of 1 +/// > results in a length of 1 pub fn find_longest_prefix_contained_within(old: &[Token], new: &[Token]) -> usize where T: PartialEq + Clone + std::fmt::Debug, diff --git a/src/utils/string_builder.rs b/src/utils/string_builder.rs index 8e37b79..63aebdc 100644 --- a/src/utils/string_builder.rs +++ b/src/utils/string_builder.rs @@ -55,18 +55,14 @@ impl StringBuilder<'_> { pub fn build(self) -> String { self.buffer } #[cfg(debug_assertions)] - /// Get a slice of the built string and the remaining original string. - /// The implementation is quite suboptimal but it's only used for debugging. - pub fn get_slice(&self, range: Range) -> String { - let result = self - .buffer - .chars() - .chain(self.remaining.chars()) - .skip(range.start) - .take(range.end - range.start) - .collect::(); + /// Get a slice of the remaining original string. The slice starts from + /// where the next delete/retain operation would start and is of length + /// `length`. The implementation is quite suboptimal but it's only used + /// for debugging. + pub fn get_slice_from_remaining(&self, length: usize) -> String { + let result = self.remaining.chars().take(length).collect::(); - debug_assert_eq!(result.chars().count(), range.len(), "Range out of bounds",); + debug_assert_eq!(result.chars().count(), length, "Range out of bounds"); result } @@ -127,12 +123,12 @@ mod tests { let builder = StringBuilder::new(original); // Test getting a slice of the original string - assert_eq!(builder.get_slice(1..4), "bcd"); + assert_eq!(builder.get_slice_from_remaining(3), "abc"); // Test getting a slice that includes both buffer and remaining original let mut builder = StringBuilder::new(original); builder.retain(2); // "ab" in buffer - assert_eq!(builder.get_slice(1..5), "bcde"); + assert_eq!(builder.get_slice_from_remaining(2), "cd"); } #[test]