From 328e0ac73afffb706a2f3da46450d6929b541c00 Mon Sep 17 00:00:00 2001 From: appflowy Date: Sat, 10 Sep 2022 10:05:27 +0800 Subject: [PATCH] refactor: rename && fix potential bugs --- .../lib-ot/src/core/document/document.rs | 123 +++++++++--------- .../lib-ot/src/core/document/transaction.rs | 20 +-- shared-lib/lib-ot/src/errors.rs | 10 +- shared-lib/lib-ot/tests/node/script.rs | 2 +- 4 files changed, 75 insertions(+), 80 deletions(-) diff --git a/shared-lib/lib-ot/src/core/document/document.rs b/shared-lib/lib-ot/src/core/document/document.rs index 5c362c5fac..ead107fe73 100644 --- a/shared-lib/lib-ot/src/core/document/document.rs +++ b/shared-lib/lib-ot/src/core/document/document.rs @@ -21,6 +21,10 @@ impl NodeTree { NodeTree { arena, root } } + pub fn get_node_data(&self, node_id: NodeId) -> Option<&NodeData> { + Some(self.arena.get(node_id)?.get()) + } + /// /// # Examples /// @@ -44,7 +48,7 @@ impl NodeTree { let mut iterate_node = self.root; for id in path.iter() { - iterate_node = self.child_from_node_with_index(iterate_node, *id)?; + iterate_node = self.child_from_node_at_index(iterate_node, *id)?; } Some(iterate_node) } @@ -54,14 +58,10 @@ impl NodeTree { let mut current_node = node_id; // Use .skip(1) on the ancestors iterator to skip the root node. let mut ancestors = node_id.ancestors(&self.arena).skip(1); - let mut parent = ancestors.next(); - - while parent.is_some() { - let parent_node = parent.unwrap(); + while let Some(parent_node) = ancestors.next() { let counter = self.index_of_node(parent_node, current_node); path.push(counter); current_node = parent_node; - parent = ancestors.next(); } Path(path) @@ -69,14 +69,11 @@ impl NodeTree { fn index_of_node(&self, parent_node: NodeId, child_node: NodeId) -> usize { let mut counter: usize = 0; - let mut children_iterator = parent_node.children(&self.arena); - let mut node = children_iterator.next(); - while node.is_some() { - if node.unwrap() == child_node { + let mut iter = parent_node.children(&self.arena); + while let Some(node) = iter.next() { + if node == child_node { return counter; } - - node = children_iterator.next(); counter += 1; } @@ -86,7 +83,7 @@ impl NodeTree { /// /// # Arguments /// - /// * `at_node`: + /// * `node_id`: /// * `index`: /// /// returns: Option @@ -105,8 +102,8 @@ impl NodeTree { /// let inserted_data = node_tree.get_node_data(inserted_note).unwrap(); /// assert_eq!(inserted_data.node_type, node.note_type); /// ``` - pub fn child_from_node_with_index(&self, at_node: NodeId, index: usize) -> Option { - let children = at_node.children(&self.arena); + pub fn child_from_node_at_index(&self, node_id: NodeId, index: usize) -> Option { + let children = node_id.children(&self.arena); for (counter, child) in children.enumerate() { if counter == index { return Some(child); @@ -120,10 +117,6 @@ impl NodeTree { node_id.children(&self.arena) } - pub fn get_node_data(&self, node_id: NodeId) -> Option<&NodeData> { - Some(self.arena.get(node_id)?.get()) - } - /// /// # Arguments /// @@ -151,14 +144,14 @@ impl NodeTree { pub fn apply_op(&mut self, op: &NodeOperation) -> Result<(), OTError> { match op { - NodeOperation::Insert { path, nodes } => self.apply_insert(path, nodes), - NodeOperation::Update { path, attributes, .. } => self.apply_update(path, attributes), - NodeOperation::Delete { path, nodes } => self.apply_delete(path, nodes.len()), - NodeOperation::TextEdit { path, delta, .. } => self.apply_text_edit(path, delta), + NodeOperation::Insert { path, nodes } => self.insert_nodes(path, nodes), + NodeOperation::Update { path, attributes, .. } => self.update_node(path, attributes), + NodeOperation::Delete { path, nodes } => self.delete_node(path, nodes), + NodeOperation::TextEdit { path, delta, .. } => self.update_delta(path, delta), } } - fn apply_insert(&mut self, path: &Path, nodes: &[Node]) -> Result<(), OTError> { + fn insert_nodes(&mut self, path: &Path, nodes: &[Node]) -> Result<(), OTError> { debug_assert!(!path.is_empty()); if path.is_empty() { return Err(OTErrorCode::PathIsEmpty.into()); @@ -170,22 +163,22 @@ impl NodeTree { .node_at_path(parent_path) .ok_or_else(|| ErrorBuilder::new(OTErrorCode::PathNotFound).build())?; - self.insert_child_at_index(parent_node, last_index, nodes) + self.insert_nodes_at_index(parent_node, last_index, nodes) } - fn insert_child_at_index(&mut self, parent: NodeId, index: usize, insert_children: &[Node]) -> Result<(), OTError> { + fn insert_nodes_at_index(&mut self, parent: NodeId, index: usize, insert_children: &[Node]) -> Result<(), OTError> { if index == 0 && parent.children(&self.arena).next().is_none() { - self.append_subtree(&parent, insert_children); + self.append_nodes(&parent, insert_children); return Ok(()); } if index == parent.children(&self.arena).count() { - self.append_subtree(&parent, insert_children); + self.append_nodes(&parent, insert_children); return Ok(()); } let node_to_insert = self - .child_from_node_with_index(parent, index) + .child_from_node_at_index(parent, index) .ok_or_else(|| ErrorBuilder::new(OTErrorCode::PathNotFound).build())?; self.insert_subtree_before(&node_to_insert, insert_children); @@ -193,12 +186,12 @@ impl NodeTree { } // recursive append the subtrees to the node - fn append_subtree(&mut self, parent: &NodeId, insert_children: &[Node]) { + fn append_nodes(&mut self, parent: &NodeId, insert_children: &[Node]) { for child in insert_children { let child_id = self.arena.new_node(child.into()); parent.append(child_id, &mut self.arena); - self.append_subtree(&child_id, &child.children); + self.append_nodes(&child_id, &child.children); } } @@ -207,32 +200,23 @@ impl NodeTree { let child_id = self.arena.new_node(child.into()); before.insert_before(child_id, &mut self.arena); - self.append_subtree(&child_id, &child.children); + self.append_nodes(&child_id, &child.children); } } - fn apply_update(&mut self, path: &Path, attributes: &NodeAttributes) -> Result<(), OTError> { - let update_node = self - .node_at_path(path) - .ok_or_else(|| ErrorBuilder::new(OTErrorCode::PathNotFound).build())?; - let node_data = self.arena.get_mut(update_node).unwrap(); - let new_node = { - let old_attributes = &node_data.get().attributes; - let new_attributes = NodeAttributes::compose(old_attributes, attributes)?; - NodeData { - attributes: new_attributes, - ..node_data.get().clone() - } - }; - *node_data.get_mut() = new_node; - Ok(()) + fn update_node(&mut self, path: &Path, attributes: &NodeAttributes) -> Result<(), OTError> { + self.mut_node_at_path(path, |node_data| { + let new_attributes = NodeAttributes::compose(&node_data.attributes, attributes)?; + node_data.attributes = new_attributes; + Ok(()) + }) } - fn apply_delete(&mut self, path: &Path, len: usize) -> Result<(), OTError> { + fn delete_node(&mut self, path: &Path, nodes: &[Node]) -> Result<(), OTError> { let mut update_node = self .node_at_path(path) .ok_or_else(|| ErrorBuilder::new(OTErrorCode::PathNotFound).build())?; - for _ in 0..len { + for _ in 0..nodes.len() { let next = update_node.following_siblings(&self.arena).next(); update_node.remove_subtree(&mut self.arena); if let Some(next_id) = next { @@ -244,22 +228,35 @@ impl NodeTree { Ok(()) } - fn apply_text_edit(&mut self, path: &Path, delta: &TextDelta) -> Result<(), OTError> { - let edit_node = self + fn update_delta(&mut self, path: &Path, delta: &TextDelta) -> Result<(), OTError> { + self.mut_node_at_path(path, |node_data| { + if node_data.delta.is_none() { + let msg = format!("The delta of the node at path:{:?} should not be empty", path); + tracing::error!("{}", msg); + return Err(OTError::new(OTErrorCode::UnexpectedEmpty, msg)); + } + let new_delta = node_data.delta.as_ref().unwrap().compose(delta)?; + let _ = node_data.delta = Some(new_delta); + Ok(()) + })?; + + Ok(()) + } + + fn mut_node_at_path(&mut self, path: &Path, f: F) -> Result<(), OTError> + where + F: Fn(&mut NodeData) -> Result<(), OTError>, + { + let node_id = self .node_at_path(path) .ok_or_else(|| ErrorBuilder::new(OTErrorCode::PathNotFound).build())?; - let node_data = self.arena.get_mut(edit_node).unwrap(); - let new_delta = if let Some(old_delta) = &node_data.get().delta { - Some(old_delta.compose(delta)?) - } else { - None - }; - if let Some(new_delta) = new_delta { - *node_data.get_mut() = NodeData { - delta: Some(new_delta), - ..node_data.get().clone() - }; - }; + match self.arena.get_mut(node_id) { + None => tracing::warn!("The path: {:?} does not contain any nodes", path), + Some(node) => { + let node_data = node.get_mut(); + let _ = f(node_data)?; + } + } Ok(()) } } diff --git a/shared-lib/lib-ot/src/core/document/transaction.rs b/shared-lib/lib-ot/src/core/document/transaction.rs index 52ab794f44..dfc9be4867 100644 --- a/shared-lib/lib-ot/src/core/document/transaction.rs +++ b/shared-lib/lib-ot/src/core/document/transaction.rs @@ -1,5 +1,5 @@ use crate::core::document::position::Path; -use crate::core::{AttributeValue, Node, NodeAttributes, NodeOperation, NodeTree}; +use crate::core::{Node, NodeAttributes, NodeOperation, NodeTree}; use indextree::NodeId; pub struct Transaction { @@ -13,14 +13,14 @@ impl Transaction { } pub struct TransactionBuilder<'a> { - document: &'a NodeTree, + node_tree: &'a NodeTree, operations: Vec, } impl<'a> TransactionBuilder<'a> { - pub fn new(document: &'a NodeTree) -> TransactionBuilder { + pub fn new(node_tree: &'a NodeTree) -> TransactionBuilder { TransactionBuilder { - document, + node_tree, operations: Vec::new(), } } @@ -82,8 +82,8 @@ impl<'a> TransactionBuilder<'a> { pub fn update_attributes_at_path(self, path: &Path, attributes: NodeAttributes) -> Self { let mut old_attributes = NodeAttributes::new(); - let node = self.document.node_at_path(path).unwrap(); - let node_data = self.document.get_node_data(node).unwrap(); + let node = self.node_tree.node_at_path(path).unwrap(); + let node_data = self.node_tree.get_node_data(node).unwrap(); for key in attributes.keys() { let old_attrs = &node_data.attributes; @@ -104,11 +104,11 @@ impl<'a> TransactionBuilder<'a> { } pub fn delete_nodes_at_path(mut self, path: &Path, length: usize) -> Self { - let mut node = self.document.node_at_path(path).unwrap(); + let mut node = self.node_tree.node_at_path(path).unwrap(); let mut deleted_nodes = vec![]; for _ in 0..length { deleted_nodes.push(self.get_deleted_nodes(node)); - node = self.document.following_siblings(node).next().unwrap(); + node = self.node_tree.following_siblings(node).next().unwrap(); } self.operations.push(NodeOperation::Delete { @@ -119,10 +119,10 @@ impl<'a> TransactionBuilder<'a> { } fn get_deleted_nodes(&self, node_id: NodeId) -> Node { - let node_data = self.document.get_node_data(node_id).unwrap(); + let node_data = self.node_tree.get_node_data(node_id).unwrap(); let mut children = vec![]; - self.document.children_from_node(node_id).for_each(|child_id| { + self.node_tree.children_from_node(node_id).for_each(|child_id| { children.push(self.get_deleted_nodes(child_id)); }); diff --git a/shared-lib/lib-ot/src/errors.rs b/shared-lib/lib-ot/src/errors.rs index 449c662883..fe2a03b206 100644 --- a/shared-lib/lib-ot/src/errors.rs +++ b/shared-lib/lib-ot/src/errors.rs @@ -25,11 +25,8 @@ impl std::convert::From for OTError { } impl OTError { - pub fn new(code: OTErrorCode, msg: &str) -> OTError { - Self { - code, - msg: msg.to_owned(), - } + pub fn new(code: OTErrorCode, msg: String) -> OTError { + Self { code, msg } } pub fn context(mut self, error: T) -> Self { @@ -76,6 +73,7 @@ pub enum OTErrorCode { Internal, PathNotFound, PathIsEmpty, + UnexpectedEmpty, } pub struct ErrorBuilder { @@ -105,6 +103,6 @@ impl ErrorBuilder { } pub fn build(mut self) -> OTError { - OTError::new(self.code, &self.msg.take().unwrap_or_else(|| "".to_owned())) + OTError::new(self.code, self.msg.take().unwrap_or_else(|| "".to_owned())) } } diff --git a/shared-lib/lib-ot/tests/node/script.rs b/shared-lib/lib-ot/tests/node/script.rs index c263a1330c..898a4a976e 100644 --- a/shared-lib/lib-ot/tests/node/script.rs +++ b/shared-lib/lib-ot/tests/node/script.rs @@ -1,4 +1,4 @@ -use lib_ot::core::{Node, NodeAttributes, NodeTree, Path, TransactionBuilder}; +use lib_ot::core::{Node, NodeAttributes, NodeTree, Path, TextDelta, TransactionBuilder}; pub enum NodeScript { InsertNode { path: Path, node: Node },