From 351caf74b50deae6dacc80d4b3efdd50acc40227 Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sat, 21 Jun 2025 10:50:35 +0100 Subject: [PATCH 01/15] Remove a few index uses --- src/operation_transformation/edited_text.rs | 136 ++++++++++++-------- tests/examples/deletes.yml | 6 + 2 files changed, 89 insertions(+), 53 deletions(-) diff --git a/src/operation_transformation/edited_text.rs b/src/operation_transformation/edited_text.rs index bd9a15d..236e4c5 100644 --- a/src/operation_transformation/edited_text.rs +++ b/src/operation_transformation/edited_text.rs @@ -1,14 +1,14 @@ #[cfg(feature = "serde")] use serde::{Deserialize, Serialize}; -use super::{CursorPosition, Operation, TextWithCursors, ordered_operation::OrderedOperation}; +use super::{ordered_operation::OrderedOperation, CursorPosition, Operation, TextWithCursors}; use crate::{ diffs::{myers::diff, raw_operation::RawOperation}, operation_transformation::{ merge_context::MergeContext, utils::{cook_operations::cook_operations, elongate_operations::elongate_operations}, }, - tokenizer::{Tokenizer, word_tokenizer::word_tokenizer}, + tokenizer::{word_tokenizer::word_tokenizer, Tokenizer}, utils::{side::Side, string_builder::StringBuilder}, }; @@ -82,18 +82,6 @@ where operations: Vec>, mut cursors: Vec, ) -> Self { - operations - .iter() - .zip(operations.iter().skip(1)) - .for_each(|(previous, next)| { - debug_assert!( - previous.operation.start_index() <= next.operation.start_index(), - "{} must not come before {} yet it does", - previous.operation, - next.operation - ); - }); - cursors.sort_by_key(|cursor| cursor.char_index); Self { @@ -126,6 +114,10 @@ where let mut maybe_left_op = left_iter.next(); let mut maybe_right_op = right_iter.next(); + let mut seen_left_length: usize = 0; + let mut seen_right_length: usize = 0; + let mut merged_length: usize = 0; + loop { let (side, OrderedOperation { operation, order }) = match (maybe_left_op.clone(), maybe_right_op.clone()) { @@ -142,58 +134,96 @@ where (None, None) => break, }; - if side == Side::Left { - maybe_left_op = left_iter.next(); - } else { - maybe_right_op = right_iter.next(); - } + let is_advancing_operation = matches!( + operation, + Operation::Insert { .. } | Operation::Equal { .. } + ); + println!(" {operation:?}"); - let original_start = operation.start_index() as i64; - let original_end = operation.end_index(); let original_length = operation.len() as i64; - let result = match side { - Side::Left => operation.merge_operations_with_context( - &mut right_merge_context, - &mut left_merge_context, - ), - Side::Right => operation.merge_operations_with_context( - &mut left_merge_context, - &mut right_merge_context, - ), - }; + Side::Left => { + let are_equal = matches!(operation, Operation::Equal { .. }) + && maybe_right_op + .as_ref() + .map(|op| { + matches!(op.operation, Operation::Equal { .. }) + && op.operation.eq(&operation) + }) + .unwrap_or_default(); - if let Some(ref op @ (Operation::Insert { .. } | Operation::Equal { .. })) = result { - let shift = - op.start_index() as i64 - original_start + op.len() as i64 - original_length; - match side { - Side::Left => { - while let Some(cursor) = - left_cursors.next_if(|cursor| cursor.char_index <= original_end + 1) - { + let result = operation.merge_operations_with_context( + &mut right_merge_context, + &mut left_merge_context, + ); + + if let Some(ref op @ (Operation::Insert { .. } | Operation::Equal { .. })) = + result + { + let shift = op.start_index() as i64 - seen_left_length as i64 + + op.len() as i64 + - original_length; + + while let Some(cursor) = left_cursors.next_if(|cursor| { + cursor.char_index <= seen_left_length + original_length as usize + }) { merged_cursors.push(cursor.with_index( (op.start_index() as i64).max(cursor.char_index as i64 + shift) as usize, )); } } - Side::Right => { - while let Some(cursor) = - right_cursors.next_if(|cursor| cursor.char_index <= original_end + 1) - { - merged_cursors.push(cursor.with_index( - (op.start_index() as i64).max(cursor.char_index as i64 + shift) - as usize, - )); - } + + if is_advancing_operation { + seen_left_length += original_length as usize; + } + + maybe_left_op = left_iter.next(); + + if are_equal { + None + } else { + result } } - } + Side::Right => { + let result = operation.merge_operations_with_context( + &mut left_merge_context, + &mut right_merge_context, + ); - merged_operations.extend(result.into_iter().map(|op| OrderedOperation { - order, - operation: op, - })); + if let Some(ref op @ (Operation::Insert { .. } | Operation::Equal { .. })) = + result + { + println!("merged_length {merged_length}, idx {}", op.start_index()); + let shift = op.start_index() as i64 - seen_right_length as i64 + + op.len() as i64 + - original_length; + + while let Some(cursor) = right_cursors.next_if(|cursor| { + cursor.char_index <= seen_right_length + original_length as usize + }) { + merged_cursors.push(cursor.with_index( + (op.start_index() as i64).max(cursor.char_index as i64 + shift) + as usize, + )); + } + } + + if is_advancing_operation { + seen_right_length += original_length as usize; + } + + maybe_right_op = right_iter.next(); + + result + } + }; + + if let Some(operation) = result { + merged_length += operation.len() as usize; + merged_operations.push(OrderedOperation { order, operation }); + } } let last_index = merged_operations diff --git a/tests/examples/deletes.yml b/tests/examples/deletes.yml index c4c145b..d8c2819 100644 --- a/tests/examples/deletes.yml +++ b/tests/examples/deletes.yml @@ -24,6 +24,12 @@ left: long small right: long with big and small expected: long small +--- +parent: long run of text where one barely has no changes but has cursors +left: long| run of tex|t where one barely has no |changes but has |cursors +right: long run one barely has no changes cursors +expected: long| run| one barely has no |changes |cursors + --- parent: long text where the cursor has to be clamped after delete left: long text where the cursor has to be clamped after delete| -- 2.47.2 From 1e974c4c9a4d8ed00f5a155a309559647fcfe3c0 Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sat, 21 Jun 2025 10:52:45 +0100 Subject: [PATCH 02/15] Format --- src/operation_transformation/ordered_operation.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/operation_transformation/ordered_operation.rs b/src/operation_transformation/ordered_operation.rs index 6a668e2..ac4b0d6 100644 --- a/src/operation_transformation/ordered_operation.rs +++ b/src/operation_transformation/ordered_operation.rs @@ -1,7 +1,7 @@ #[cfg(feature = "serde")] use serde::{Deserialize, Serialize}; -use crate::operation_transformation::Operation; +use crate::{Token, operation_transformation::Operation}; #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] #[derive(Debug, Clone, PartialEq)] @@ -25,10 +25,9 @@ where // is left or right. match &self.operation { Operation::Equal { index, .. } => index.to_string(), - Operation::Insert { text, .. } => text - .iter() - .map(crate::tokenizer::token::Token::original) - .collect::(), + Operation::Insert { text, .. } => { + text.iter().map(Token::original).collect::() + } Operation::Delete { deleted_character_count, .. -- 2.47.2 From 5afb2c21f83fe56f5741d67a791483a4e2f6a354 Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sat, 21 Jun 2025 13:18:02 +0100 Subject: [PATCH 03/15] Remove indices from string builder --- src/operation_transformation/operation.rs | 85 +++++++++--- src/utils/string_builder.rs | 159 ++++++++++++++-------- 2 files changed, 171 insertions(+), 73 deletions(-) diff --git a/src/operation_transformation/operation.rs b/src/operation_transformation/operation.rs index e194f9c..827da77 100644 --- a/src/operation_transformation/operation.rs +++ b/src/operation_transformation/operation.rs @@ -15,7 +15,7 @@ use crate::{ /// Represents a change that can be applied on a `StringBuilder`. #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] -#[derive(Clone, PartialEq)] +#[derive(Clone)] pub enum Operation where T: PartialEq + Clone + std::fmt::Debug, @@ -42,6 +42,40 @@ where }, } +impl PartialEq for Operation +where + T: PartialEq + Clone + std::fmt::Debug, +{ + fn eq(&self, other: &Self) -> bool { + match (self, other) { + ( + Operation::Equal { length, .. }, + Operation::Equal { + length: other_length, + .. + }, + ) => length == other_length, + ( + Operation::Insert { text, .. }, + Operation::Insert { + text: other_text, .. + }, + ) => text == other_text, + ( + Operation::Delete { + deleted_character_count, + .. + }, + Operation::Delete { + deleted_character_count: other_deleted_character_count, + .. + }, + ) => deleted_character_count == other_deleted_character_count, + _ => false, + } + } +} + impl Operation where T: PartialEq + Clone + std::fmt::Debug, @@ -129,24 +163,28 @@ where Operation::Equal { #[cfg(debug_assertions)] text, + length, .. } => { #[cfg(debug_assertions)] debug_assert!( text.as_ref() .is_none_or(|text| builder.get_slice(self.range()) == *text), - "Text which is supposed to be equal does not match the text in the range" + "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()) ); - return builder; + builder.retain(*length) + } + Operation::Insert { text, .. } => { + builder.insert(&text.iter().map(Token::original).collect::()) } - Operation::Insert { text, .. } => builder.insert( - self.start_index(), - &text.iter().map(Token::original).collect::(), - ), Operation::Delete { #[cfg(debug_assertions)] deleted_text, + deleted_character_count, .. } => { #[cfg(debug_assertions)] @@ -154,10 +192,12 @@ where deleted_text .as_ref() .is_none_or(|text| builder.get_slice(self.range()) == *text), - "Text to delete does not match the text in the range" + "Text to delete (`{}`) does not match the text in the range: {}", + deleted_text.as_ref().unwrap_or(&"".to_owned()), + builder.get_slice(self.range()) ); - builder.delete(self.range()); + builder.delete(*deleted_character_count) } } @@ -424,7 +464,7 @@ where f, "", text.as_ref() - .map(|text| format!("'{text}'")) + .map(|text| format!("'{}'", text.replace('\n', "\\n"))) .unwrap_or(format!("{length} characters")), index )?; @@ -438,7 +478,10 @@ where write!( f, "", - text.iter().map(Token::original).collect::(), + text.iter() + .map(Token::original) + .collect::() + .replace('\n', "\\n"), index ) } @@ -455,7 +498,7 @@ where "", deleted_text .as_ref() - .map(|text| format!("'{text}'")) + .map(|text| format!("'{}'", text.replace('\n', "\\n"))) .unwrap_or(format!("{deleted_character_count} characters")), index )?; @@ -498,16 +541,26 @@ mod tests { #[test] fn test_apply_delete_with_create() { let builder = StringBuilder::new("hello world"); - let operation = Operation::<()>::create_delete_with_text(5, " world".to_owned()).unwrap(); + let delete_operation = + Operation::<()>::create_delete_with_text(0, "hello ".to_owned()).unwrap(); + let retain_operation = Operation::<()>::create_equal(5, 5).unwrap(); - assert_eq!(operation.apply(builder).build(), "hello"); + let mut builder = delete_operation.apply(builder); + builder = retain_operation.apply(builder); + + assert_eq!(builder.build(), "world"); } #[test] fn test_apply_insert() { let builder = StringBuilder::new("hello"); - let operation = Operation::create_insert(5, vec![" my friend".into()]).unwrap(); - assert_eq!(operation.apply(builder).build(), "hello my friend"); + let retain_operation = Operation::<()>::create_equal(5, 5).unwrap(); + let insert_operation = Operation::create_insert(5, vec![" my friend".into()]).unwrap(); + + let mut builder = retain_operation.apply(builder); + builder = insert_operation.apply(builder); + + assert_eq!(builder.build(), "hello my friend"); } } diff --git a/src/utils/string_builder.rs b/src/utils/string_builder.rs index b19bcbb..8e37b79 100644 --- a/src/utils/string_builder.rs +++ b/src/utils/string_builder.rs @@ -1,78 +1,67 @@ use core::ops::Range; +use std::iter::Iterator; -/// A helper for building a string in order based on an original string and a -/// series of insertions and deletions applied to it. It is safe to use with -/// UTF-8 strings as all operations are based on character indices. -#[derive(Debug, Clone)] +/// A helper for building a string in-order based on an original string and a +/// series of insertions, deletions, and copies applied to it. It is safe to use +/// with UTF-8 strings as all operations are based on character indices. The +/// methods must be called in-order. pub struct StringBuilder<'a> { - original: &'a str, - last_old_char_index: usize, + original: Box + 'a>, buffer: String, + + #[cfg(debug_assertions)] + remaining: String, } impl StringBuilder<'_> { pub fn new(original: &str) -> StringBuilder<'_> { StringBuilder { - original, - last_old_char_index: 0, + original: Box::new(original.chars()), buffer: String::with_capacity(original.len()), + + #[cfg(debug_assertions)] + remaining: original.to_string(), } } - /// Insert a string at the given index after copying the original string up - /// to that index from the last insertion or deletion. - pub fn insert(&mut self, from: usize, text: &str) { - self.copy_until(from); - self.buffer.push_str(text); + /// Insert a string at the end of the built buffer. + pub fn insert(&mut self, text: &str) { self.buffer.push_str(text); } + + /// Skip copying `length` characters from the original string to the built + /// buffer. + pub fn delete(&mut self, length: usize) { + if length == 0 { + return; + } + + self.original.nth(length - 1); + + if cfg!(debug_assertions) { + self.remaining = self.remaining.chars().skip(length).collect(); + } } - /// Delete a string at the given index after copying the original string up - /// to that index from the last insertion or deletion. - pub fn delete(&mut self, range: core::ops::Range) { - self.copy_until(range.start); - self.last_old_char_index += range.len(); - } + /// Copy `length` characters from the original string to the built buffer. + pub fn retain(&mut self, length: usize) { + self.buffer.extend(self.original.by_ref().take(length)); - fn copy_until(&mut self, index: usize) { - let current_char_count = self.buffer.chars().count(); - debug_assert!( - index >= current_char_count, - "String builder only support building in order" - ); - - let jump = index - current_char_count; - - self.buffer.push_str( - &self - .original - .chars() - .skip(self.last_old_char_index) - .take(jump) - .collect::(), - ); - self.last_old_char_index += jump; + if cfg!(debug_assertions) { + self.remaining = self.remaining.chars().skip(length).collect(); + } } /// Finish building the string after copying the remaining original string /// since the last insertion or deletion. - pub fn build(mut self) -> String { - self.buffer.push_str( - &self - .original - .chars() - .skip(self.last_old_char_index) - .collect::(), - ); + pub fn build(self) -> String { self.buffer } - self.buffer - } - - #[allow(dead_code)] + #[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.original.chars().skip(self.last_old_char_index)) + .chain(self.remaining.chars()) .skip(range.start) .take(range.end - range.start) .collect::(); @@ -85,6 +74,8 @@ impl StringBuilder<'_> { #[cfg(test)] mod tests { + use pretty_assertions::assert_eq; + use super::*; #[test] @@ -92,20 +83,74 @@ mod tests { let original = "aaa bbb ccc"; let mut builder = StringBuilder::new(original); - builder.insert(0, "ddd "); - builder.delete(4..8); - builder.insert(11, " eee"); + builder.insert("ddd"); + builder.delete(3); + builder.retain(8); + builder.insert(" eee"); assert_eq!(builder.build(), "ddd bbb ccc eee"); - } - #[test] - fn test_string_builder2() { let original = "abcde"; let mut builder = StringBuilder::new(original); - builder.delete(1..4); + builder.retain(1); + builder.delete(3); + builder.retain(1); assert_eq!(builder.build(), "ae"); } + + #[test] + fn test_empty_original() { + let original = ""; + let mut builder = StringBuilder::new(original); + + builder.insert("test"); + assert_eq!(builder.build(), "test"); + } + + #[test] + fn test_unicode_characters() { + let original = "こんにちは"; + let mut builder = StringBuilder::new(original); + + builder.retain(3); + builder.insert("世界, "); // Insert "World, " + builder.retain(2); + + assert_eq!(builder.build(), "こんに世界, ちは"); + } + + #[test] + fn test_get_slice() { + let original = "abcdef"; + let builder = StringBuilder::new(original); + + // Test getting a slice of the original string + assert_eq!(builder.get_slice(1..4), "bcd"); + + // 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"); + } + + #[test] + fn test_retain_all() { + let original = "Hello, world!"; + let mut builder = StringBuilder::new(original); + + builder.retain(original.len()); + assert_eq!(builder.build(), original); + } + + #[test] + fn test_delete_all() { + let original = "Hello"; + let mut builder = StringBuilder::new(original); + + builder.delete(original.len()); + builder.insert("Hi"); + assert_eq!(builder.build(), "Hi"); + } } -- 2.47.2 From 7589e1bf2e5fa0fd6acf043637039149a6921d1a Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sat, 21 Jun 2025 13:18:13 +0100 Subject: [PATCH 04/15] Sort deletes first --- ...__edited_text__tests__calculate_operations.snap | 14 +++++++------- .../utils/elongate_operations.rs | 8 ++++---- 2 files changed, 11 insertions(+), 11 deletions(-) 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 246b2fe..cd775b2 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 @@ -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,11 +8,11 @@ EditedText { operations: [ OrderedOperation { order: 0, - operation: , + operation: , }, OrderedOperation { - order: 0, - operation: , + order: 12, + operation: , }, OrderedOperation { order: 12, @@ -32,11 +32,11 @@ EditedText { }, OrderedOperation { order: 20, - operation: , + operation: , }, OrderedOperation { - order: 20, - operation: , + order: 31, + operation: , }, ], cursors: [], diff --git a/src/operation_transformation/utils/elongate_operations.rs b/src/operation_transformation/utils/elongate_operations.rs index 9883791..212ba7e 100644 --- a/src/operation_transformation/utils/elongate_operations.rs +++ b/src/operation_transformation/utils/elongate_operations.rs @@ -41,20 +41,20 @@ where } }, RawOperation::Equal(..) => Box::new( - maybe_previous_insert + maybe_previous_delete .take() .into_iter() - .chain(maybe_previous_delete.take()) + .chain(maybe_previous_insert.take()) .chain(iter::once(next)), ) as Box>>, }) .collect(); - if let Some(prev) = maybe_previous_insert { + if let Some(prev) = maybe_previous_delete { result.push(prev); } - if let Some(prev) = maybe_previous_delete { + if let Some(prev) = maybe_previous_insert { result.push(prev); } -- 2.47.2 From 2089cbc7aa680a4350f920fc6890652c521c3141 Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sat, 21 Jun 2025 13:58:06 +0100 Subject: [PATCH 05/15] Fix with new string builder --- README.md | 25 +++--- src/operation_transformation.rs | 2 +- src/operation_transformation/edited_text.rs | 59 +++++++------- src/operation_transformation/merge_context.rs | 26 +++--- src/operation_transformation/operation.rs | 80 +++++++++---------- .../ordered_operation.rs | 9 ++- tests/examples/various.yml | 2 +- 7 files changed, 101 insertions(+), 102 deletions(-) diff --git a/README.md b/README.md index 4735cf5..a3d3d7f 100644 --- a/README.md +++ b/README.md @@ -1,26 +1,22 @@ # VaultLink self-hosted Obsidian plugin for file syncing -[![Check](https://github.com/schmelczer/vault-link/actions/workflows/check.yml/badge.svg)](https://github.com/schmelczer/vault-link/actions/workflows/check.yml) -[![E2E tests](https://github.com/schmelczer/vault-link/actions/workflows/e2e.yml/badge.svg)](https://github.com/schmelczer/vault-link/actions/workflows/e2e.yml) -[![Publish server Docker image](https://github.com/schmelczer/vault-link/actions/workflows/publish-docker.yml/badge.svg)](https://github.com/schmelczer/vault-link/actions/workflows/publish-docker.yml) -[![Publish Obsidian plugin](https://github.com/schmelczer/vault-link/actions/workflows/publish-plugin.yml/badge.svg)](https://github.com/schmelczer/vault-link/actions/workflows/publish-plugin.yml) - +[![Check](https://github.com/schmelczer/reconcile/actions/workflows/check.yml/badge.svg)](https://github.com/schmelczer/reconcile/actions/workflows/check.yml) +[![Publish to GitHub Pages](https://github.com/schmelczer/reconcile/actions/workflows/gh-pages.yml/badge.svg)](https://github.com/schmelczer/reconcile/actions/workflows/gh-pages.yml) ## Develop ### Install [nvm](https://github.com/nvm-sh/nvm) -- `curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.40.1/install.sh | bash` -- `nvm install 22` -- `nvm use 22` -- Optionally set the system-wide default: `nvm alias default 22` - +- `curl -o- https://raw.githubusercontent.com/nvm-sh/nvm/v0.40.1/install.sh | bash` +- `nvm install 22` +- `nvm use 22` +- Optionally set the system-wide default: `nvm alias default 22` ### Set up Rust -- Install [`rustup`](https://rustup.rs): `curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh` -- Install [`wasm-pack`](https://rustwasm.github.io/wasm-pack/installer): `curl https://rustwasm.github.io/wasm-pack/installer/init.sh -sSf | sh` -- `cargo install cargo-insta sqlx-cli cargo-edit` +- Install [`rustup`](https://rustup.rs): `curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh` +- Install [`wasm-pack`](https://rustwasm.github.io/wasm-pack/installer): `curl https://rustwasm.github.io/wasm-pack/installer/init.sh -sSf | sh` +- `cargo install cargo-insta sqlx-cli cargo-edit` ### Install Obsidian on Linux @@ -45,7 +41,6 @@ scripts/update-api-types.sh scripts/bump-version.sh patch ``` - #### Run E2E tests ```sh @@ -56,4 +51,4 @@ And to clean up the logs & database files, run `scripts/clean-up.sh` ## Projects -- [Sync server](./backend/sync_server/README.md) +- [Sync server](./backend/sync_server/README.md) diff --git a/src/operation_transformation.rs b/src/operation_transformation.rs index fc280ec..9fd5a5d 100644 --- a/src/operation_transformation.rs +++ b/src/operation_transformation.rs @@ -111,7 +111,7 @@ mod test { }, // inside of "s|ample" because "text" got replaced by "sample" CursorPosition { id: 3, - char_index: 43 + char_index: 42 }, // before "cursor movements" ] ) diff --git a/src/operation_transformation/edited_text.rs b/src/operation_transformation/edited_text.rs index 236e4c5..4548a41 100644 --- a/src/operation_transformation/edited_text.rs +++ b/src/operation_transformation/edited_text.rs @@ -1,14 +1,14 @@ #[cfg(feature = "serde")] use serde::{Deserialize, Serialize}; -use super::{ordered_operation::OrderedOperation, CursorPosition, Operation, TextWithCursors}; +use super::{CursorPosition, Operation, TextWithCursors, ordered_operation::OrderedOperation}; use crate::{ diffs::{myers::diff, raw_operation::RawOperation}, operation_transformation::{ merge_context::MergeContext, utils::{cook_operations::cook_operations, elongate_operations::elongate_operations}, }, - tokenizer::{word_tokenizer::word_tokenizer, Tokenizer}, + tokenizer::{Tokenizer, word_tokenizer::word_tokenizer}, utils::{side::Side, string_builder::StringBuilder}, }; @@ -93,6 +93,7 @@ where #[must_use] pub fn merge(self, other: Self) -> Self { + println!("\n\n\n\n\n\n----\n"); debug_assert_eq!( self.text, other.text, "`EditedText`-s must be derived from the same text to be mergable" @@ -138,20 +139,11 @@ where operation, Operation::Insert { .. } | Operation::Equal { .. } ); - println!(" {operation:?}"); + println!("{side} {operation:?} ({maybe_left_op:?}, {maybe_right_op:?})"); let original_length = operation.len() as i64; let result = match side { Side::Left => { - let are_equal = matches!(operation, Operation::Equal { .. }) - && maybe_right_op - .as_ref() - .map(|op| { - matches!(op.operation, Operation::Equal { .. }) - && op.operation.eq(&operation) - }) - .unwrap_or_default(); - let result = operation.merge_operations_with_context( &mut right_merge_context, &mut left_merge_context, @@ -160,6 +152,8 @@ where if let Some(ref op @ (Operation::Insert { .. } | Operation::Equal { .. })) = result { + println!("merged_length {merged_length}, idx {}", op.start_index()); + // assert_eq!(op.start_index() as i64, merged_length as i64); let shift = op.start_index() as i64 - seen_left_length as i64 + op.len() as i64 - original_length; @@ -180,11 +174,7 @@ where maybe_left_op = left_iter.next(); - if are_equal { - None - } else { - result - } + result } Side::Right => { let result = operation.merge_operations_with_context( @@ -196,6 +186,8 @@ where result { println!("merged_length {merged_length}, idx {}", op.start_index()); + // assert_eq!(op.start_index() as i64, merged_length as i64); + let shift = op.start_index() as i64 - seen_right_length as i64 + op.len() as i64 - original_length; @@ -220,25 +212,30 @@ where } }; + println!(" = {result:?}"); + if let Some(operation) = result { - merged_length += operation.len() as usize; + let last_operation = merged_operations.last(); + if let Some(last_operation) = last_operation { + if matches!(last_operation.operation, Operation::Equal { .. }) + && matches!(operation, Operation::Equal { .. }) + && last_operation.operation.eq(&operation) + { + println!("skipping equal"); + continue; + } + } + + if is_advancing_operation { + merged_length += operation.len(); + } + merged_operations.push(OrderedOperation { order, operation }); } } - let last_index = merged_operations - .iter() - .filter(|operation| { - matches!( - operation.operation, - Operation::Insert { .. } | Operation::Equal { .. } - ) - }) - .next_back() - .map_or(0, |op| op.operation.end_index()); - for cursor in left_cursors.chain(right_cursors) { - merged_cursors.push(cursor.with_index(last_index)); + merged_cursors.push(cursor.with_index(merged_length)); } Self::new(self.text, merged_operations, merged_cursors) @@ -250,6 +247,8 @@ where let mut builder: StringBuilder<'_> = StringBuilder::new(self.text); for OrderedOperation { operation, .. } in &self.operations { + println!("applying operation {operation:?}"); + builder = operation.apply(builder); } diff --git a/src/operation_transformation/merge_context.rs b/src/operation_transformation/merge_context.rs index 5cf0972..e397226 100644 --- a/src/operation_transformation/merge_context.rs +++ b/src/operation_transformation/merge_context.rs @@ -52,22 +52,24 @@ where /// threshold operation. This updates the `shift` in case the last operation /// was a delete. pub fn consume_last_operation_if_it_is_too_behind(&mut self, threshold_index: i64) { - if let Some(last_operation) = self.last_operation.as_ref() { - if let Operation::Delete { - deleted_character_count, - .. - } = last_operation - { - if threshold_index + self.shift > last_operation.end_index() as i64 { + match self.last_operation.as_ref() { + Some( + op @ Operation::Delete { + deleted_character_count, + .. + }, + ) => { + if threshold_index + self.shift > op.end_index() as i64 { self.shift -= *deleted_character_count as i64; self.last_operation = None; } - } else if let Operation::Insert { .. } = last_operation - && threshold_index + self.shift - last_operation.len() as i64 - > last_operation.end_index() as i64 - { - self.last_operation = None; } + Some(op @ Operation::Insert { .. }) | Some(op @ Operation::Equal { .. }) => { + if threshold_index + self.shift - op.len() as i64 > op.end_index() as i64 { + self.last_operation = None; + } + } + _ => {} } } } diff --git a/src/operation_transformation/operation.rs b/src/operation_transformation/operation.rs index 827da77..862a5b6 100644 --- a/src/operation_transformation/operation.rs +++ b/src/operation_transformation/operation.rs @@ -6,16 +6,16 @@ use serde::{Deserialize, Serialize}; use super::merge_context::MergeContext; use crate::{ - Token, utils::{ find_longest_prefix_contained_within::find_longest_prefix_contained_within, string_builder::StringBuilder, }, + Token, }; /// Represents a change that can be applied on a `StringBuilder`. #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] -#[derive(Clone)] +#[derive(Clone, PartialEq)] pub enum Operation where T: PartialEq + Clone + std::fmt::Debug, @@ -42,39 +42,39 @@ where }, } -impl PartialEq for Operation -where - T: PartialEq + Clone + std::fmt::Debug, -{ - fn eq(&self, other: &Self) -> bool { - match (self, other) { - ( - Operation::Equal { length, .. }, - Operation::Equal { - length: other_length, - .. - }, - ) => length == other_length, - ( - Operation::Insert { text, .. }, - Operation::Insert { - text: other_text, .. - }, - ) => text == other_text, - ( - Operation::Delete { - deleted_character_count, - .. - }, - Operation::Delete { - deleted_character_count: other_deleted_character_count, - .. - }, - ) => deleted_character_count == other_deleted_character_count, - _ => false, - } - } -} +// impl PartialEq for Operation +// where +// T: PartialEq + Clone + std::fmt::Debug, +// { +// fn eq(&self, other: &Self) -> bool { +// match (self, other) { +// ( +// Operation::Equal { length, .. }, +// Operation::Equal { +// length: other_length, +// .. +// }, +// ) => length == other_length, +// ( +// Operation::Insert { text, .. }, +// Operation::Insert { +// text: other_text, .. +// }, +// ) => text == other_text, +// ( +// Operation::Delete { +// deleted_character_count, +// .. +// }, +// Operation::Delete { +// deleted_character_count: other_deleted_character_count, +// .. +// }, +// ) => deleted_character_count == other_deleted_character_count, +// _ => false, +// } +// } +// } impl Operation where @@ -192,7 +192,7 @@ where deleted_text .as_ref() .is_none_or(|text| builder.get_slice(self.range()) == *text), - "Text to delete (`{}`) does not match the text in the range: {}", + "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()) ); @@ -531,11 +531,9 @@ mod tests { #[test] #[should_panic(expected = "Shifted index must be non-negative")] fn test_shifting_error() { - insta::assert_debug_snapshot!( - Operation::create_insert(1, vec!["hi".into()]) - .unwrap() - .with_shifted_index(-2) - ); + insta::assert_debug_snapshot!(Operation::create_insert(1, vec!["hi".into()]) + .unwrap() + .with_shifted_index(-2)); } #[test] diff --git a/src/operation_transformation/ordered_operation.rs b/src/operation_transformation/ordered_operation.rs index ac4b0d6..c9c0b67 100644 --- a/src/operation_transformation/ordered_operation.rs +++ b/src/operation_transformation/ordered_operation.rs @@ -1,7 +1,7 @@ #[cfg(feature = "serde")] use serde::{Deserialize, Serialize}; -use crate::{Token, operation_transformation::Operation}; +use crate::{operation_transformation::Operation, Token}; #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] #[derive(Debug, Clone, PartialEq)] @@ -17,9 +17,14 @@ impl OrderedOperation where T: PartialEq + Clone + std::fmt::Debug, { - pub fn get_sort_key(&self) -> (usize, usize, String) { + pub fn get_sort_key(&self) -> (usize, usize, usize, String) { ( self.order, + match &self.operation { + Operation::Delete { .. } => 1, + Operation::Insert { .. } => 2, + Operation::Equal { .. } => 3, + }, self.operation.start_index(), // Make sure that the ordering is deterministic regardless of which text // is left or right. diff --git a/tests/examples/various.yml b/tests/examples/various.yml index cfbeb42..7a87e4e 100644 --- a/tests/examples/various.yml +++ b/tests/examples/various.yml @@ -67,7 +67,7 @@ expected: market| placemarket|space parent: A B C D left: A X B D| right: A B Y| -expected: A X B |Y| +expected: A X B Y|| --- parent: Please submit your assignment by Friday -- 2.47.2 From d58b474669ee826a7247bc112e504334fe4693d8 Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sun, 22 Jun 2025 09:24:36 +0100 Subject: [PATCH 06/15] DOn't use start_index in EditedText --- src/operation_transformation/edited_text.rs | 48 +++++++-------- src/operation_transformation/merge_context.rs | 4 +- src/operation_transformation/operation.rs | 60 ++++++++++++------- .../ordered_operation.rs | 2 +- 4 files changed, 61 insertions(+), 53 deletions(-) diff --git a/src/operation_transformation/edited_text.rs b/src/operation_transformation/edited_text.rs index 4548a41..56254a0 100644 --- a/src/operation_transformation/edited_text.rs +++ b/src/operation_transformation/edited_text.rs @@ -120,18 +120,18 @@ where let mut merged_length: usize = 0; loop { - let (side, OrderedOperation { operation, order }) = + 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 < right_op { - (Side::Left, left_op) + (Side::Left, left_op, maybe_right_op.clone()) } else { - (Side::Right, right_op) + (Side::Right, right_op, maybe_left_op.clone()) } } - (Some(left_op), None) => (Side::Left, left_op), - (None, Some(right_op)) => (Side::Right, right_op), + (Some(left_op), None) => (Side::Left, left_op, None), + (None, Some(right_op)) => (Side::Right, right_op, None), (None, None) => break, }; @@ -145,24 +145,25 @@ where let result = match side { Side::Left => { let result = operation.merge_operations_with_context( + order, &mut right_merge_context, &mut left_merge_context, + maybe_other_operation, ); if let Some(ref op @ (Operation::Insert { .. } | Operation::Equal { .. })) = result { - println!("merged_length {merged_length}, idx {}", op.start_index()); - // assert_eq!(op.start_index() as i64, merged_length as i64); - let shift = op.start_index() as i64 - seen_left_length as i64 - + op.len() as i64 - - original_length; + let mut shift = merged_length as i64 - seen_left_length as i64; + if !matches!(op, Operation::Equal { .. }) { + shift += op.len() as i64 - original_length; + } while let Some(cursor) = left_cursors.next_if(|cursor| { cursor.char_index <= seen_left_length + original_length as usize }) { merged_cursors.push(cursor.with_index( - (op.start_index() as i64).max(cursor.char_index as i64 + shift) + (merged_length as i64).max(cursor.char_index as i64 + shift) as usize, )); } @@ -178,25 +179,25 @@ where } Side::Right => { let result = operation.merge_operations_with_context( + order, &mut left_merge_context, &mut right_merge_context, + maybe_other_operation, ); if let Some(ref op @ (Operation::Insert { .. } | Operation::Equal { .. })) = result { - println!("merged_length {merged_length}, idx {}", op.start_index()); - // assert_eq!(op.start_index() as i64, merged_length as i64); - - let shift = op.start_index() as i64 - seen_right_length as i64 - + op.len() as i64 - - original_length; + let mut shift = merged_length as i64 - seen_right_length as i64; + if !matches!(op, Operation::Equal { .. }) { + shift += op.len() as i64 - original_length; + } while let Some(cursor) = right_cursors.next_if(|cursor| { cursor.char_index <= seen_right_length + original_length as usize }) { merged_cursors.push(cursor.with_index( - (op.start_index() as i64).max(cursor.char_index as i64 + shift) + (merged_length as i64).max(cursor.char_index as i64 + shift) as usize, )); } @@ -215,15 +216,8 @@ where println!(" = {result:?}"); if let Some(operation) = result { - let last_operation = merged_operations.last(); - if let Some(last_operation) = last_operation { - if matches!(last_operation.operation, Operation::Equal { .. }) - && matches!(operation, Operation::Equal { .. }) - && last_operation.operation.eq(&operation) - { - println!("skipping equal"); - continue; - } + if operation.len() == 0 { + continue; } if is_advancing_operation { diff --git a/src/operation_transformation/merge_context.rs b/src/operation_transformation/merge_context.rs index e397226..b40d2f3 100644 --- a/src/operation_transformation/merge_context.rs +++ b/src/operation_transformation/merge_context.rs @@ -59,13 +59,13 @@ where .. }, ) => { - if threshold_index + self.shift > op.end_index() as i64 { + if threshold_index + self.shift >= op.end_index() as i64 { self.shift -= *deleted_character_count as i64; self.last_operation = None; } } Some(op @ Operation::Insert { .. }) | Some(op @ Operation::Equal { .. }) => { - if threshold_index + self.shift - op.len() as i64 > op.end_index() as i64 { + if threshold_index + self.shift - op.len() as i64 >= op.end_index() as i64 { self.last_operation = None; } } diff --git a/src/operation_transformation/operation.rs b/src/operation_transformation/operation.rs index 862a5b6..73722aa 100644 --- a/src/operation_transformation/operation.rs +++ b/src/operation_transformation/operation.rs @@ -6,11 +6,12 @@ use serde::{Deserialize, Serialize}; use super::merge_context::MergeContext; use crate::{ + Token, + operation_transformation::ordered_operation::OrderedOperation, utils::{ find_longest_prefix_contained_within::find_longest_prefix_contained_within, string_builder::StringBuilder, }, - Token, }; /// Represents a change that can be applied on a `StringBuilder`. @@ -84,10 +85,6 @@ where /// This operation is used to indicate that the text at the given index /// is unchanged. pub fn create_equal(index: usize, length: usize) -> Option { - if length == 0 { - return None; - } - Some(Operation::Equal { index, length, @@ -213,18 +210,12 @@ where } } - /// Returns the index of the last character that the operation affects. - pub fn end_index(&self) -> usize { - debug_assert!( - self.len() > 0, - " len() must be greater than 0 because operations must be non-empty" - ); - self.start_index() + self.len() - 1 - } + /// Returns the first index after the last character that the operation + /// affects. + pub fn end_index(&self) -> usize { self.start_index() + self.len() } /// Returns the range of indices of characters that the operation affects. - #[allow(clippy::range_plus_one)] - pub fn range(&self) -> Range { self.start_index()..self.end_index() + 1 } + pub fn range(&self) -> Range { self.start_index()..self.end_index() } /// Returns the number of affected characters. It is always greater than 0 /// because empty operations cannot be created. @@ -294,8 +285,10 @@ where #[allow(clippy::too_many_lines)] pub fn merge_operations_with_context( self, + order: usize, affecting_context: &mut MergeContext, produced_context: &mut MergeContext, + 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); @@ -362,7 +355,7 @@ where let moved_operation = operation.with_index(last_delete.start_index()); affecting_context.replace_last_operation(Operation::create_delete( - moved_operation.end_index() + 1, + moved_operation.end_index(), (last_delete.len() as i64 - difference) as usize, )); affecting_context.shift -= difference; @@ -415,19 +408,19 @@ where ); let overlap = (length as i64) - .min(last_delete.end_index() as i64 - operation.start_index() as i64 + 1); + .min(last_delete.end_index() as i64 - operation.start_index() as i64); #[cfg(debug_assertions)] let result = text.as_ref().map_or_else( || { Operation::create_equal( - operation.end_index().min(last_delete.end_index()), + operation.end_index().min(last_delete.end_index()) - 1, (length as i64 - overlap) as usize, ) }, |text| { Operation::create_equal_with_text( - operation.end_index().min(last_delete.end_index()), + operation.end_index().min(last_delete.end_index()) - 1, text.chars().skip(overlap as usize).collect::(), ) }, @@ -441,7 +434,26 @@ where result } - (operation @ Operation::Equal { .. }, _) => Some(operation), + + (operation @ Operation::Equal { .. }, _) => { + let result = 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) + } else { + Some(operation) + } + } else { + Some(operation) + }; + + produced_context.consume_and_replace_last_operation(result.clone()); + + result + } } } } @@ -531,9 +543,11 @@ mod tests { #[test] #[should_panic(expected = "Shifted index must be non-negative")] fn test_shifting_error() { - insta::assert_debug_snapshot!(Operation::create_insert(1, vec!["hi".into()]) - .unwrap() - .with_shifted_index(-2)); + insta::assert_debug_snapshot!( + Operation::create_insert(1, vec!["hi".into()]) + .unwrap() + .with_shifted_index(-2) + ); } #[test] diff --git a/src/operation_transformation/ordered_operation.rs b/src/operation_transformation/ordered_operation.rs index c9c0b67..8878c45 100644 --- a/src/operation_transformation/ordered_operation.rs +++ b/src/operation_transformation/ordered_operation.rs @@ -1,7 +1,7 @@ #[cfg(feature = "serde")] use serde::{Deserialize, Serialize}; -use crate::{operation_transformation::Operation, Token}; +use crate::{Token, operation_transformation::Operation}; #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] #[derive(Debug, Clone, PartialEq)] -- 2.47.2 From 46d2e0c485e7a51e32520923a9abd672856323e7 Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sun, 22 Jun 2025 09:48:26 +0100 Subject: [PATCH 07/15] Remove start_index from sorting --- src/operation_transformation/edited_text.rs | 6 +++++- src/operation_transformation/ordered_operation.rs | 13 ++----------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/operation_transformation/edited_text.rs b/src/operation_transformation/edited_text.rs index 56254a0..6aea48e 100644 --- a/src/operation_transformation/edited_text.rs +++ b/src/operation_transformation/edited_text.rs @@ -123,7 +123,11 @@ where 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 < 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()) diff --git a/src/operation_transformation/ordered_operation.rs b/src/operation_transformation/ordered_operation.rs index 8878c45..c404079 100644 --- a/src/operation_transformation/ordered_operation.rs +++ b/src/operation_transformation/ordered_operation.rs @@ -17,7 +17,7 @@ impl OrderedOperation where T: PartialEq + Clone + std::fmt::Debug, { - pub fn get_sort_key(&self) -> (usize, usize, usize, String) { + pub fn get_sort_key(&self, insertion_index: usize) -> (usize, usize, usize, String) { ( self.order, match &self.operation { @@ -25,7 +25,7 @@ where Operation::Insert { .. } => 2, Operation::Equal { .. } => 3, }, - self.operation.start_index(), + insertion_index, // Make sure that the ordering is deterministic regardless of which text // is left or right. match &self.operation { @@ -41,12 +41,3 @@ where ) } } - -impl PartialOrd for OrderedOperation -where - T: PartialEq + Clone + std::fmt::Debug, -{ - fn partial_cmp(&self, other: &Self) -> Option { - self.get_sort_key().partial_cmp(&other.get_sort_key()) - } -} -- 2.47.2 From a4f1a496bd8535919a02540196337d44d3276345 Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sun, 22 Jun 2025 09:48:56 +0100 Subject: [PATCH 08/15] Remove merge context --- src/operation_transformation.rs | 1 - src/operation_transformation/edited_text.rs | 24 ++---- src/operation_transformation/merge_context.rs | 75 ------------------- 3 files changed, 6 insertions(+), 94 deletions(-) delete mode 100644 src/operation_transformation/merge_context.rs diff --git a/src/operation_transformation.rs b/src/operation_transformation.rs index 9fd5a5d..c971229 100644 --- a/src/operation_transformation.rs +++ b/src/operation_transformation.rs @@ -1,6 +1,5 @@ mod cursor; mod edited_text; -mod merge_context; mod operation; mod ordered_operation; mod utils; diff --git a/src/operation_transformation/edited_text.rs b/src/operation_transformation/edited_text.rs index 6aea48e..4190157 100644 --- a/src/operation_transformation/edited_text.rs +++ b/src/operation_transformation/edited_text.rs @@ -4,9 +4,8 @@ use serde::{Deserialize, Serialize}; use super::{CursorPosition, Operation, TextWithCursors, ordered_operation::OrderedOperation}; use crate::{ diffs::{myers::diff, raw_operation::RawOperation}, - operation_transformation::{ - merge_context::MergeContext, - utils::{cook_operations::cook_operations, elongate_operations::elongate_operations}, + operation_transformation::utils::{ + cook_operations::cook_operations, elongate_operations::elongate_operations, }, tokenizer::{Tokenizer, word_tokenizer::word_tokenizer}, utils::{side::Side, string_builder::StringBuilder}, @@ -99,9 +98,6 @@ where "`EditedText`-s must be derived from the same text to be mergable" ); - let mut left_merge_context = MergeContext::default(); - let mut right_merge_context = MergeContext::default(); - let mut merged_cursors = Vec::with_capacity(self.cursors.len() + other.cursors.len()); let mut left_cursors = self.cursors.into_iter().peekable(); let mut right_cursors = other.cursors.into_iter().peekable(); @@ -148,12 +144,8 @@ where let original_length = operation.len() as i64; let result = match side { Side::Left => { - let result = operation.merge_operations_with_context( - order, - &mut right_merge_context, - &mut left_merge_context, - maybe_other_operation, - ); + let result = + operation.merge_operations_with_context(order, maybe_other_operation); if let Some(ref op @ (Operation::Insert { .. } | Operation::Equal { .. })) = result @@ -182,12 +174,8 @@ where result } Side::Right => { - let result = operation.merge_operations_with_context( - order, - &mut left_merge_context, - &mut right_merge_context, - maybe_other_operation, - ); + let result = + operation.merge_operations_with_context(order, maybe_other_operation); if let Some(ref op @ (Operation::Insert { .. } | Operation::Equal { .. })) = result diff --git a/src/operation_transformation/merge_context.rs b/src/operation_transformation/merge_context.rs deleted file mode 100644 index b40d2f3..0000000 --- a/src/operation_transformation/merge_context.rs +++ /dev/null @@ -1,75 +0,0 @@ -use core::fmt::Debug; - -use crate::operation_transformation::Operation; - -#[derive(Clone, Debug)] -pub struct MergeContext -where - T: PartialEq + Clone + std::fmt::Debug, -{ - last_operation: Option>, - pub shift: i64, -} - -impl Default for MergeContext -where - T: PartialEq + Clone + std::fmt::Debug, -{ - fn default() -> Self { - MergeContext { - last_operation: None, - shift: 0, - } - } -} - -impl MergeContext -where - T: PartialEq + Clone + std::fmt::Debug, -{ - pub fn last_operation(&self) -> Option<&Operation> { self.last_operation.as_ref() } - - pub fn replace_last_operation(&mut self, operation: Option>) { - self.last_operation = operation; - } - - /// Replace the last delete operation (if there was one) with a new one - /// while applying it to the `shift` in case the last operation - /// was a delete. - pub fn consume_and_replace_last_operation(&mut self, operation: Option>) { - if let Some(Operation::Delete { - deleted_character_count, - .. - }) = self.last_operation.take() - { - self.shift -= deleted_character_count as i64; - } - - self.last_operation = operation; - } - - /// Remove the last operation (if there was one) in case it is behind the - /// threshold operation. This updates the `shift` in case the last operation - /// was a delete. - pub fn consume_last_operation_if_it_is_too_behind(&mut self, threshold_index: i64) { - match self.last_operation.as_ref() { - Some( - op @ Operation::Delete { - deleted_character_count, - .. - }, - ) => { - if threshold_index + self.shift >= op.end_index() as i64 { - self.shift -= *deleted_character_count as i64; - self.last_operation = None; - } - } - Some(op @ Operation::Insert { .. }) | Some(op @ Operation::Equal { .. }) => { - if threshold_index + self.shift - op.len() as i64 >= op.end_index() as i64 { - self.last_operation = None; - } - } - _ => {} - } - } -} -- 2.47.2 From 917d47fbaa12d3b2c8a11497028dc3292fbc5bab Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sun, 22 Jun 2025 09:50:37 +0100 Subject: [PATCH 09/15] Remove indexes from Operation --- src/operation_transformation/operation.rs | 149 +++--------------- .../ordered_operation.rs | 4 +- .../utils/cook_operations.rs | 23 +-- 3 files changed, 27 insertions(+), 149 deletions(-) diff --git a/src/operation_transformation/operation.rs b/src/operation_transformation/operation.rs index 73722aa..653020f 100644 --- a/src/operation_transformation/operation.rs +++ b/src/operation_transformation/operation.rs @@ -22,7 +22,6 @@ where T: PartialEq + Clone + std::fmt::Debug, { Equal { - index: usize, length: usize, #[cfg(debug_assertions)] @@ -30,12 +29,10 @@ where }, Insert { - index: usize, text: Vec>, }, Delete { - index: usize, deleted_character_count: usize, #[cfg(debug_assertions)] @@ -43,39 +40,7 @@ where }, } -// impl PartialEq for Operation -// where -// T: PartialEq + Clone + std::fmt::Debug, -// { -// fn eq(&self, other: &Self) -> bool { -// match (self, other) { -// ( -// Operation::Equal { length, .. }, -// Operation::Equal { -// length: other_length, -// .. -// }, -// ) => length == other_length, -// ( -// Operation::Insert { text, .. }, -// Operation::Insert { -// text: other_text, .. -// }, -// ) => text == other_text, -// ( -// Operation::Delete { -// deleted_character_count, -// .. -// }, -// Operation::Delete { -// deleted_character_count: other_deleted_character_count, -// .. -// }, -// ) => deleted_character_count == other_deleted_character_count, -// _ => false, -// } -// } -// } + impl Operation where @@ -84,9 +49,8 @@ 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(index: usize, length: usize) -> Option { + pub fn create_equal( length: usize) -> Option { Some(Operation::Equal { - index, length, #[cfg(debug_assertions)] @@ -94,13 +58,12 @@ where }) } - pub fn create_equal_with_text(index: usize, text: String) -> Option { + pub fn create_equal_with_text( text: String) -> Option { if text.is_empty() { return None; } Some(Operation::Equal { - index, length: text.chars().count(), #[cfg(debug_assertions)] @@ -111,24 +74,23 @@ where /// Creates an insert operation with the given index and text. /// If the text is empty (meaning that the operation would be a no-op), /// returns None. - pub fn create_insert(index: usize, text: Vec>) -> Option { + pub fn create_insert(text: Vec>) -> Option { if text.is_empty() { return None; } - Some(Operation::Insert { index, text }) + Some(Operation::Insert { text }) } /// Creates a delete operation with the given index and number of /// to-be-deleted characters. If the operation would delete 0 (meaning /// that the operation would be a no-op), returns None. - pub fn create_delete(index: usize, deleted_character_count: usize) -> Option { + pub fn create_delete(deleted_character_count: usize) -> Option { if deleted_character_count == 0 { return None; } Some(Operation::Delete { - index, deleted_character_count, #[cfg(debug_assertions)] @@ -136,13 +98,12 @@ where }) } - pub fn create_delete_with_text(index: usize, text: String) -> Option { + pub fn create_delete_with_text(text: String) -> Option { if text.is_empty() { return None; } Some(Operation::Delete { - index, deleted_character_count: text.chars().count(), #[cfg(debug_assertions)] @@ -201,21 +162,6 @@ where builder } - /// Returns the index of the first character that the operation affects. - pub fn start_index(&self) -> usize { - match self { - Operation::Equal { index, .. } - | Operation::Insert { index, .. } - | Operation::Delete { index, .. } => *index, - } - } - - /// Returns the first index after the last character that the operation - /// affects. - pub fn end_index(&self) -> usize { self.start_index() + self.len() } - - /// Returns the range of indices of characters that the operation affects. - pub fn range(&self) -> Range { self.start_index()..self.end_index() } /// Returns the number of affected characters. It is always greater than 0 /// because empty operations cannot be created. @@ -230,53 +176,8 @@ where } } - /// Creates a new operation with the same type and text but with the given - /// index. - pub fn with_index(self, index: usize) -> Self { - match self { - Operation::Equal { - length, + - #[cfg(debug_assertions)] - text, - .. - } => Operation::Equal { - index, - length, - - #[cfg(debug_assertions)] - text, - }, - Operation::Insert { text, .. } => Operation::Insert { index, text }, - Operation::Delete { - deleted_character_count, - - #[cfg(debug_assertions)] - deleted_text, - .. - } => Operation::Delete { - index, - deleted_character_count, - - #[cfg(debug_assertions)] - deleted_text, - }, - } - } - - /// Creates a new operation with the same type and text but with the index - /// shifted by the given offset. The offset can be negative but the - /// resulting index must be non-negative. - /// - /// # Panics - /// - /// In debug mode, panics if the resulting index is negative. - pub fn with_shifted_index(self, offset: i64) -> Self { - let index = self.start_index() as i64 + offset; - debug_assert!(index >= 0, "Shifted index must be non-negative"); - - self.with_index(index as usize) - } /// Merges the operation with the given context, producing a new operation /// and updating the context. This implements a comples FSM that handles @@ -465,7 +366,6 @@ where fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { match self { Operation::Equal { - index, length, #[cfg(debug_assertions)] @@ -474,31 +374,28 @@ where #[cfg(debug_assertions)] write!( f, - "", + "", text.as_ref() .map(|text| format!("'{}'", text.replace('\n', "\\n"))) .unwrap_or(format!("{length} characters")), - index )?; #[cfg(not(debug_assertions))] - write!(f, "")?; + write!(f, "")?; Ok(()) } - Operation::Insert { index, text } => { + Operation::Insert { text } => { write!( f, - "", + "", text.iter() .map(Token::original) .collect::() .replace('\n', "\\n"), - index ) } Operation::Delete { - index, deleted_character_count, #[cfg(debug_assertions)] @@ -507,18 +404,17 @@ where #[cfg(debug_assertions)] write!( f, - "", + "", deleted_text .as_ref() .map(|text| format!("'{}'", text.replace('\n', "\\n"))) .unwrap_or(format!("{deleted_character_count} characters")), - index )?; #[cfg(not(debug_assertions))] write!( f, - "", + "", )?; Ok(()) @@ -540,22 +436,13 @@ mod tests { use super::*; - #[test] - #[should_panic(expected = "Shifted index must be non-negative")] - fn test_shifting_error() { - insta::assert_debug_snapshot!( - Operation::create_insert(1, vec!["hi".into()]) - .unwrap() - .with_shifted_index(-2) - ); - } #[test] fn test_apply_delete_with_create() { let builder = StringBuilder::new("hello world"); let delete_operation = - Operation::<()>::create_delete_with_text(0, "hello ".to_owned()).unwrap(); - let retain_operation = Operation::<()>::create_equal(5, 5).unwrap(); + Operation::<()>::create_delete_with_text("hello ".to_owned()).unwrap(); + let retain_operation = Operation::<()>::create_equal( 5).unwrap(); let mut builder = delete_operation.apply(builder); builder = retain_operation.apply(builder); @@ -567,8 +454,8 @@ mod tests { fn test_apply_insert() { let builder = StringBuilder::new("hello"); - let retain_operation = Operation::<()>::create_equal(5, 5).unwrap(); - let insert_operation = Operation::create_insert(5, vec![" my friend".into()]).unwrap(); + let retain_operation = Operation::<()>::create_equal(5).unwrap(); + let insert_operation = Operation::create_insert(vec![" my friend".into()]).unwrap(); let mut builder = retain_operation.apply(builder); builder = insert_operation.apply(builder); diff --git a/src/operation_transformation/ordered_operation.rs b/src/operation_transformation/ordered_operation.rs index c404079..7f8308a 100644 --- a/src/operation_transformation/ordered_operation.rs +++ b/src/operation_transformation/ordered_operation.rs @@ -1,7 +1,7 @@ #[cfg(feature = "serde")] use serde::{Deserialize, Serialize}; -use crate::{Token, operation_transformation::Operation}; +use crate::{operation_transformation::Operation, Token}; #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] #[derive(Debug, Clone, PartialEq)] @@ -29,7 +29,7 @@ where // Make sure that the ordering is deterministic regardless of which text // is left or right. match &self.operation { - Operation::Equal { index, .. } => index.to_string(), + Operation::Equal { length, .. } => length.to_string(), Operation::Insert { text, .. } => { text.iter().map(Token::original).collect::() } diff --git a/src/operation_transformation/utils/cook_operations.rs b/src/operation_transformation/utils/cook_operations.rs index 4405689..d03ea51 100644 --- a/src/operation_transformation/utils/cook_operations.rs +++ b/src/operation_transformation/utils/cook_operations.rs @@ -3,14 +3,12 @@ use crate::{ operation_transformation::{Operation, ordered_operation::OrderedOperation}, }; -/// Turn raw operations into ordered operations while keeping track of old & new -/// indexes. +/// Turn raw operations into ordered operations while keeping track of indexes. pub fn cook_operations(raw_operations: I) -> impl Iterator> where I: IntoIterator>, T: PartialEq + Clone + std::fmt::Debug, { - let mut new_index = 0; // this is the start index of the operation on the new text let mut order = 0; // this is the start index of the operation on the original text raw_operations.into_iter().filter_map(move |raw_operation| { @@ -19,30 +17,23 @@ where match raw_operation { RawOperation::Equal(..) => { let op = if cfg!(debug_assertions) { - Operation::create_equal_with_text(new_index, raw_operation.get_original_text()) + Operation::create_equal_with_text(raw_operation.get_original_text()) } else { - Operation::create_equal(new_index, length) + Operation::create_equal(length) } .map(|operation| OrderedOperation { order, operation }); - new_index += length; order += length; op } - RawOperation::Insert(tokens) => { - let op = Operation::create_insert(new_index, tokens) - .map(|operation| OrderedOperation { order, operation }); - - new_index += length; - - op - } + RawOperation::Insert(tokens) => Operation::create_insert(tokens) + .map(|operation| OrderedOperation { order, operation }), RawOperation::Delete(..) => { let op = if cfg!(debug_assertions) { - Operation::create_delete_with_text(new_index, raw_operation.get_original_text()) + Operation::create_delete_with_text(raw_operation.get_original_text()) } else { - Operation::create_delete(new_index, length) + Operation::create_delete(length) } .map(|operation| OrderedOperation { order, operation }); -- 2.47.2 From f95bd4324018930652996ef1ea321387fe0cf945 Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sun, 22 Jun 2025 12:02:18 +0100 Subject: [PATCH 10/15] 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] -- 2.47.2 From 5759b6bf66b09885c19ead41c69a2cbc174004b5 Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sun, 22 Jun 2025 13:06:36 +0100 Subject: [PATCH 11/15] Fix cursors --- src/operation_transformation.rs | 4 +- src/operation_transformation/edited_text.rs | 151 ++++++++++-------- src/operation_transformation/operation.rs | 132 ++++++--------- .../utils/cook_operations.rs | 36 +++-- src/utils/string_builder.rs | 1 - tests/examples/deletes.yml | 2 +- tests/examples/replacing.yml | 2 +- tests/examples/various.yml | 4 +- 8 files changed, 157 insertions(+), 175 deletions(-) diff --git a/src/operation_transformation.rs b/src/operation_transformation.rs index c971229..5e00ae6 100644 --- a/src/operation_transformation.rs +++ b/src/operation_transformation.rs @@ -110,8 +110,8 @@ mod test { }, // inside of "s|ample" because "text" got replaced by "sample" CursorPosition { id: 3, - char_index: 42 - }, // before "cursor movements" + char_index: 30 + }, // after "complex sample" ] ) ); diff --git a/src/operation_transformation/edited_text.rs b/src/operation_transformation/edited_text.rs index 10a0133..d081c27 100644 --- a/src/operation_transformation/edited_text.rs +++ b/src/operation_transformation/edited_text.rs @@ -161,41 +161,47 @@ where let original_length = operation.len() as i64; let result = match side { Side::Left => { - let result = operation.merge_operations_with_context( - order, - &mut last_other_op, - maybe_other_operation, - ); + let result = operation.merge_operations_with_context(order, &mut last_other_op); - if let Some( - ref op @ (OrderedOperation { - operation: Operation::Insert { .. }, - .. - } - | OrderedOperation { - operation: Operation::Equal { .. }, - .. - }), - ) = result + if let 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, - OrderedOperation { - operation: Operation::Equal { .. }, - .. - } - ) { - shift += op.operation.len() as i64 - original_length; - } + println!( + "merrged_length: {}, seen_left_length: {}, op len {}, original_length \ + {}", + merged_length, + seen_left_length, + op.operation.len(), + original_length + ); + let mut shift = merged_length as i64 + // - last_other_op + // .map(|op| op.operation.len() as i64) + // .unwrap_or(0) as i64 + - seen_left_length as i64; + // if !matches!( + // op, + // OrderedOperation { + // operation: Operation::Equal { .. }, + // .. + // } + // ) { + shift += op.operation.len() as i64 - original_length; + // } while let Some(cursor) = left_cursors.next_if(|cursor| { cursor.char_index <= seen_left_length + original_length as usize }) { - merged_cursors.push(cursor.with_index( - (merged_length as i64).max(cursor.char_index as i64 + shift) - as usize, - )); + println!("cursor {}", cursor.char_index); + merged_cursors.push( + cursor.with_index((cursor.char_index as i64 + shift) as usize), + ); } } @@ -204,46 +210,53 @@ where } maybe_left_op = left_iter.next(); - last_left_op = result.clone(); + last_left_op = Some(result.clone()); result } Side::Right => { - let result = operation.merge_operations_with_context( - order, - &mut last_other_op, - maybe_other_operation, - ); + let result = operation.merge_operations_with_context(order, &mut last_other_op); - if let Some( - ref op @ (OrderedOperation { - operation: Operation::Insert { .. }, - .. - } - | OrderedOperation { - operation: Operation::Equal { .. }, - .. - }), - ) = result + if let 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, - OrderedOperation { - operation: Operation::Equal { .. }, - .. - } - ) { - shift += op.operation.len() as i64 - original_length; - } + println!( + "merrged_length: {}, seen_left_length: {}, op len {}, original_length \ + {}", + merged_length, + seen_left_length, + op.operation.len(), + original_length + ); + let mut shift = merged_length as i64 + // - last_other_op + // .map(|op| op.operation.len() as i64) + // .unwrap_or(0) as i64 + - seen_right_length as i64; + // if !matches!( + // op, + // OrderedOperation { + // operation: Operation::Equal { .. }, + // .. + // } + // ) { + shift += op.operation.len() as i64 - original_length; + // } while let Some(cursor) = right_cursors.next_if(|cursor| { cursor.char_index <= seen_right_length + original_length as usize }) { - merged_cursors.push(cursor.with_index( - (merged_length as i64).max(cursor.char_index as i64 + shift) - as usize, - )); + println!("cursor {}", cursor.char_index); + + merged_cursors.push( + cursor.with_index((cursor.char_index as i64 + shift) as usize), + ); } } @@ -252,7 +265,7 @@ where } maybe_right_op = right_iter.next(); - last_right_op = result.clone(); + last_right_op = Some(result.clone()); result } @@ -260,17 +273,15 @@ where println!(" = {result:?}"); - if let Some(operation) = result { - if operation.operation.len() == 0 { - continue; - } - - if is_advancing_operation { - merged_length += operation.operation.len(); - } - - merged_operations.push(operation); + if result.operation.len() == 0 { + continue; } + + if is_advancing_operation { + merged_length += result.operation.len(); + } + + merged_operations.push(result); } for cursor in left_cursors.chain(right_cursors) { diff --git a/src/operation_transformation/operation.rs b/src/operation_transformation/operation.rs index fbaf786..c0b2f5c 100644 --- a/src/operation_transformation/operation.rs +++ b/src/operation_transformation/operation.rs @@ -46,66 +46,45 @@ 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 { - Some(Operation::Equal { + pub fn create_equal(length: usize) -> Self { + Operation::Equal { length, #[cfg(debug_assertions)] text: None, - }) + } } - pub fn create_equal_with_text(text: String) -> Option { - if text.is_empty() { - return None; - } - - Some(Operation::Equal { + pub fn create_equal_with_text(text: String) -> Self { + Operation::Equal { length: text.chars().count(), #[cfg(debug_assertions)] text: Some(text), - }) + } } /// Creates an insert operation with the given index and text. - /// If the text is empty (meaning that the operation would be a no-op), - /// returns None. - pub fn create_insert(text: Vec>) -> Option { - if text.is_empty() { - return None; - } - - Some(Operation::Insert { text }) - } + pub fn create_insert(text: Vec>) -> Self { Operation::Insert { text } } /// Creates a delete operation with the given index and number of - /// to-be-deleted characters. If the operation would delete 0 (meaning - /// that the operation would be a no-op), returns None. - pub fn create_delete(deleted_character_count: usize) -> Option { - if deleted_character_count == 0 { - return None; - } - - Some(Operation::Delete { + /// to-be-deleted characters. + pub fn create_delete(deleted_character_count: usize) -> Self { + Operation::Delete { deleted_character_count, #[cfg(debug_assertions)] deleted_text: None, - }) + } } - pub fn create_delete_with_text(text: String) -> Option { - if text.is_empty() { - return None; - } - - Some(Operation::Delete { + pub fn create_delete_with_text(text: String) -> Self { + Operation::Delete { deleted_character_count: text.chars().count(), #[cfg(debug_assertions)] deleted_text: Some(text), - }) + } } /// Applies the operation to the given `StringBuilder`, returning the @@ -181,12 +160,8 @@ where self, order: usize, previous_operation: &mut Option>, - other_operation: Option>, - ) -> Option> { - println!( - "mergin: {self} (order {order}) - previous: {previous_operation:?} - other: \ - {other_operation:?}" - ); + ) -> OrderedOperation { + println!("mergin: {self} (order {order}) - previous: {previous_operation:?}"); let operation = self; match (operation, previous_operation) { @@ -209,7 +184,10 @@ where let trimmed_operation = Operation::create_insert(text[offset_in_tokens..].to_vec()); - trimmed_operation.map(|operation| OrderedOperation { order, operation }) + OrderedOperation { + order, + operation: trimmed_operation, + } } ( @@ -248,10 +226,10 @@ where #[cfg(not(debug_assertions))] let updated_delete = Operation::create_delete(new_length); - updated_delete.map(|operation| OrderedOperation { + OrderedOperation { order: order + overlap, - operation, - }) + operation: updated_delete, + } } ( @@ -268,19 +246,6 @@ where }, ), ) => { - 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 last_delete_end_index = last_delete.order + last_delete.operation.len(); let overlap = @@ -299,30 +264,32 @@ where #[cfg(not(debug_assertions))] let updated_equal = Operation::create_equal((length as i64 - overlap) as usize); - updated_equal.map(|operation| OrderedOperation { + OrderedOperation { order: order + overlap as usize, - operation, - }) - } - - (operation @ Operation::Equal { .. }, _) => { - 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(0) - } else { - Some(operation) - } - } else { - Some(operation) + operation: updated_equal, } - .map(|operation| OrderedOperation { order, operation }) } - (operation, _) => Some(OrderedOperation { order, operation }), + ( + operation @ Operation::Equal { .. }, + Some( + last_equal @ OrderedOperation { + operation: Operation::Equal { .. }, + .. + }, + ), + ) => OrderedOperation { + order, + operation: if operation.len() == last_equal.operation.len() + && order == last_equal.order + { + Operation::create_equal(0) + } else { + operation + }, + }, + + (operation, _) => OrderedOperation { order, operation }, } } } @@ -404,9 +371,8 @@ mod tests { #[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 delete_operation = Operation::<()>::create_delete_with_text("hello ".to_owned()); + let retain_operation = Operation::<()>::create_equal(5); let mut builder = delete_operation.apply(builder); builder = retain_operation.apply(builder); @@ -418,8 +384,8 @@ mod tests { fn test_apply_insert() { let builder = StringBuilder::new("hello"); - let retain_operation = Operation::<()>::create_equal(5).unwrap(); - let insert_operation = Operation::create_insert(vec![" my friend".into()]).unwrap(); + let retain_operation = Operation::<()>::create_equal(5); + let insert_operation = Operation::create_insert(vec![" my friend".into()]); let mut builder = retain_operation.apply(builder); builder = insert_operation.apply(builder); diff --git a/src/operation_transformation/utils/cook_operations.rs b/src/operation_transformation/utils/cook_operations.rs index d03ea51..0e21de7 100644 --- a/src/operation_transformation/utils/cook_operations.rs +++ b/src/operation_transformation/utils/cook_operations.rs @@ -11,31 +11,37 @@ where { let mut order = 0; // this is the start index of the operation on the original text - raw_operations.into_iter().filter_map(move |raw_operation| { + raw_operations.into_iter().map(move |raw_operation| { let length = raw_operation.original_text_length(); match raw_operation { RawOperation::Equal(..) => { - let op = if cfg!(debug_assertions) { - Operation::create_equal_with_text(raw_operation.get_original_text()) - } else { - Operation::create_equal(length) - } - .map(|operation| OrderedOperation { order, operation }); + let op = OrderedOperation { + order, + operation: if cfg!(debug_assertions) { + Operation::create_equal_with_text(raw_operation.get_original_text()) + } else { + Operation::create_equal(length) + }, + }; order += length; op } - RawOperation::Insert(tokens) => Operation::create_insert(tokens) - .map(|operation| OrderedOperation { order, operation }), + RawOperation::Insert(tokens) => OrderedOperation { + order, + operation: Operation::create_insert(tokens), + }, RawOperation::Delete(..) => { - let op = if cfg!(debug_assertions) { - Operation::create_delete_with_text(raw_operation.get_original_text()) - } else { - Operation::create_delete(length) - } - .map(|operation| OrderedOperation { order, operation }); + let op = OrderedOperation { + order, + operation: if cfg!(debug_assertions) { + Operation::create_delete_with_text(raw_operation.get_original_text()) + } else { + Operation::create_delete(length) + }, + }; order += length; diff --git a/src/utils/string_builder.rs b/src/utils/string_builder.rs index 63aebdc..5659d3e 100644 --- a/src/utils/string_builder.rs +++ b/src/utils/string_builder.rs @@ -1,4 +1,3 @@ -use core::ops::Range; use std::iter::Iterator; /// A helper for building a string in-order based on an original string and a diff --git a/tests/examples/deletes.yml b/tests/examples/deletes.yml index d8c2819..3476e95 100644 --- a/tests/examples/deletes.yml +++ b/tests/examples/deletes.yml @@ -28,7 +28,7 @@ expected: long small parent: long run of text where one barely has no changes but has cursors left: long| run of tex|t where one barely has no |changes but has |cursors right: long run one barely has no changes cursors -expected: long| run| one barely has no |changes |cursors +expected: long| ru|n one barely has no |changes |cursors --- parent: long text where the cursor has to be clamped after delete diff --git a/tests/examples/replacing.yml b/tests/examples/replacing.yml index cea57b8..15126d8 100644 --- a/tests/examples/replacing.yml +++ b/tests/examples/replacing.yml @@ -2,7 +2,7 @@ parent: original_1 original_2 original_3 left: original_1 edit_1| original_3 right: original_1 original_2| edit_2 -expected: original_1 edit_1|| edit_2 +expected: original_1| edit_1| edit_2 --- # Both replace the same token with the same value diff --git a/tests/examples/various.yml b/tests/examples/various.yml index 7a87e4e..4576edc 100644 --- a/tests/examples/various.yml +++ b/tests/examples/various.yml @@ -43,7 +43,7 @@ expected: Send the |detailed |quarterly |detailed report to the |entire |team parent: Ready, Set go left: Ready! Set go| right: Ready, Set, go!| -expected: Ready! Set, go!|| +expected: Ready!| Set, go!| --- parent: "Total: $100" @@ -67,7 +67,7 @@ expected: market| placemarket|space parent: A B C D left: A X B D| right: A B Y| -expected: A X B Y|| +expected: A X B| Y| --- parent: Please submit your assignment by Friday -- 2.47.2 From a070872bd3a077943205f55e909814301d44bedb Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sun, 22 Jun 2025 14:53:45 +0100 Subject: [PATCH 12/15] Remove OrderedOperation --- src/operation_transformation.rs | 1 - src/operation_transformation/edited_text.rs | 143 +++---------- src/operation_transformation/operation.rs | 197 ++++++++++-------- .../ordered_operation.rs | 43 ---- ...ted_text__tests__calculate_operations.snap | 40 +--- ...ts__calculate_operations_with_no_diff.snap | 15 +- .../utils/cook_operations.rs | 34 +-- 7 files changed, 171 insertions(+), 302 deletions(-) delete mode 100644 src/operation_transformation/ordered_operation.rs diff --git a/src/operation_transformation.rs b/src/operation_transformation.rs index 5e00ae6..777c2f0 100644 --- a/src/operation_transformation.rs +++ b/src/operation_transformation.rs @@ -1,7 +1,6 @@ mod cursor; mod edited_text; mod operation; -mod ordered_operation; mod utils; pub use cursor::{CursorPosition, TextWithCursors}; diff --git a/src/operation_transformation/edited_text.rs b/src/operation_transformation/edited_text.rs index d081c27..090d965 100644 --- a/src/operation_transformation/edited_text.rs +++ b/src/operation_transformation/edited_text.rs @@ -1,13 +1,13 @@ #[cfg(feature = "serde")] use serde::{Deserialize, Serialize}; -use super::{CursorPosition, Operation, TextWithCursors, ordered_operation::OrderedOperation}; +use super::{CursorPosition, Operation, TextWithCursors}; use crate::{ diffs::{myers::diff, raw_operation::RawOperation}, operation_transformation::utils::{ cook_operations::cook_operations, elongate_operations::elongate_operations, }, - tokenizer::{Tokenizer, word_tokenizer::word_tokenizer}, + tokenizer::{word_tokenizer::word_tokenizer, Tokenizer}, utils::{side::Side, string_builder::StringBuilder}, }; @@ -30,7 +30,7 @@ where T: PartialEq + Clone + std::fmt::Debug, { text: &'a str, - operations: Vec>, + operations: Vec>, pub(crate) cursors: Vec, } @@ -76,11 +76,7 @@ where /// Create a new `EditedText` with the given operations. /// The operations must be in the order in which they are meant to be /// applied. The operations must not overlap. - fn new( - text: &'a str, - operations: Vec>, - mut cursors: Vec, - ) -> Self { + fn new(text: &'a str, operations: Vec>, mut cursors: Vec) -> Self { cursors.sort_by_key(|cursor| cursor.char_index); Self { @@ -92,7 +88,6 @@ where #[must_use] pub fn merge(self, other: Self) -> Self { - println!("\n\n\n\n\n\n----\n"); debug_assert_eq!( self.text, other.text, "`EditedText`-s must be derived from the same text to be mergable" @@ -102,7 +97,7 @@ where let mut left_cursors = self.cursors.into_iter().peekable(); let mut right_cursors = other.cursors.into_iter().peekable(); - let mut merged_operations: Vec> = + let mut merged_operations: Vec> = Vec::with_capacity(self.operations.len() + other.operations.len()); let mut left_iter = self.operations.into_iter(); @@ -119,38 +114,24 @@ where let mut last_right_op = None; 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(), - ) + let (side, 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, last_right_op.clone()) + } else { + (Side::Right, right_op, 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, - }; + (Some(left_op), None) => (Side::Left, left_op, last_right_op.clone()), + (None, Some(right_op)) => (Side::Right, right_op, last_left_op.clone()), + (None, None) => break, + }; let is_advancing_operation = matches!( operation, @@ -161,39 +142,12 @@ where let original_length = operation.len() as i64; let result = match side { Side::Left => { - let result = operation.merge_operations_with_context(order, &mut last_other_op); + let result = operation.merge_operations(&mut last_other_op); - if let ref op @ (OrderedOperation { - operation: Operation::Insert { .. }, - .. - } - | OrderedOperation { - operation: Operation::Equal { .. }, - .. - }) = result - { - println!( - "merrged_length: {}, seen_left_length: {}, op len {}, original_length \ - {}", - merged_length, - seen_left_length, - op.operation.len(), - original_length - ); - let mut shift = merged_length as i64 - // - last_other_op - // .map(|op| op.operation.len() as i64) - // .unwrap_or(0) as i64 - - seen_left_length as i64; - // if !matches!( - // op, - // OrderedOperation { - // operation: Operation::Equal { .. }, - // .. - // } - // ) { - shift += op.operation.len() as i64 - original_length; - // } + if let ref op @ (Operation::Insert { .. } | Operation::Equal { .. }) = result { + let shift = merged_length as i64 - seen_left_length as i64 + + op.len() as i64 + - original_length; while let Some(cursor) = left_cursors.next_if(|cursor| { cursor.char_index <= seen_left_length + original_length as usize @@ -215,39 +169,12 @@ where result } Side::Right => { - let result = operation.merge_operations_with_context(order, &mut last_other_op); + let result = operation.merge_operations(&mut last_other_op); - if let ref op @ (OrderedOperation { - operation: Operation::Insert { .. }, - .. - } - | OrderedOperation { - operation: Operation::Equal { .. }, - .. - }) = result - { - println!( - "merrged_length: {}, seen_left_length: {}, op len {}, original_length \ - {}", - merged_length, - seen_left_length, - op.operation.len(), - original_length - ); - let mut shift = merged_length as i64 - // - last_other_op - // .map(|op| op.operation.len() as i64) - // .unwrap_or(0) as i64 - - seen_right_length as i64; - // if !matches!( - // op, - // OrderedOperation { - // operation: Operation::Equal { .. }, - // .. - // } - // ) { - shift += op.operation.len() as i64 - original_length; - // } + if let ref op @ (Operation::Insert { .. } | Operation::Equal { .. }) = result { + let shift = merged_length as i64 - seen_right_length as i64 + + op.len() as i64 + - original_length; while let Some(cursor) = right_cursors.next_if(|cursor| { cursor.char_index <= seen_right_length + original_length as usize @@ -273,12 +200,12 @@ where println!(" = {result:?}"); - if result.operation.len() == 0 { + if result.len() == 0 { continue; } if is_advancing_operation { - merged_length += result.operation.len(); + merged_length += result.len(); } merged_operations.push(result); @@ -296,9 +223,7 @@ where pub fn apply(&self) -> String { let mut builder: StringBuilder<'_> = StringBuilder::new(self.text); - for OrderedOperation { operation, .. } in &self.operations { - println!("applying operation {operation:?}"); - + for operation in &self.operations { builder = operation.apply(builder); } diff --git a/src/operation_transformation/operation.rs b/src/operation_transformation/operation.rs index c0b2f5c..63702cd 100644 --- a/src/operation_transformation/operation.rs +++ b/src/operation_transformation/operation.rs @@ -1,12 +1,10 @@ use core::fmt::{Debug, Display}; -use std::ops::Range; #[cfg(feature = "serde")] use serde::{Deserialize, Serialize}; use crate::{ Token, - operation_transformation::ordered_operation::OrderedOperation, utils::{ find_longest_prefix_contained_within::find_longest_prefix_contained_within, string_builder::StringBuilder, @@ -21,6 +19,7 @@ where T: PartialEq + Clone + std::fmt::Debug, { Equal { + order: usize, length: usize, #[cfg(debug_assertions)] @@ -28,10 +27,12 @@ where }, Insert { + order: usize, text: Vec>, }, Delete { + order: usize, deleted_character_count: usize, #[cfg(debug_assertions)] @@ -46,8 +47,9 @@ 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) -> Self { + pub fn create_equal(order: usize, length: usize) -> Self { Operation::Equal { + order, length, #[cfg(debug_assertions)] @@ -55,8 +57,9 @@ where } } - pub fn create_equal_with_text(text: String) -> Self { + pub fn create_equal_with_text(order: usize, text: String) -> Self { Operation::Equal { + order, length: text.chars().count(), #[cfg(debug_assertions)] @@ -65,12 +68,15 @@ where } /// Creates an insert operation with the given index and text. - pub fn create_insert(text: Vec>) -> Self { Operation::Insert { text } } + pub fn create_insert(order: usize, text: Vec>) -> Self { + Operation::Insert { order, text } + } /// Creates a delete operation with the given index and number of /// to-be-deleted characters. - pub fn create_delete(deleted_character_count: usize) -> Self { + pub fn create_delete(order: usize, deleted_character_count: usize) -> Self { Operation::Delete { + order, deleted_character_count, #[cfg(debug_assertions)] @@ -78,8 +84,9 @@ where } } - pub fn create_delete_with_text(text: String) -> Self { + pub fn create_delete_with_text(order: usize, text: String) -> Self { Operation::Delete { + order, deleted_character_count: text.chars().count(), #[cfg(debug_assertions)] @@ -87,6 +94,38 @@ where } } + fn order(&self) -> usize { + match self { + Operation::Equal { order, .. } => *order, + Operation::Insert { order, .. } => *order, + Operation::Delete { order, .. } => *order, + } + } + + pub fn get_sort_key(&self, insertion_index: usize) -> (usize, usize, usize, String) { + ( + self.order(), + match self { + Operation::Delete { .. } => 1, + Operation::Insert { .. } => 2, + Operation::Equal { .. } => 3, + }, + insertion_index, + // Make sure that the ordering is deterministic regardless of which text + // is left or right. + match self { + Operation::Equal { length, .. } => length.to_string(), + Operation::Insert { text, .. } => { + text.iter().map(Token::original).collect::() + } + Operation::Delete { + deleted_character_count, + .. + } => deleted_character_count.to_string(), + }, + ) + } + /// Applies the operation to the given `StringBuilder`, returning the /// modified `StringBuilder`. /// @@ -156,23 +195,14 @@ where /// the merging of operations in a way that is consistent with the text. /// The contexts are updated in-place. #[allow(clippy::too_many_lines)] - pub fn merge_operations_with_context( - self, - order: usize, - previous_operation: &mut Option>, - ) -> OrderedOperation { - println!("mergin: {self} (order {order}) - previous: {previous_operation:?}"); + pub fn merge_operations(self, previous_operation: &mut Option) -> Operation { let operation = self; match (operation, previous_operation) { ( - Operation::Insert { text }, - Some(OrderedOperation { - operation: - Operation::Insert { - text: previous_inserted_text, - .. - }, + Operation::Insert { order, text }, + Some(Operation::Insert { + text: previous_inserted_text, .. }), ) => { @@ -182,29 +212,26 @@ where let offset_in_tokens = find_longest_prefix_contained_within(previous_inserted_text, &text); - let trimmed_operation = Operation::create_insert(text[offset_in_tokens..].to_vec()); - - OrderedOperation { - order, - operation: trimmed_operation, - } + Operation::create_insert(order, text[offset_in_tokens..].to_vec()) } ( Operation::Delete { + order, + deleted_character_count, + #[cfg(debug_assertions)] deleted_text, - deleted_character_count, }, - Some( - last_delete @ OrderedOperation { - operation: Operation::Delete { .. }, - .. - }, - ), + Some(Operation::Delete { + order: last_delete_order, + deleted_character_count: last_delete_deleted_character_count, + .. + }), ) => { let operation_end_index = order + deleted_character_count; - let last_delete_end_index = last_delete.order + last_delete.operation.len(); + let last_delete_end_index = + *last_delete_order + *last_delete_deleted_character_count; let new_length = deleted_character_count .min(0.max(operation_end_index as i64 - last_delete_end_index as i64) as usize); @@ -213,9 +240,10 @@ where #[cfg(debug_assertions)] let updated_delete = deleted_text.as_ref().map_or_else( - || Operation::create_delete(new_length), + || Operation::create_delete(order + overlap, new_length), |text| { Operation::create_delete_with_text( + order + overlap, text.chars() .skip((deleted_character_count - new_length) as usize) .collect::(), @@ -224,72 +252,72 @@ where ); #[cfg(not(debug_assertions))] - let updated_delete = Operation::create_delete(new_length); + let updated_delete = Operation::create_delete(order + overlap, new_length); - OrderedOperation { - order: order + overlap, - operation: updated_delete, - } + updated_delete } ( - ref operation @ Operation::Equal { + Operation::Equal { + order, length, + #[cfg(debug_assertions)] ref text, - .. }, - Some( - last_delete @ OrderedOperation { - operation: Operation::Delete { .. }, - .. - }, - ), + Some(Operation::Delete { + order: last_delete_order, + deleted_character_count: last_delete_deleted_character_count, + .. + }), ) => { - let last_delete_end_index = last_delete.order + last_delete.operation.len(); + let last_delete_end_index = + *last_delete_order + *last_delete_deleted_character_count; let overlap = 0.max((length as i64).min(last_delete_end_index as i64 - order as i64)); #[cfg(debug_assertions)] let updated_equal = text.as_ref().map_or_else( - || Operation::create_equal((length as i64 - overlap) as usize), + || { + Operation::create_equal( + order + overlap as usize, + (length as i64 - overlap) as usize, + ) + }, |text| { Operation::create_equal_with_text( + order + overlap as usize, text.chars().skip(overlap as usize).collect::(), ) }, ); #[cfg(not(debug_assertions))] - let updated_equal = Operation::create_equal((length as i64 - overlap) as usize); + let updated_equal = Operation::create_equal( + order + overlap as usize, + (length as i64 - overlap) as usize, + ); - OrderedOperation { - order: order + overlap as usize, - operation: updated_equal, - } + updated_equal } ( - operation @ Operation::Equal { .. }, - Some( - last_equal @ OrderedOperation { - operation: Operation::Equal { .. }, - .. - }, - ), - ) => OrderedOperation { - order, - operation: if operation.len() == last_equal.operation.len() - && order == last_equal.order - { - Operation::create_equal(0) + ref operation @ Operation::Equal { ref order, .. }, + Some(Operation::Equal { + order: last_equal_order, + length: last_equal_length, + .. + }), + ) => { + if operation.len() == *last_equal_length && *order == *last_equal_order { + Operation::create_equal(*order, 0) } else { - operation - }, - }, + operation.clone() + } + } - (operation, _) => OrderedOperation { order, operation }, + (operation, _) => operation, } } } @@ -301,6 +329,7 @@ where fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { match self { Operation::Equal { + order, length, #[cfg(debug_assertions)] @@ -309,21 +338,21 @@ where #[cfg(debug_assertions)] write!( f, - "", + "", text.as_ref() .map(|text| format!("'{}'", text.replace('\n', "\\n"))) .unwrap_or(format!("{length} characters")), )?; #[cfg(not(debug_assertions))] - write!(f, "")?; + write!(f, "")?; Ok(()) } - Operation::Insert { text } => { + Operation::Insert { order, text } => { write!( f, - "", + "", text.iter() .map(Token::original) .collect::() @@ -331,6 +360,7 @@ where ) } Operation::Delete { + order, deleted_character_count, #[cfg(debug_assertions)] @@ -339,7 +369,7 @@ where #[cfg(debug_assertions)] write!( f, - "", + "", deleted_text .as_ref() .map(|text| format!("'{}'", text.replace('\n', "\\n"))) @@ -347,7 +377,10 @@ where )?; #[cfg(not(debug_assertions))] - write!(f, "",)?; + write!( + f, + "", + )?; Ok(()) } @@ -371,8 +404,8 @@ mod tests { #[test] fn test_apply_delete_with_create() { let builder = StringBuilder::new("hello world"); - let delete_operation = Operation::<()>::create_delete_with_text("hello ".to_owned()); - let retain_operation = Operation::<()>::create_equal(5); + let delete_operation = Operation::<()>::create_delete_with_text(0, "hello ".to_owned()); + let retain_operation = Operation::<()>::create_equal(6, 5); let mut builder = delete_operation.apply(builder); builder = retain_operation.apply(builder); @@ -384,8 +417,8 @@ mod tests { fn test_apply_insert() { let builder = StringBuilder::new("hello"); - let retain_operation = Operation::<()>::create_equal(5); - let insert_operation = Operation::create_insert(vec![" my friend".into()]); + let retain_operation = Operation::<()>::create_equal(0, 5); + let insert_operation = Operation::create_insert(5, vec![" my friend".into()]); let mut builder = retain_operation.apply(builder); builder = insert_operation.apply(builder); diff --git a/src/operation_transformation/ordered_operation.rs b/src/operation_transformation/ordered_operation.rs deleted file mode 100644 index 7f8308a..0000000 --- a/src/operation_transformation/ordered_operation.rs +++ /dev/null @@ -1,43 +0,0 @@ -#[cfg(feature = "serde")] -use serde::{Deserialize, Serialize}; - -use crate::{operation_transformation::Operation, Token}; - -#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] -#[derive(Debug, Clone, PartialEq)] -pub struct OrderedOperation -where - T: PartialEq + Clone + std::fmt::Debug, -{ - pub order: usize, - pub operation: Operation, -} - -impl OrderedOperation -where - T: PartialEq + Clone + std::fmt::Debug, -{ - pub fn get_sort_key(&self, insertion_index: usize) -> (usize, usize, usize, String) { - ( - self.order, - match &self.operation { - Operation::Delete { .. } => 1, - Operation::Insert { .. } => 2, - Operation::Equal { .. } => 3, - }, - insertion_index, - // Make sure that the ordering is deterministic regardless of which text - // is left or right. - match &self.operation { - Operation::Equal { length, .. } => length.to_string(), - Operation::Insert { text, .. } => { - text.iter().map(Token::original).collect::() - } - Operation::Delete { - deleted_character_count, - .. - } => deleted_character_count.to_string(), - }, - ) - } -} 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 dc186e0..abbabbd 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 @@ -6,38 +6,14 @@ snapshot_kind: text EditedText { text: "hello world! How are you? Adam", operations: [ - OrderedOperation { - order: 0, - operation: , - }, - OrderedOperation { - order: 12, - operation: , - }, - OrderedOperation { - order: 12, - operation: , - }, - OrderedOperation { - order: 13, - operation: , - }, - OrderedOperation { - order: 16, - operation: , - }, - OrderedOperation { - order: 17, - operation: , - }, - OrderedOperation { - order: 20, - operation: , - }, - OrderedOperation { - order: 31, - 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 4dad79d..275a552 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 @@ -6,18 +6,9 @@ snapshot_kind: text EditedText { text: "hello world!", operations: [ - OrderedOperation { - order: 0, - operation: , - }, - OrderedOperation { - order: 5, - operation: , - }, - OrderedOperation { - order: 6, - operation: , - }, + , + , + , ], cursors: [], } diff --git a/src/operation_transformation/utils/cook_operations.rs b/src/operation_transformation/utils/cook_operations.rs index 0e21de7..c642d3c 100644 --- a/src/operation_transformation/utils/cook_operations.rs +++ b/src/operation_transformation/utils/cook_operations.rs @@ -1,10 +1,7 @@ -use crate::{ - diffs::raw_operation::RawOperation, - operation_transformation::{Operation, ordered_operation::OrderedOperation}, -}; +use crate::{diffs::raw_operation::RawOperation, operation_transformation::Operation}; /// Turn raw operations into ordered operations while keeping track of indexes. -pub fn cook_operations(raw_operations: I) -> impl Iterator> +pub fn cook_operations(raw_operations: I) -> impl Iterator> where I: IntoIterator>, T: PartialEq + Clone + std::fmt::Debug, @@ -16,31 +13,22 @@ where match raw_operation { RawOperation::Equal(..) => { - let op = OrderedOperation { - order, - operation: if cfg!(debug_assertions) { - Operation::create_equal_with_text(raw_operation.get_original_text()) - } else { - Operation::create_equal(length) - }, + let op = if cfg!(debug_assertions) { + Operation::create_equal_with_text(order, raw_operation.get_original_text()) + } else { + Operation::create_equal(order, length) }; order += length; op } - RawOperation::Insert(tokens) => OrderedOperation { - order, - operation: Operation::create_insert(tokens), - }, + RawOperation::Insert(tokens) => Operation::create_insert(order, tokens), RawOperation::Delete(..) => { - let op = OrderedOperation { - order, - operation: if cfg!(debug_assertions) { - Operation::create_delete_with_text(raw_operation.get_original_text()) - } else { - Operation::create_delete(length) - }, + let op = if cfg!(debug_assertions) { + Operation::create_delete_with_text(order, raw_operation.get_original_text()) + } else { + Operation::create_delete(order, length) }; order += length; -- 2.47.2 From df6ec254cad210d8b0ba69f6647fae35e0e757f3 Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sun, 22 Jun 2025 14:58:38 +0100 Subject: [PATCH 13/15] Lint --- src/operation_transformation/edited_text.rs | 2 +- src/operation_transformation/operation.rs | 2 +- .../utils/cook_operations.rs | 25 ++++--- .../utils/elongate_operations.rs | 68 +------------------ 4 files changed, 21 insertions(+), 76 deletions(-) diff --git a/src/operation_transformation/edited_text.rs b/src/operation_transformation/edited_text.rs index 090d965..3e37641 100644 --- a/src/operation_transformation/edited_text.rs +++ b/src/operation_transformation/edited_text.rs @@ -7,7 +7,7 @@ use crate::{ operation_transformation::utils::{ cook_operations::cook_operations, elongate_operations::elongate_operations, }, - tokenizer::{word_tokenizer::word_tokenizer, Tokenizer}, + tokenizer::{Tokenizer, word_tokenizer::word_tokenizer}, utils::{side::Side, string_builder::StringBuilder}, }; diff --git a/src/operation_transformation/operation.rs b/src/operation_transformation/operation.rs index 63702cd..29fdeff 100644 --- a/src/operation_transformation/operation.rs +++ b/src/operation_transformation/operation.rs @@ -245,7 +245,7 @@ where Operation::create_delete_with_text( order + overlap, text.chars() - .skip((deleted_character_count - new_length) as usize) + .skip(deleted_character_count - new_length) .collect::(), ) }, diff --git a/src/operation_transformation/utils/cook_operations.rs b/src/operation_transformation/utils/cook_operations.rs index c642d3c..7d5f85e 100644 --- a/src/operation_transformation/utils/cook_operations.rs +++ b/src/operation_transformation/utils/cook_operations.rs @@ -1,12 +1,13 @@ use crate::{diffs::raw_operation::RawOperation, operation_transformation::Operation}; -/// Turn raw operations into ordered operations while keeping track of indexes. +/// Turn raw operations into ordered operations while keeping track of the +/// original token's indexes. pub fn cook_operations(raw_operations: I) -> impl Iterator> where I: IntoIterator>, T: PartialEq + Clone + std::fmt::Debug, { - let mut order = 0; // this is the start index of the operation on the original text + let mut original_text_index = 0; // this is the start index of the operation on the original text raw_operations.into_iter().map(move |raw_operation| { let length = raw_operation.original_text_length(); @@ -14,24 +15,30 @@ where match raw_operation { RawOperation::Equal(..) => { let op = if cfg!(debug_assertions) { - Operation::create_equal_with_text(order, raw_operation.get_original_text()) + Operation::create_equal_with_text( + original_text_index, + raw_operation.get_original_text(), + ) } else { - Operation::create_equal(order, length) + Operation::create_equal(original_text_index, length) }; - order += length; + original_text_index += length; op } - RawOperation::Insert(tokens) => Operation::create_insert(order, tokens), + RawOperation::Insert(tokens) => Operation::create_insert(original_text_index, tokens), RawOperation::Delete(..) => { let op = if cfg!(debug_assertions) { - Operation::create_delete_with_text(order, raw_operation.get_original_text()) + Operation::create_delete_with_text( + original_text_index, + raw_operation.get_original_text(), + ) } else { - Operation::create_delete(order, length) + Operation::create_delete(original_text_index, length) }; - order += length; + original_text_index += length; op } diff --git a/src/operation_transformation/utils/elongate_operations.rs b/src/operation_transformation/utils/elongate_operations.rs index 212ba7e..1368346 100644 --- a/src/operation_transformation/utils/elongate_operations.rs +++ b/src/operation_transformation/utils/elongate_operations.rs @@ -17,6 +17,9 @@ where let mut maybe_previous_insert: Option> = None; let mut maybe_previous_delete: Option> = None; + // We don't elongate `equals` as they're needed to maintain cursor positions + // when merging against deletes. + let mut result: Vec> = raw_operations .into_iter() .flat_map(|next| match next { @@ -60,68 +63,3 @@ where result } - -// #[cfg(test)] -// mod tests { - -// use super::*; - -// #[test] -// fn test_elongate_operations_empty() { -// let operations: Vec> = vec![]; -// let result = elongate_operations(operations); -// assert_eq!(result, vec![]); -// } - -// #[test] -// fn test_elongate_operations_single_operation() { -// let operations = vec![RawOperation::Insert(vec!["test".into()])]; -// let result = elongate_operations(operations); -// assert_eq!(result.len(), 1); -// assert!(matches!(result[0], RawOperation::Insert(_))); -// } - -// #[test] -// fn test_elongate_operations_interleaved() { -// let operations = vec![ -// RawOperation::Insert(vec!["a".into()]), -// RawOperation::Delete(vec!["b".into()]), -// RawOperation::Insert(vec!["c".into()]), -// RawOperation::Delete(vec!["d".into()]), -// ]; -// let result = elongate_operations(operations); -// assert_eq!(result.len(), 2); -// assert!(matches!(result[0], RawOperation::Insert(_))); -// assert!(matches!(result[1], RawOperation::Delete(_))); -// } - -// #[test] -// fn test_elongate_operations_with_equal() { -// let operations = vec![ -// RawOperation::Equal(vec!["a".into()]), -// RawOperation::Equal(vec!["b".into()]), -// RawOperation::Insert(vec!["c".into()]), -// RawOperation::Insert(vec!["d".into()]), -// ]; -// let result = elongate_operations(operations); -// assert_eq!(result.len(), 2); -// assert!(matches!(result[0], RawOperation::Equal(_))); -// assert!(matches!(result[1], RawOperation::Insert(_))); -// } - -// #[test] -// fn test_elongate_operations_mixed_sequence() { -// let operations = vec![ -// RawOperation::Insert(vec!["a".into()]), -// RawOperation::Equal(vec!["b".into()]), -// RawOperation::Delete(vec!["c".into()]), -// RawOperation::Equal(vec!["d".into()]), -// ]; -// let result = elongate_operations(operations); -// assert_eq!(result.len(), 4); -// assert!(matches!(result[0], RawOperation::Insert(_))); -// assert!(matches!(result[1], RawOperation::Equal(_))); -// assert!(matches!(result[2], RawOperation::Delete(_))); -// assert!(matches!(result[3], RawOperation::Equal(_))); -// } -// } -- 2.47.2 From 9af06183e7837b5d864181babad95465ead67dac Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sun, 22 Jun 2025 15:02:50 +0100 Subject: [PATCH 14/15] Fix lints --- Cargo.toml | 4 ++-- src/operation_transformation/edited_text.rs | 8 -------- src/operation_transformation/operation.rs | 16 ++++++++-------- src/utils/string_builder.rs | 2 +- 4 files changed, 11 insertions(+), 19 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 272e1bf..4adaf56 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -41,12 +41,12 @@ lto = true opt-level = 3 strip="debuginfo" # Keep some info for better panics -[workspace.lints.rust] +[lints.rust] unsafe_code = "forbid" rust_2018_idioms = { level = "warn", priority = -1 } missing_debug_implementations = "warn" -[workspace.lints.clippy] +[lints.clippy] await_holding_lock = "warn" dbg_macro = "warn" empty_enum = "warn" diff --git a/src/operation_transformation/edited_text.rs b/src/operation_transformation/edited_text.rs index 3e37641..4e9c7be 100644 --- a/src/operation_transformation/edited_text.rs +++ b/src/operation_transformation/edited_text.rs @@ -137,7 +137,6 @@ where operation, Operation::Insert { .. } | Operation::Equal { .. } ); - println!("{side} {operation:?} ({maybe_left_op:?}, {maybe_right_op:?})"); let original_length = operation.len() as i64; let result = match side { @@ -152,7 +151,6 @@ where while let Some(cursor) = left_cursors.next_if(|cursor| { cursor.char_index <= seen_left_length + original_length as usize }) { - println!("cursor {}", cursor.char_index); merged_cursors.push( cursor.with_index((cursor.char_index as i64 + shift) as usize), ); @@ -179,8 +177,6 @@ where while let Some(cursor) = right_cursors.next_if(|cursor| { cursor.char_index <= seen_right_length + original_length as usize }) { - println!("cursor {}", cursor.char_index); - merged_cursors.push( cursor.with_index((cursor.char_index as i64 + shift) as usize), ); @@ -198,8 +194,6 @@ where } }; - println!(" = {result:?}"); - if result.len() == 0 { continue; } @@ -233,8 +227,6 @@ where #[cfg(test)] mod tests { - use std::env; - use insta::assert_debug_snapshot; use pretty_assertions::assert_eq; diff --git a/src/operation_transformation/operation.rs b/src/operation_transformation/operation.rs index 29fdeff..6efaa91 100644 --- a/src/operation_transformation/operation.rs +++ b/src/operation_transformation/operation.rs @@ -96,9 +96,9 @@ where fn order(&self) -> usize { match self { - Operation::Equal { order, .. } => *order, - Operation::Insert { order, .. } => *order, - Operation::Delete { order, .. } => *order, + Operation::Equal { order, .. } + | Operation::Insert { order, .. } + | Operation::Delete { order, .. } => *order, } } @@ -145,14 +145,14 @@ where .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()), + text.as_ref().unwrap_or(&String::new()), builder.get_slice_from_remaining(self.len()) ); - builder.retain(*length) + builder.retain(*length); } Operation::Insert { text, .. } => { - builder.insert(&text.iter().map(Token::original).collect::()) + builder.insert(&text.iter().map(Token::original).collect::()); } Operation::Delete { #[cfg(debug_assertions)] @@ -166,11 +166,11 @@ where .as_ref() .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()), + deleted_text.as_ref().unwrap_or(&String::new()), builder.get_slice_from_remaining(self.len()) ); - builder.delete(*deleted_character_count) + builder.delete(*deleted_character_count); } } diff --git a/src/utils/string_builder.rs b/src/utils/string_builder.rs index 5659d3e..af24d82 100644 --- a/src/utils/string_builder.rs +++ b/src/utils/string_builder.rs @@ -19,7 +19,7 @@ impl StringBuilder<'_> { buffer: String::with_capacity(original.len()), #[cfg(debug_assertions)] - remaining: original.to_string(), + remaining: original.to_owned(), } } -- 2.47.2 From f9ac4d1a2d950d70a15e0d4c465aa889eea885ea Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sun, 22 Jun 2025 15:04:24 +0100 Subject: [PATCH 15/15] Add test --- .../utils/elongate_operations.rs | 59 ++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/src/operation_transformation/utils/elongate_operations.rs b/src/operation_transformation/utils/elongate_operations.rs index 1368346..1a218c6 100644 --- a/src/operation_transformation/utils/elongate_operations.rs +++ b/src/operation_transformation/utils/elongate_operations.rs @@ -19,7 +19,6 @@ where // We don't elongate `equals` as they're needed to maintain cursor positions // when merging against deletes. - let mut result: Vec> = raw_operations .into_iter() .flat_map(|next| match next { @@ -63,3 +62,61 @@ where result } + +#[cfg(test)] +mod tests { + use super::*; + use crate::tokenizer::token::Token; + + // Helper constructors for cleaner tests + fn ins(texts: &[&str]) -> RawOperation { + RawOperation::Insert(texts.iter().map(|t| Token::from(*t)).collect()) + } + + fn del(texts: &[&str]) -> RawOperation { + RawOperation::Delete(texts.iter().map(|t| Token::from(*t)).collect()) + } + + fn ins_custom(text: &str, lj: bool, rj: bool) -> RawOperation { + RawOperation::Insert(vec![Token::new(text.to_string(), text.to_string(), lj, rj)]) + } + + #[test] + fn merges_adjacent_joinable_inserts() { + let ops = vec![ins(&["a"]), ins(&["b"]), ins(&["c"])]; + let result = elongate_operations(ops); + assert_eq!(result.len(), 1); + match &result[0] { + RawOperation::Insert(tokens) => { + let originals: String = tokens.iter().map(|t| t.original()).collect(); + assert_eq!(originals, "abc"); + } + _ => panic!("Expected single Insert operation"), + } + } + + #[test] + fn does_not_merge_when_not_joinable() { + let ops = vec![ + ins_custom("a", true, false), // not right-joinable + ins_custom("b", true, true), // left-joinable but previous isn't right-joinable + ]; + let result = elongate_operations(ops); + assert_eq!( + result.len(), + 2, + "Operations should remain separate when not joinable" + ); + } + + #[test] + fn merges_interleaved_insert_delete_sequences() { + // Pattern IDID -> II DD + let ops = vec![ins(&["i1"]), del(&["d1"]), ins(&["i2"]), del(&["d2"])]; + let result = elongate_operations(ops); + + assert_eq!(result.len(), 2); + assert!(matches!(result[0], RawOperation::Delete(_))); + assert!(matches!(result[1], RawOperation::Insert(_))); + } +} -- 2.47.2