Merge pull request #362 from teleological/fix_text_merge

Fix text merge
libxmljs · Dec 6, 2015 · 00405a8 · 00405a8
2 parents dd5e00c + 7eb33b9
commit 00405a8
Show file tree

Hide file tree

Showing 8 changed files with 187 additions and 34 deletions.
diff --git a/src/xml_element.cc b/src/xml_element.cc
@@ -101,15 +101,26 @@ NAN_METHOD(XmlElement::AddChild) {
   XmlElement* element = Nan::ObjectWrap::Unwrap<XmlElement>(info.Holder());
   assert(element);
 
-  XmlElement* child = Nan::ObjectWrap::Unwrap<XmlElement>(info[0]->ToObject());
+  XmlNode* child = Nan::ObjectWrap::Unwrap<XmlNode>(info[0]->ToObject());
   assert(child);
 
-  xmlNode *imported_child = element->import_element(child);
+  xmlNode *imported_child = element->import_node(child->xml_obj);
   if (imported_child == NULL) {
       return Nan::ThrowError("Could not add child. Failed to copy node to new Document.");
   }
+
+  bool will_merge = element->child_will_merge(imported_child);
+  if ((child->xml_obj == imported_child) && will_merge) {
+      // merged child will be free, so ensure it is a copy
+      imported_child = xmlCopyNode(imported_child, 0);
+  }
+
   element->add_child(imported_child);
 
+  if (!will_merge && (imported_child->_private != NULL)) {
+      static_cast<XmlNode*>(imported_child->_private)->ref_wrapped_ancestor();
+  }
+
   return info.GetReturnValue().Set(info.Holder());
 }
 
@@ -230,31 +241,49 @@ NAN_METHOD(XmlElement::AddPrevSibling) {
   XmlElement* element = Nan::ObjectWrap::Unwrap<XmlElement>(info.Holder());
   assert(element);
 
-  XmlElement* new_sibling = Nan::ObjectWrap::Unwrap<XmlElement>(info[0]->ToObject());
+  XmlNode* new_sibling = Nan::ObjectWrap::Unwrap<XmlNode>(info[0]->ToObject());
   assert(new_sibling);
 
-  xmlNode *imported_sibling = element->import_element(new_sibling);
+  xmlNode *imported_sibling = element->import_node(new_sibling->xml_obj);
   if (imported_sibling == NULL) {
       return Nan::ThrowError("Could not add sibling. Failed to copy node to new Document.");
   }
+
+  bool will_merge = element->prev_sibling_will_merge(imported_sibling);
+  if ((new_sibling->xml_obj == imported_sibling) && will_merge) {
+      imported_sibling = xmlCopyNode(imported_sibling, 0); // merged sibling is freed, so copy it
+  }
   element->add_prev_sibling(imported_sibling);
 
+  if (!will_merge && (imported_sibling->_private != NULL)) {
+      static_cast<XmlNode*>(imported_sibling->_private)->ref_wrapped_ancestor();
+  }
+
   return info.GetReturnValue().Set(info[0]);
 }
 
 NAN_METHOD(XmlElement::AddNextSibling) {
   XmlElement* element = Nan::ObjectWrap::Unwrap<XmlElement>(info.Holder());
   assert(element);
 
-  XmlElement* new_sibling = Nan::ObjectWrap::Unwrap<XmlElement>(info[0]->ToObject());
+  XmlNode* new_sibling = Nan::ObjectWrap::Unwrap<XmlNode>(info[0]->ToObject());
   assert(new_sibling);
 
-  xmlNode *imported_sibling = element->import_element(new_sibling);
+  xmlNode *imported_sibling = element->import_node(new_sibling->xml_obj);
   if (imported_sibling == NULL) {
       return Nan::ThrowError("Could not add sibling. Failed to copy node to new Document.");
   }
+
+  bool will_merge = element->next_sibling_will_merge(imported_sibling);
+  if ((new_sibling->xml_obj == imported_sibling) && will_merge) {
+      imported_sibling = xmlCopyNode(imported_sibling, 0);
+  }
   element->add_next_sibling(imported_sibling);
 
+  if (!will_merge && (imported_sibling->_private != NULL)) {
+      static_cast<XmlNode*>(imported_sibling->_private)->ref_wrapped_ancestor();
+  }
+
   return info.GetReturnValue().Set(info[0]);
 }
 
