Merge pull request #32 from niklak/feature/improve-selection-append-s…

…election

improve `Selection::append_selection` and `Selection::replace_with_selection`

CHANGELOG.md

@@ -9,13 +9,15 @@ All notable changes to the `dom_query` crate will be documented in this file.
 - Using `Tree::merge_with_fn` instead of `Tree::merge` to reduce code duplication.
 - `Tree::child_ids_of_it` now require `rev` argument. Set `true` to iterate children in reverse order.
 - `NodeRef::children_it` now require `rev` argument. Set `true` to iterate children in reverse order.
+- Improved internal logic of `Selection::append_selection` and `Selection::replace_with_selection`.
 
 ### Added
 - Introduced new `NodeRef::prepend_child` method, that inserts a child at the beginning of node content.
 - Introduced new `NodeRef::prepend_children` method, that inserts a child and its siblings at the beginning of the node content.
 - Introduced new `NodeRef::prepend_html` method, that parses html string and inserts its parsed nodes at the beginning of the node content.
 - Introduced new `Selection::prepend_html` method, which parses an HTML string and inserts its parsed nodes at the beginning of the content of all matched nodes.
 - Introduced new selection methods: `Selection::add_selection`, `Selection:add_matcher`, `Selection::add` and `Selection::try_add` to extend current selection with other selections.
+
 ### Fixed
 - Fixed `Selection::append_selection` to work with selections with multiple nodes and selections from another tree.
 - Fixed `Selection::replace_with_selection` to work with selections with multiple nodes and selections from another tree.

src/dom_tree.rs

@@ -5,6 +5,7 @@ use html5ever::LocalName;
 use html5ever::{namespace_url, ns, QualName};
 use tendril::StrTendril;
 
+use crate::entities::InnerHashMap;
 use crate::node::{ancestor_nodes, child_nodes, AncestorNodes, ChildNodes};
 use crate::node::{Element, NodeData, NodeId, NodeRef, TreeNode};
 
@@ -519,4 +520,91 @@ impl Tree {
         self.merge(other);
         f(new_node_id);
     }
+
+    ///Adds a copy of the node and its children to the current tree
+    ///
+    /// # Arguments
+    ///
+    /// * `node` - reference to a node in the some tree
+    ///
+    /// # Returns
+    ///
+    /// * `NodeId` - id of the new node, that was added into the current tree
+    pub(crate) fn copy_node(&self, node: &NodeRef) -> NodeId {
+        let base_id = self.get_new_id();
+        let mut next_id_val = base_id.value;
+
+        let mut id_map: InnerHashMap<usize, usize> = InnerHashMap::default();
+        id_map.insert(node.id.value, next_id_val);
+
+        let mut ops = vec![node.clone()];
+
+        while let Some(op) = ops.pop() {
+            for child in op.children_it(false) {
+                next_id_val += 1;
+                id_map.insert(child.id.value, next_id_val);
+            }
+
+            ops.extend(op.children_it(true));
+        }
+
+        // source tree may be the same tree that owns the copy_node fn, and may be not.
+        let source_tree = node.tree;
+        let new_nodes = self.copy_tree_nodes(source_tree, &id_map);
+
+        let mut nodes = self.nodes.borrow_mut();
+        nodes.extend(new_nodes);
+
+        base_id
+    }
+
+
+    fn copy_tree_nodes(&self, source_tree: &Tree, id_map: &InnerHashMap<usize, usize>) -> Vec<TreeNode> {
+        let mut new_nodes: Vec<TreeNode> = vec![];
+        let source_nodes = source_tree.nodes.borrow();
+        let tree_nodes_it = id_map.iter().flat_map(|(old_id, new_id)| {
+            source_nodes.get(*old_id).map(|sn|
+                TreeNode {
+                    id: NodeId::new(*new_id),
+                    parent: sn
+                        .parent
+                        .and_then(|old_id| id_map.get(&old_id.value).map(|id| NodeId::new(*id))),
+                    prev_sibling: sn
+                        .prev_sibling
+                        .and_then(|old_id| id_map.get(&old_id.value).map(|id| NodeId::new(*id))),
+                    next_sibling: sn
+                        .next_sibling
+                        .and_then(|old_id| id_map.get(&old_id.value).map(|id| NodeId::new(*id))),
+                    first_child: sn
+                        .first_child
+                        .and_then(|old_id| id_map.get(&old_id.value).map(|id| NodeId::new(*id))),
+                    last_child: sn
+                        .last_child
+                        .and_then(|old_id| id_map.get(&old_id.value).map(|id| NodeId::new(*id))),
+                    data: sn.data.clone(),
+                }
+            )
+
+        });
+        new_nodes.extend(tree_nodes_it);
+        new_nodes.sort_by_key(|k| k.id.value);
+        new_nodes
+    }
+
+    /// Copies nodes from another tree to the current tree and applies the given function
+    /// to each copied node. The function is called with the ID of each copied node.
+    /// 
+    /// # Arguments
+    /// 
+    /// * `other_nodes` - slice of nodes to be copied
+    /// * `f` - function to be applied to each copied node
+    pub(crate) fn copy_nodes_with_fn<F>(&self, other_nodes: &[NodeRef], f: F)
+    where
+        F: Fn(NodeId),
+    {
+        for other_node in other_nodes {
+            let new_node_id = self.copy_node(other_node);
+            f(new_node_id);
+        }
+    }
 }

src/entities.rs

