From bc5548ff75a020630f96adf03a0bcb40c48d7093 Mon Sep 17 00:00:00 2001 From: "Nathan.fooo" <86001920+appflowy@users.noreply.github.com> Date: Sun, 30 Oct 2022 12:54:07 +0800 Subject: [PATCH] fix: changeset composing (#1398) --- frontend/app_flowy/pubspec.lock | 2 +- .../flowy-document/tests/editor/op_test.rs | 3 +- shared-lib/lib-ot/src/core/delta/builder.rs | 12 +- .../src/core/delta/operation/builder.rs | 51 ----- .../lib-ot/src/core/delta/operation/mod.rs | 2 - shared-lib/lib-ot/src/core/delta/ops.rs | 4 + shared-lib/lib-ot/src/core/node_tree/node.rs | 2 +- .../lib-ot/src/core/node_tree/operation.rs | 10 - .../tests/node/changeset_compose_test.rs | 194 ++++++++++++++++++ shared-lib/lib-ot/tests/node/mod.rs | 2 + .../tests/node/operation_attribute_test.rs | 4 +- .../tests/node/operation_compose_test.rs | 135 ++++++++++++ shared-lib/lib-ot/tests/node/script.rs | 12 ++ 13 files changed, 358 insertions(+), 75 deletions(-) delete mode 100644 shared-lib/lib-ot/src/core/delta/operation/builder.rs create mode 100644 shared-lib/lib-ot/tests/node/changeset_compose_test.rs create mode 100644 shared-lib/lib-ot/tests/node/operation_compose_test.rs diff --git a/frontend/app_flowy/pubspec.lock b/frontend/app_flowy/pubspec.lock index 823a744857..2711b7651d 100644 --- a/frontend/app_flowy/pubspec.lock +++ b/frontend/app_flowy/pubspec.lock @@ -35,7 +35,7 @@ packages: path: "packages/appflowy_editor" relative: true source: path - version: "0.0.6" + version: "0.0.7" appflowy_popover: dependency: "direct main" description: diff --git a/frontend/rust-lib/flowy-document/tests/editor/op_test.rs b/frontend/rust-lib/flowy-document/tests/editor/op_test.rs index b51febfedb..d1cec0c7f0 100644 --- a/frontend/rust-lib/flowy-document/tests/editor/op_test.rs +++ b/frontend/rust-lib/flowy-document/tests/editor/op_test.rs @@ -36,8 +36,7 @@ fn attributes_insert_text_at_middle() { #[test] fn delta_get_ops_in_interval_1() { - let operations = OperationsBuilder::new().insert("123").insert("4").build(); - let delta = DeltaTextOperationBuilder::from_operations(operations); + let delta = DeltaTextOperationBuilder::new().insert("123").insert("4").build(); let mut iterator = OperationIterator::from_interval(&delta, Interval::new(0, 4)); assert_eq!(iterator.ops(), delta.ops); diff --git a/shared-lib/lib-ot/src/core/delta/builder.rs b/shared-lib/lib-ot/src/core/delta/builder.rs index 75014e1ec1..805d58580a 100644 --- a/shared-lib/lib-ot/src/core/delta/builder.rs +++ b/shared-lib/lib-ot/src/core/delta/builder.rs @@ -1,6 +1,5 @@ use crate::core::delta::operation::OperationAttributes; use crate::core::delta::{trim, DeltaOperations}; -use crate::core::DeltaOperation; /// A builder for creating new [Operations] objects. /// @@ -39,12 +38,13 @@ where DeltaOperationBuilder::default() } - pub fn from_operations(operations: Vec>) -> DeltaOperations { - let mut delta = DeltaOperationBuilder::default().build(); - operations.into_iter().for_each(|operation| { - delta.add(operation); + pub fn from_delta_operation(delta_operation: DeltaOperations) -> Self { + debug_assert!(delta_operation.utf16_base_len == 0); + let mut builder = DeltaOperationBuilder::new(); + delta_operation.ops.into_iter().for_each(|operation| { + builder.delta.add(operation); }); - delta + builder } /// Retain the 'n' characters with the attributes. Use 'retain' instead if you don't diff --git a/shared-lib/lib-ot/src/core/delta/operation/builder.rs b/shared-lib/lib-ot/src/core/delta/operation/builder.rs deleted file mode 100644 index bdea0e80ae..0000000000 --- a/shared-lib/lib-ot/src/core/delta/operation/builder.rs +++ /dev/null @@ -1,51 +0,0 @@ -use crate::core::delta::operation::{DeltaOperation, EmptyAttributes, OperationAttributes}; - -// pub type RichTextOpBuilder = OperationsBuilder; -pub type PlainTextOpBuilder = OperationsBuilder; - -#[derive(Default)] -pub struct OperationsBuilder { - operations: Vec>, -} - -impl OperationsBuilder -where - T: OperationAttributes, -{ - pub fn new() -> OperationsBuilder { - OperationsBuilder::default() - } - - pub fn retain_with_attributes(mut self, n: usize, attributes: T) -> OperationsBuilder { - let retain = DeltaOperation::retain_with_attributes(n, attributes); - self.operations.push(retain); - self - } - - pub fn retain(mut self, n: usize) -> OperationsBuilder { - let retain = DeltaOperation::retain(n); - self.operations.push(retain); - self - } - - pub fn delete(mut self, n: usize) -> OperationsBuilder { - self.operations.push(DeltaOperation::Delete(n)); - self - } - - pub fn insert_with_attributes(mut self, s: &str, attributes: T) -> OperationsBuilder { - let insert = DeltaOperation::insert_with_attributes(s, attributes); - self.operations.push(insert); - self - } - - pub fn insert(mut self, s: &str) -> OperationsBuilder { - let insert = DeltaOperation::insert(s); - self.operations.push(insert); - self - } - - pub fn build(self) -> Vec> { - self.operations - } -} diff --git a/shared-lib/lib-ot/src/core/delta/operation/mod.rs b/shared-lib/lib-ot/src/core/delta/operation/mod.rs index 814cb76794..067c55e98a 100644 --- a/shared-lib/lib-ot/src/core/delta/operation/mod.rs +++ b/shared-lib/lib-ot/src/core/delta/operation/mod.rs @@ -1,8 +1,6 @@ #![allow(clippy::module_inception)] -mod builder; mod operation; mod operation_serde; -pub use builder::*; pub use operation::*; pub use operation_serde::*; diff --git a/shared-lib/lib-ot/src/core/delta/ops.rs b/shared-lib/lib-ot/src/core/delta/ops.rs index 32bca15859..2d41278198 100644 --- a/shared-lib/lib-ot/src/core/delta/ops.rs +++ b/shared-lib/lib-ot/src/core/delta/ops.rs @@ -224,6 +224,10 @@ where Ok(new_s) } + pub fn inverted(&self) -> Self { + self.invert_str("") + } + /// Computes the inverse [Delta]. The inverse of an operation is the /// operation that reverts the effects of the operation /// # Arguments diff --git a/shared-lib/lib-ot/src/core/node_tree/node.rs b/shared-lib/lib-ot/src/core/node_tree/node.rs index f0ce53e6d8..2608912645 100644 --- a/shared-lib/lib-ot/src/core/node_tree/node.rs +++ b/shared-lib/lib-ot/src/core/node_tree/node.rs @@ -212,7 +212,7 @@ impl Changeset { inverted: _, }, ) => { - let original = delta.invert(inverted); + let original = delta.compose(inverted)?; let new_delta = delta.compose(other_delta)?; let new_inverted = new_delta.invert(&original); diff --git a/shared-lib/lib-ot/src/core/node_tree/operation.rs b/shared-lib/lib-ot/src/core/node_tree/operation.rs index d432b8456a..7314584b89 100644 --- a/shared-lib/lib-ot/src/core/node_tree/operation.rs +++ b/shared-lib/lib-ot/src/core/node_tree/operation.rs @@ -234,16 +234,6 @@ impl NodeOperations { } } - // if let Some(operations) = self.inner.get_mut(other.get_path()) { - // if let Some(last_operation) = operations.last_mut() { - // if last_operation.can_compose(&other) { - // let mut_operation = Arc::make_mut(last_operation); - // if mut_operation.compose(&other).is_ok() { - // return; - // } - // } - // } - // } // If the passed-in operation can't be composed, then append it to the end. self.inner.push(other); } diff --git a/shared-lib/lib-ot/tests/node/changeset_compose_test.rs b/shared-lib/lib-ot/tests/node/changeset_compose_test.rs new file mode 100644 index 0000000000..7e800952be --- /dev/null +++ b/shared-lib/lib-ot/tests/node/changeset_compose_test.rs @@ -0,0 +1,194 @@ +use crate::node::script::NodeScript::*; +use crate::node::script::NodeTest; +use lib_ot::core::{AttributeEntry, Changeset, NodeData, OperationTransform}; +use lib_ot::text_delta::DeltaTextOperationBuilder; + +#[test] +fn changeset_delta_compose_delta_test() { + // delta 1 + let delta_1 = DeltaTextOperationBuilder::new().insert("Hello world").build(); + let inverted_1 = delta_1.inverted(); + let mut changeset_1 = Changeset::Delta { + delta: delta_1.clone(), + inverted: inverted_1, + }; + + // delta 2 + let delta_2 = DeltaTextOperationBuilder::new() + .retain(delta_1.utf16_target_len) + .insert("!") + .build(); + let inverted_2 = delta_2.inverted(); + let changeset_2 = Changeset::Delta { + delta: delta_2, + inverted: inverted_2, + }; + + // compose + changeset_1.compose(&changeset_2).unwrap(); + + if let Changeset::Delta { delta, inverted } = changeset_1 { + assert_eq!(delta.content().unwrap(), "Hello world!"); + let new_delta = delta.compose(&inverted).unwrap(); + assert_eq!(new_delta.content().unwrap(), ""); + } +} + +#[test] +fn operation_compose_delta_changeset_then_invert_test() { + let delta = DeltaTextOperationBuilder::new().insert("Hello world").build(); + let inverted = delta.inverted(); + let changeset = Changeset::Delta { + delta: delta.clone(), + inverted: inverted.clone(), + }; + + let mut test = NodeTest::new(); + let text_node = NodeData::new("text"); + let scripts = vec![ + InsertNode { + path: 0.into(), + node_data: text_node, + rev_id: 1, + }, + UpdateBody { + path: 0.into(), + changeset: changeset.clone(), + }, + AssertNodeDelta { + path: 0.into(), + expected: delta.clone(), + }, + UpdateBody { + path: 0.into(), + changeset: changeset.inverted(), + }, + AssertNodeDelta { + path: 0.into(), + expected: delta.compose(&inverted).unwrap(), + }, + ]; + test.run_scripts(scripts); +} + +#[test] +fn operation_compose_multiple_delta_changeset_then_invert_test() { + // delta 1 + let delta_1 = DeltaTextOperationBuilder::new().insert("Hello world").build(); + let inverted_1 = delta_1.inverted(); + let changeset_1 = Changeset::Delta { + delta: delta_1.clone(), + inverted: inverted_1, + }; + + // delta 2 + let delta_2 = DeltaTextOperationBuilder::new() + .retain(delta_1.utf16_target_len) + .insert("!") + .build(); + let inverted_2 = delta_2.inverted(); + let changeset_2 = Changeset::Delta { + delta: delta_2.clone(), + inverted: inverted_2, + }; + + // delta 3 + let delta_3 = DeltaTextOperationBuilder::new() + .retain(delta_2.utf16_target_len) + .insert("AppFlowy") + .build(); + let inverted_3 = delta_3.inverted(); + let changeset_3 = Changeset::Delta { + delta: delta_3.clone(), + inverted: inverted_3, + }; + + let mut test = NodeTest::new(); + let text_node = NodeData::new("text"); + let scripts = vec![ + InsertNode { + path: 0.into(), + node_data: text_node, + rev_id: 1, + }, + UpdateBody { + path: 0.into(), + changeset: changeset_1.clone(), + }, + UpdateBody { + path: 0.into(), + changeset: changeset_2.clone(), + }, + UpdateBody { + path: 0.into(), + changeset: changeset_3.clone(), + }, + AssertNodeDelta { + path: 0.into(), + expected: delta_1.compose(&delta_2).unwrap().compose(&delta_3).unwrap(), + }, + UpdateBody { + path: 0.into(), + changeset: changeset_3.inverted(), + }, + AssertNodeDeltaContent { + path: 0.into(), + expected: r#"Hello world!"#, + }, + UpdateBody { + path: 0.into(), + changeset: changeset_2.inverted(), + }, + AssertNodeDeltaContent { + path: 0.into(), + expected: r#"Hello world"#, + }, + UpdateBody { + path: 0.into(), + changeset: changeset_1.inverted(), + }, + AssertNodeDeltaContent { + path: 0.into(), + expected: r#""#, + }, + ]; + test.run_scripts(scripts); +} + +#[test] +#[should_panic] +fn changeset_delta_compose_attributes_test() { + // delta 1 + let delta = DeltaTextOperationBuilder::new().insert("Hello world").build(); + let inverted = delta.inverted(); + let mut delta_changeset = Changeset::Delta { delta, inverted }; + + // attributes + let attribute_changeset = Changeset::Attributes { + new: Default::default(), + old: Default::default(), + }; + + // compose + delta_changeset.compose(&attribute_changeset).unwrap(); +} + +#[test] +fn changeset_attributes_compose_attributes_test() { + // attributes + let mut changeset_1 = Changeset::Attributes { + new: AttributeEntry::new("bold", true).into(), + old: Default::default(), + }; + + let changeset_2 = Changeset::Attributes { + new: AttributeEntry::new("Italic", true).into(), + old: Default::default(), + }; + // compose + changeset_1.compose(&changeset_2).unwrap(); + + if let Changeset::Attributes { new, old: _ } = changeset_1 { + assert_eq!(new, AttributeEntry::new("Italic", true).into()); + } +} diff --git a/shared-lib/lib-ot/tests/node/mod.rs b/shared-lib/lib-ot/tests/node/mod.rs index dd3f7c446f..904f44f4d8 100644 --- a/shared-lib/lib-ot/tests/node/mod.rs +++ b/shared-lib/lib-ot/tests/node/mod.rs @@ -1,4 +1,6 @@ +mod changeset_compose_test; mod operation_attribute_test; +mod operation_compose_test; mod operation_delete_test; mod operation_delta_test; mod operation_insert_test; diff --git a/shared-lib/lib-ot/tests/node/operation_attribute_test.rs b/shared-lib/lib-ot/tests/node/operation_attribute_test.rs index 77690e267e..cb7308d9d6 100644 --- a/shared-lib/lib-ot/tests/node/operation_attribute_test.rs +++ b/shared-lib/lib-ot/tests/node/operation_attribute_test.rs @@ -9,7 +9,7 @@ fn operation_update_attribute_with_float_value_test() { let scripts = vec![ InsertNode { path: 0.into(), - node_data: text_node.clone(), + node_data: text_node, rev_id: 1, }, UpdateBody { @@ -34,7 +34,7 @@ fn operation_update_attribute_with_negative_value_test() { let scripts = vec![ InsertNode { path: 0.into(), - node_data: text_node.clone(), + node_data: text_node, rev_id: 1, }, UpdateBody { diff --git a/shared-lib/lib-ot/tests/node/operation_compose_test.rs b/shared-lib/lib-ot/tests/node/operation_compose_test.rs new file mode 100644 index 0000000000..698add3387 --- /dev/null +++ b/shared-lib/lib-ot/tests/node/operation_compose_test.rs @@ -0,0 +1,135 @@ +use lib_ot::core::{Changeset, NodeOperation}; + +#[test] +fn operation_insert_compose_delta_update_test() { + let insert_operation = NodeOperation::Insert { + path: 0.into(), + nodes: vec![], + }; + + let update_operation = NodeOperation::Update { + path: 0.into(), + changeset: Changeset::Delta { + delta: Default::default(), + inverted: Default::default(), + }, + }; + + assert!(insert_operation.can_compose(&update_operation)) +} + +#[test] +fn operation_insert_compose_attribute_update_test() { + let insert_operation = NodeOperation::Insert { + path: 0.into(), + nodes: vec![], + }; + + let update_operation = NodeOperation::Update { + path: 0.into(), + changeset: Changeset::Attributes { + new: Default::default(), + old: Default::default(), + }, + }; + + assert!(!insert_operation.can_compose(&update_operation)) +} +#[test] +fn operation_insert_compose_update_with_diff_path_test() { + let insert_operation = NodeOperation::Insert { + path: 0.into(), + nodes: vec![], + }; + + let update_operation = NodeOperation::Update { + path: 1.into(), + changeset: Changeset::Attributes { + new: Default::default(), + old: Default::default(), + }, + }; + + assert!(!insert_operation.can_compose(&update_operation)) +} + +#[test] +fn operation_insert_compose_insert_operation_test() { + let insert_operation = NodeOperation::Insert { + path: 0.into(), + nodes: vec![], + }; + + assert!(!insert_operation.can_compose(&NodeOperation::Insert { + path: 0.into(), + nodes: vec![], + }),) +} + +#[test] +fn operation_update_compose_insert_operation_test() { + let update_operation = NodeOperation::Update { + path: 0.into(), + changeset: Changeset::Attributes { + new: Default::default(), + old: Default::default(), + }, + }; + + assert!(!update_operation.can_compose(&NodeOperation::Insert { + path: 0.into(), + nodes: vec![], + })) +} +#[test] +fn operation_update_compose_update_test() { + let update_operation_1 = NodeOperation::Update { + path: 0.into(), + changeset: Changeset::Attributes { + new: Default::default(), + old: Default::default(), + }, + }; + let update_operation_2 = NodeOperation::Update { + path: 0.into(), + changeset: Changeset::Attributes { + new: Default::default(), + old: Default::default(), + }, + }; + + assert!(update_operation_1.can_compose(&update_operation_2)); +} +#[test] +fn operation_update_compose_update_with_diff_path_test() { + let update_operation_1 = NodeOperation::Update { + path: 0.into(), + changeset: Changeset::Attributes { + new: Default::default(), + old: Default::default(), + }, + }; + let update_operation_2 = NodeOperation::Update { + path: 1.into(), + changeset: Changeset::Attributes { + new: Default::default(), + old: Default::default(), + }, + }; + + assert!(!update_operation_1.can_compose(&update_operation_2)); +} + +#[test] +fn operation_insert_compose_insert_test() { + let insert_operation_1 = NodeOperation::Insert { + path: 0.into(), + nodes: vec![], + }; + let insert_operation_2 = NodeOperation::Insert { + path: 0.into(), + nodes: vec![], + }; + + assert!(!insert_operation_1.can_compose(&insert_operation_2)); +} diff --git a/shared-lib/lib-ot/tests/node/script.rs b/shared-lib/lib-ot/tests/node/script.rs index a8a35b8ab9..7fff043653 100644 --- a/shared-lib/lib-ot/tests/node/script.rs +++ b/shared-lib/lib-ot/tests/node/script.rs @@ -55,6 +55,10 @@ pub enum NodeScript { path: Path, expected: DeltaTextOperations, }, + AssertNodeDeltaContent { + path: Path, + expected: &'static str, + }, #[allow(dead_code)] AssertTreeJSON { expected: String, @@ -165,6 +169,14 @@ impl NodeTest { panic!("Node body type not match, expect Delta"); } } + NodeScript::AssertNodeDeltaContent { path, expected } => { + let node = self.node_tree.get_node_at_path(&path).unwrap(); + if let Body::Delta(delta) = node.body.clone() { + debug_assert_eq!(delta.content().unwrap(), expected); + } else { + panic!("Node body type not match, expect Delta"); + } + } NodeScript::AssertTreeJSON { expected } => { let json = serde_json::to_string(&self.node_tree).unwrap(); assert_eq!(json, expected)