From bb326d10c653ea0fd5b900f300cfdfdeb1a24e06 Mon Sep 17 00:00:00 2001
From: Jordi <jordi.burguet.castell@gmail.com>
Date: Thu, 12 Oct 2023 10:04:38 +0200
Subject: [PATCH] Fix children losing track of a different parent on removal.

If we have already t_new.add_child(node) a node to a new tree, and
we remove it from its old one (t_old.remove_child(node) or t_old.pop_node(i)),
we do want the child to still remember it belongs to a different tree!
---
 ete4/coretype/tree.pyx | 14 +++++++---
 tests/test_tree.py     | 58 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+), 4 deletions(-)

diff --git a/ete4/coretype/tree.pyx b/ete4/coretype/tree.pyx
index 7dc4a16b3..685d19f19 100644
--- a/ete4/coretype/tree.pyx
+++ b/ete4/coretype/tree.pyx
@@ -393,8 +393,11 @@ cdef class Tree(object):
 
     def pop_child(self, child_idx=-1):
         try:
-            child = self.children.pop(child_idx)
-            child.up = None
+            child = self.children.pop(child_idx)  # parent removes child
+
+            if child.up == self:  # (it may point to another already!)
+                child.up = None  # child removes parent
+
             return child
         except ValueError as e:
             raise TreeError(f'Cannot pop child: not found ({e})')
@@ -406,8 +409,11 @@ cdef class Tree(object):
         but are no longer connected.
         """
         try:
-            self.children.remove(child)  # parent will not know about child
-            child.up = None  # child will not know about parent
+            self.children.remove(child)  # parent removes child
+
+            if child.up == self:  # (it may point to another already!)
+                child.up = None  # child removes parent
+
             return child
         except ValueError as e:
             raise TreeError(f'Cannot remove child: not found ({e})')
diff --git a/tests/test_tree.py b/tests/test_tree.py
index ba42a460f..b9dd6ad6f 100644
--- a/tests/test_tree.py
+++ b/tests/test_tree.py
@@ -479,6 +479,64 @@ def test_tree_manipulation(self):
         self.assertEqual(set([n.name for n in t1.traverse()]),
                          set(['A', 'B', 'C', 'I', 'root']))
 
+    def test_remove_child(self):
+        """Test removing children."""
+        t = Tree()
+        t.populate(20)
+
+        # Removed node loses its parent.
+        node1 = t[1]
+        self.assertEqual(node1.up, t)  # no surprise here
+
+        node1_returned = t.remove_child(t[1])
+        self.assertEqual(node1_returned, node1)
+        self.assertEqual(node1.up, None)  # node1 lost its parent
+
+        t.add_child(node1)  # ok, let's put it back
+
+        # A node that was already "stolen" does not lose its parent.
+        node0 = t[0]  # first child from t, node that we will be moving around
+
+        t2 = Tree()
+        t2.add_child(node0)  # "steals" the first child from t
+
+        self.assertEqual(t[0], t2[0])  # they should be the same node
+
+        t.remove_child(node0)
+        self.assertTrue(t[0] != node0)  # node is no longer a child of t
+
+        self.assertEqual(node0.up, t2)  # but has not lost track of its parent
+        self.assertEqual(t2[0], node0)
+
+    def test_pop_child(self):
+        """Test popping children."""
+        t = Tree()
+        t.populate(20)
+
+        # Removed node loses its parent.
+        node1 = t[1]
+        self.assertEqual(node1.up, t)  # no surprise here
+
+        node1_returned = t.pop_child()
+        self.assertEqual(node1_returned, node1)
+        self.assertEqual(node1.up, None)  # node1 lost its parent
+
+        t.add_child(node1)  # ok, let's put it back
+
+        # A node that was already "stolen" does not lose its parent.
+        node0 = t[0]  # first child from t, node that we will be moving around
+
+        t2 = Tree()
+        t2.add_child(node0)  # "steals" the first child from t
+
+        self.assertEqual(t[0], t2[0])  # they should be the same node
+
+        t.pop_child(0)
+        self.assertTrue(t[0] != node0)  # node is no longer a child of t
+
+        self.assertEqual(node0.up, t2)  # but has not lost track of its parent
+        self.assertEqual(t2[0], node0)
+
     def test_pruning(self):
         # test prune preserving distances
         for i in range(3):  # NOTE: each iteration is quite slow