@@ -1,15 +1,17 @@
 #[cfg(feature = "hashbrown")]
 mod inline {
-    use hashbrown::HashSet;
+    use hashbrown::{HashMap, HashSet};
     pub type NodeIdSet = HashSet<crate::NodeId>;
     pub type HashSetFx<K> = HashSet<K>;
+    pub type InnerHashMap<K, V> = HashMap<K, V>;
 }
 
 #[cfg(not(feature = "hashbrown"))]
 mod inline {
-    use foldhash::HashSet;
+    use foldhash::{HashMap, HashSet};
     pub type NodeIdSet = HashSet<crate::NodeId>;
     pub type HashSetFx<K> = HashSet<K>;
+    pub type InnerHashMap<K, V> = HashMap<K, V>;
 }
 
-pub(crate) use inline::{HashSetFx, NodeIdSet};
+pub(crate) use inline::{HashSetFx, InnerHashMap, NodeIdSet};

src/selection.rs

@@ -358,7 +358,6 @@ impl<'a> Selection<'a> {
     ///
     /// A new `Selection` object containing the combined elements.
     pub fn add_selection(&self, other: &'a Selection) -> Selection<'a> {
-
         if self.is_empty() {
             return other.clone();
         }
@@ -435,17 +434,17 @@ impl<'a> Selection<'a> {
     /// This follows the same rules as `append`.
     ///
     pub fn replace_with_selection(&self, sel: &Selection) {
-        //! This is working solution, but it's not optimal yet!
         //! Note: goquery's behavior is taken as the basis.
+        if sel.is_empty() {
+            return;
+        }
 
-        let mut contents: StrTendril = StrTendril::new();
-        sel.iter().for_each(|s| contents.push_tendril(&s.html()));
-        let fragment = Document::from(contents);
         sel.remove();
 
+        let sel_nodes = sel.nodes();
         for node in self.nodes() {
-            node.tree.merge_with_fn(fragment.tree.clone(), |node_id| {
-                node.append_prev_siblings(&node_id)
+            node.tree.copy_nodes_with_fn(sel_nodes, |new_node_id| {
+                node.append_prev_sibling(&new_node_id)
             });
         }
 
@@ -455,18 +454,17 @@ impl<'a> Selection<'a> {
     /// Appends the elements in the selection to the end of each element
     /// in the set of matched elements.
     pub fn append_selection(&self, sel: &Selection) {
-        //! This is working solution, but it's not optimal yet!
         //! Note: goquery's behavior is taken as the basis.
 
-        let mut contents: StrTendril = StrTendril::new();
-        sel.iter().for_each(|s| contents.push_tendril(&s.html()));
-        let fragment = Document::from(contents);
-        sel.remove();
+        if sel.is_empty() {
+            return;
+        }
 
+        sel.remove();
+        let sel_nodes = sel.nodes();
         for node in self.nodes() {
-            node.tree.merge_with_fn(fragment.tree.clone(), |node_id| {
-                node.append_children(&node_id)
-            });
+            node.tree
+                .copy_nodes_with_fn(sel_nodes, |new_node_id| node.append_children(&new_node_id));
         }
     }
 

tests/selection-manipulation.rs

@@ -162,6 +162,7 @@ fn test_append_selection_multiple() {
     let sel_dst = doc.select(".ad-content p");
     let sel_src = doc.select("span.source");
 
+    // sel_src will be detached from it's tree
     sel_dst.append_selection(&sel_src);
     assert_eq!(doc.select(".ad-content .source").length(), 2);
     assert_eq!(doc.select(".ad-content span").length(), 4)
@@ -187,7 +188,7 @@ fn test_replace_with_another_tree_selection() {
 
 #[cfg_attr(not(target_arch = "wasm32"), test)]
 #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
-fn test_append_tree_selection() {
+fn test_append_another_tree_selection() {
     let doc_dst = Document::from(REPLACEMENT_SEL_CONTENTS);
 
     let contents_src = r#"
@@ -203,3 +204,45 @@ fn test_append_tree_selection() {
     assert_eq!(doc_dst.select(".ad-content .source").length(), 4);
     assert_eq!(doc_dst.select(".ad-content span").length(), 6)
 }
+
+#[cfg_attr(not(target_arch = "wasm32"), test)]
+#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+fn test_append_another_tree_selection_empty() {
+    let doc_dst = Document::from(REPLACEMENT_SEL_CONTENTS);
+
+    let contents_src = r#"
+    <span class="source">example</span>
+    <span class="source">example</span>"#;
+
+    let doc_src = Document::from(contents_src);
+
+    let sel_dst = doc_dst.select(".ad-content p");
+
+    // selecting non-existing elements
+    let sel_src = doc_src.select("span.src");
+    assert!(!sel_src.exists());
+
+    // sel_dst remained without changes
+    sel_dst.append_selection(&sel_src);
+    assert_eq!(doc_dst.select(".ad-content span").length(), 2)
+}
+
+#[cfg_attr(not(target_arch = "wasm32"), test)]
+#[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)]
+fn test_replace_with_another_tree_selection_empty() {
+    let doc_dst = Document::from(REPLACEMENT_SEL_CONTENTS);
+
+    let contents_src = r#"
+    <span class="source">example</span>
+    <span class="source">example</span>"#;
+
+    let doc_src = Document::from(contents_src);
+
+    let sel_dst = doc_dst.select(".ad-content p span");
+    // selecting non-existing elements
+    let sel_src = doc_src.select("span.src");
+    assert!(!sel_src.exists());
+    sel_dst.replace_with_selection(&sel_src);
+    // sel_dst remained without changes
+    assert_eq!(doc_dst.select(".ad-content span").length(), 2)
+}