@@ -268,7 +297,7 @@ NAN_METHOD(XmlElement::Replace) {
     XmlElement* new_sibling = Nan::ObjectWrap::Unwrap<XmlElement>(info[0]->ToObject());
     assert(new_sibling);
 
-    xmlNode *imported_sibling = element->import_element(new_sibling);
+    xmlNode *imported_sibling = element->import_node(new_sibling->xml_obj);
     if (imported_sibling == NULL) {
         return Nan::ThrowError("Could not replace. Failed to copy node to new Document.");
     }
@@ -338,7 +367,6 @@ XmlElement::add_cdata(xmlNode* cdata) {
   xmlAddChild(xml_obj, cdata);
 }
 
-
 v8::Local<v8::Value>
 XmlElement::get_child(int32_t idx) {
   Nan::EscapableHandleScope scope;
@@ -483,6 +511,33 @@ XmlElement::replace_text(const char* content) {
   xmlReplaceNode(xml_obj, txt);
 }
 
+bool
+XmlElement::child_will_merge(xmlNode *child) {
+  return ((child->type == XML_TEXT_NODE)         &&
+          (xml_obj->last != NULL)                &&
+          (xml_obj->last->type == XML_TEXT_NODE) &&
+          (xml_obj->last->name == child->name)   &&
+          (xml_obj->last != child));
+}
+
+bool
+XmlElement::next_sibling_will_merge(xmlNode *child) {
+  return ((child->type == XML_TEXT_NODE) &&
+          ((xml_obj->type == XML_TEXT_NODE) ||
+           ((xml_obj->next != NULL) &&
+            (xml_obj->next->type == XML_TEXT_NODE) &&
+            (xml_obj->name == xml_obj->next->name)))); // libxml2 bug?
+}
+
+bool
+XmlElement::prev_sibling_will_merge(xmlNode *child) {
+  return ((child->type == XML_TEXT_NODE) &&
+          ((xml_obj->type == XML_TEXT_NODE) ||
+           ((xml_obj->prev != NULL) &&
+            (xml_obj->prev->type == XML_TEXT_NODE) &&
+            (xml_obj->name == xml_obj->prev->name))));
+}
+
 void
 XmlElement::Initialize(v8::Handle<v8::Object> target)
 {

diff --git a/src/xml_element.h b/src/xml_element.h
@@ -54,6 +54,9 @@ class XmlElement : public XmlNode {
     v8::Local<v8::Value> get_prev_element();
     void replace_element(xmlNode* element);
     void replace_text(const char* content);
+    bool child_will_merge(xmlNode* child);
+    bool prev_sibling_will_merge(xmlNode* node);
+    bool next_sibling_will_merge(xmlNode* node);
 };
 
 }  // namespace libxmljs

diff --git a/src/xml_node.cc b/src/xml_node.cc
@@ -463,7 +463,7 @@ XmlNode::get_wrapped_ancestor() {
 
 void
 XmlNode::ref_wrapped_ancestor() {
-    xmlNode* ancestor = get_wrapped_ancestor();
+    xmlNode* ancestor = this->get_wrapped_ancestor();
 
     // if our closest wrapped ancestor has changed then we either
     // got removed, added, or a closer ancestor was wrapped
@@ -631,37 +631,27 @@ XmlNode::remove() {
 void
 XmlNode::add_child(xmlNode* child) {
   xmlAddChild(xml_obj, child);
-  // if the child is wrapped then we have to ref its parents
-  if (child->_private != NULL) {
-      XmlNode* node = static_cast<XmlNode*>(child->_private);
-      node->ref_wrapped_ancestor();
-  }
 }
 
 void
 XmlNode::add_prev_sibling(xmlNode* node) {
   xmlAddPrevSibling(xml_obj, node);
-  if (node->_private != NULL)
-    static_cast<XmlNode*>(node->_private)->ref_wrapped_ancestor();
 }
 
 void
 XmlNode::add_next_sibling(xmlNode* node) {
   xmlAddNextSibling(xml_obj, node);
-  if (node->_private != NULL)
-    static_cast<XmlNode*>(node->_private)->ref_wrapped_ancestor();
 }
 
 xmlNode*
-XmlNode::import_element(XmlNode *element) {
-  if (xml_obj->doc == element->xml_obj->doc) {
-      // remove the child from its parent
-      // (alternatively, we could clone and keep the original in place)
-      if (element->xml_obj->parent != NULL)
-        element->remove();
-      return element->xml_obj;
+XmlNode::import_node(xmlNode *node) {
+  if (xml_obj->doc == node->doc) {
+      if ((node->parent != NULL) && (node->_private != NULL)) {
+        static_cast<XmlNode*>(node->_private)->remove();
+      }
+      return node;
   }else{
-      return xmlDocCopyNode(element->xml_obj, xml_obj->doc, 1);
+      return xmlDocCopyNode(node, xml_obj->doc, 1);
   }
 }
 

diff --git a/src/xml_node.h b/src/xml_node.h
@@ -68,7 +68,7 @@ class XmlNode : public Nan::ObjectWrap {
     void add_next_sibling(xmlNode* element);
     void replace_element(xmlNode* element);
     void replace_text(const char* content);
-    xmlNode *import_element(XmlNode* element);
+    xmlNode *import_node(xmlNode* node);
 };
 
 }  // namespace libxmljs

diff --git a/src/xml_text.cc b/src/xml_text.cc
@@ -82,6 +82,44 @@ NAN_METHOD(XmlText::Text) {
   return info.GetReturnValue().Set(info.Holder());
 }
 
+NAN_METHOD(XmlText::AddPrevSibling) {
+  XmlText* text = Nan::ObjectWrap::Unwrap<XmlText>(info.Holder());
+  assert(text);
+
+  XmlNode* new_sibling = Nan::ObjectWrap::Unwrap<XmlNode>(info[0]->ToObject());
+  assert(new_sibling);
+
+  xmlNode *imported_sibling = text->import_node(new_sibling->xml_obj);
+  if (imported_sibling == NULL) {
+      return Nan::ThrowError("Could not add sibling. Failed to copy node to new Document.");
+  }
+  else if ((new_sibling->xml_obj == imported_sibling) && text->prev_sibling_will_merge(imported_sibling)) {
+      imported_sibling = xmlCopyNode(imported_sibling, 0);
+  }
+  text->add_prev_sibling(imported_sibling);
+
+  return info.GetReturnValue().Set(info[0]);
+}
+
+NAN_METHOD(XmlText::AddNextSibling) {
+  XmlText* text = Nan::ObjectWrap::Unwrap<XmlText>(info.Holder());
+  assert(text);
+
+  XmlNode* new_sibling = Nan::ObjectWrap::Unwrap<XmlNode>(info[0]->ToObject());
+  assert(new_sibling);
+
+  xmlNode *imported_sibling = text->import_node(new_sibling->xml_obj);
+  if (imported_sibling == NULL) {
+      return Nan::ThrowError("Could not add sibling. Failed to copy node to new Document.");
+  }
+  else if ((new_sibling->xml_obj == imported_sibling) && text->next_sibling_will_merge(imported_sibling)) {
+      imported_sibling = xmlCopyNode(imported_sibling, 0);
+  }
+  text->add_next_sibling(imported_sibling);
+
+  return info.GetReturnValue().Set(info[0]);
+}
+
 NAN_METHOD(XmlText::Replace) {
   XmlText* element = Nan::ObjectWrap::Unwrap<XmlText>(info.Holder());
   assert(element);
@@ -92,7 +130,7 @@ NAN_METHOD(XmlText::Replace) {
     XmlText* new_sibling = Nan::ObjectWrap::Unwrap<XmlText>(info[0]->ToObject());
     assert(new_sibling);
 
-    xmlNode *imported_sibling = element->import_element(new_sibling);
+    xmlNode *imported_sibling = element->import_node(new_sibling->xml_obj);
     if (imported_sibling == NULL) {
         return Nan::ThrowError("Could not replace. Failed to copy node to new Document.");
     }
@@ -200,10 +238,14 @@ XmlText::replace_text(const char* content) {
   xmlReplaceNode(xml_obj, txt);
 }
 
-xmlNode*
-XmlText::import_element(XmlText *element) {
-  return (xml_obj->doc == element->xml_obj->doc) ?
-        element->xml_obj : xmlDocCopyNode(element->xml_obj, xml_obj->doc, 1);
+bool
+XmlText::next_sibling_will_merge(xmlNode *child) {
+  return (child->type == XML_TEXT_NODE);
+}
+
+bool
+XmlText::prev_sibling_will_merge(xmlNode *child) {
+  return (child->type == XML_TEXT_NODE);
 }
 
 void

diff --git a/src/xml_text.h b/src/xml_text.h
@@ -36,9 +36,11 @@ class XmlText : public XmlNode {
     void set_content(const char* content);
     void replace_text(const char* content);
     void replace_element(xmlNode* element);
-    xmlNode *import_element(XmlText* element);
     void add_prev_sibling(xmlNode* element);
     void add_next_sibling(xmlNode* element);
+    bool prev_sibling_will_merge(xmlNode* node);
+    bool next_sibling_will_merge(xmlNode* node);
+
 };
 
 }  // namespace libxmljs

diff --git a/test/element.js b/test/element.js
@@ -205,4 +205,24 @@ module.exports.replace = function(assert) {
   assert.equal(doc.root().childNodes()[1].name(), 'enchanted');
 
   assert.done();
-}
+};
+
+module.exports.add_child_merge_text = function(assert) {
+  var str = "<foo>bar</foo>";
+  var doc = libxml.parseXml(str);
+  var foo = doc.root();
+  var baz = new libxml.Text(doc, "baz");
+  foo.addChild(baz);
+
+  // added text is merged into existing child node
+  assert.strictEqual("barbaz", foo.text());
+  assert.strictEqual(foo.childNodes().length, 1);
+  assert.ok(foo.childNodes()[0] != baz);
+
+  // passed node is not changed
+  assert.strictEqual(doc, baz.parent());
+  assert.strictEqual("baz", baz.text());
+
+  assert.done();
+};
+
diff --git a/test/text.js b/test/text.js
@@ -92,3 +92,44 @@ module.exports.addSiblings = function(assert) {
 
     assert.done();
 };
+
+module.exports.add_prev_sibling_merge_text = function(assert) {
+  var str = '<foo>bar<baz/></foo>';
+  var doc = libxml.parseXml(str);
+  var bar = doc.root().childNodes()[0];
+
+  var qux = new libxml.Text(doc, 'qux');
+  bar.addPrevSibling(qux);
+  // added text is merged into existing child node
+
+  var children = doc.root().childNodes();
+  assert.strictEqual(children.length, 2);
+  assert.strictEqual('quxbar', children[0].text());
+  assert.ok(children[0] != qux);
+
+  // passed node is not changed
+  assert.strictEqual(doc, qux.parent());
+  assert.strictEqual('qux', qux.text());
+
+  assert.done();
+};
+
+module.exports.add_next_sibling_merge_text = function(assert) {
+  var str = '<foo>bar<baz/></foo>';
+  var doc = libxml.parseXml(str);
+  var bar = doc.root().childNodes()[0];
+
+  var qux = new libxml.Text(doc, 'qux');
+  bar.addNextSibling(qux);
+
+  var children = doc.root().childNodes();
+  assert.strictEqual(children.length, 2);
+  assert.strictEqual('barqux', children[0].text());
+  assert.ok(children[0] != qux);
+
+  assert.strictEqual(doc, qux.parent());
+  assert.strictEqual('qux', qux.text());
+
+  assert.done();
+};
+