Merge pull request #100 from VEZY/reparent-and-rechildren-update-parent-and-children

Fix issue #95

Author: VEZY

src/compute_MTG/delete_nodes.jl

@@ -143,7 +143,7 @@ function delete_node!(node::Node{N,A}; child_link_fun=new_child_link) where {N<:
 
         if !isleaf(node)
             # We re-parent the children to the parent of the node.
-            for chnode in children(node)
+            for chnode in copy(children(node))
                 # Updating the link of the children:
                 link!(chnode, child_link_fun(chnode))
                 addchild!(parent_node, chnode; force=true)

src/compute_MTG/node_funs.jl

@@ -129,13 +129,11 @@ end
 function addchild!(p::Node{N,A}, child::Node; force=false) where {N<:AbstractNodeMTG,A}
     child_was_root = parent(child) === nothing
 
-    if child_was_root || force == true
-        reparent!(child, p)
-    elseif parent(child) != p && force == false
+    if !child_was_root && parent(child) !== p && force == false
         error("The node already has a parent. Hint: use `force=true` if needed.")
     end
 
-    push!(children(p), child)
+    reparent!(child, p)
     _maybe_recolumnarize_after_attach!(p, child, child_was_root)
 
     return child

src/types/Node.jl

@@ -175,25 +175,75 @@ AbstractTrees.parent(node::Node{T,A}) where {T,A} = Base.parent(node)
 AbstractTrees.childrentype(node::Node{T,A}) where {T,A} = Vector{Node{T,A}}
 AbstractTrees.childtype(::Type{Node{T,A}}) where {T,A} = Node{T,A}
 
+@inline function _child_index_by_identity(chnodes, child)
+    @inbounds for i in eachindex(chnodes)
+        chnodes[i] === child && return i
+    end
+    return nothing
+end
+
+function _detach_child!(p::Node, child::Node)
+    chnodes = children(p)
+    removed = false
+    for i in reverse(eachindex(chnodes))
+        if chnodes[i] === child
+            deleteat!(chnodes, i)
+            removed = true
+        end
+    end
+    removed && _mark_structure_mutation!(p)
+    return removed
+end
+
+function _attach_child!(p::Node, child::Node)
+    chnodes = children(p)
+    _child_index_by_identity(chnodes, child) !== nothing && return false
+    push!(chnodes, child)
+    _mark_structure_mutation!(p)
+    return true
+end
+
 """
     reparent!(node::N, p::N) where N<:Node{T,A}
 
-Set the parent of the node.
+Set the parent of the node, removing it from the old parent's children and adding it
+to the new parent's children.
 """
 function reparent!(node::N, p::N2) where {N<:Node{T,A},N2<:Union{Nothing,Node{T,A}}} where {T,A}
+    p === node && error("A node cannot be its own parent.")
+
+    old_parent = parent(node)
+    changed = old_parent !== p
+    if old_parent !== nothing && changed
+        _detach_child!(old_parent, node)
+    end
+
     setfield!(node, :parent, p)
-    _mark_structure_mutation!(node)
-    p === nothing || _mark_structure_mutation!(p)
+    attached = p === nothing ? false : _attach_child!(p, node)
+    (changed || attached) && _mark_structure_mutation!(node)
     return p
 end
 
 """
     rechildren!(node::Node{T,A}, chnodes::Vector{Node{T,A}}) where {T,A}
 
-Set the children of the node.
+Set the children of the node, detaching removed children and setting this node as the
+parent of the new children.
 """
 function rechildren!(node::Node{T,A}, chnodes::Vector{Node{T,A}}) where {T,A}
+    old_children = children(node)
     setfield!(node, :children, chnodes)
+
+    for old_child in old_children
+        if parent(old_child) === node && _child_index_by_identity(chnodes, old_child) === nothing
+            reparent!(old_child, nothing)
+        end
+    end
+
+    for child in chnodes
+        reparent!(child, node)
+    end
+
     _mark_structure_mutation!(node)
     return chnodes
 end

test/test-descendants.jl

@@ -72,7 +72,6 @@ end
       mtg_b = read_mtg("files/simple_plant.mtg")
       # Bypass addchild! on purpose to build an incoherent tree (mixed stores).
       reparent!(mtg_b, mtg_a)
-      push!(children(mtg_a), mtg_b)
 
       err = try
             descendants(mtg_a, :Width)

test/test-nodes.jl

@@ -85,6 +85,40 @@ end
     @test children(mtg) == [internode]
 end
 
+@testset "parent and children setters stay synchronized" begin
+    mtg = MultiScaleTreeGraph.Node(MultiScaleTreeGraph.NodeMTG(:/, :Plant, 1, 1))
+    first_parent = MultiScaleTreeGraph.Node(mtg, MultiScaleTreeGraph.NodeMTG(:/, :Axis, 1, 2))
+    second_parent = MultiScaleTreeGraph.Node(mtg, MultiScaleTreeGraph.NodeMTG(:+, :Axis, 2, 2))
+    child = MultiScaleTreeGraph.Node(first_parent, MultiScaleTreeGraph.NodeMTG(:/, :Internode, 1, 3))
+    # Simulate a stale duplicate child entry from direct children vector mutation.
+    push!(children(first_parent), child)
+
+    reparent!(child, second_parent)
+    @test parent(child) === second_parent
+    @test !any(n -> n === child, children(first_parent))
+    @test count(n -> n === child, children(second_parent)) == 1
+
+    reparent!(child, second_parent)
+    @test count(n -> n === child, children(second_parent)) == 1
+
+    reparent!(child, nothing)
+    @test parent(child) === nothing
+    @test !any(n -> n === child, children(second_parent))
+
+    rechildren!(first_parent, typeof(children(first_parent))([child]))
+    @test parent(child) === first_parent
+    @test children(first_parent) == [child]
+
+    rechildren!(second_parent, typeof(children(second_parent))([child]))
+    @test parent(child) === second_parent
+    @test !any(n -> n === child, children(first_parent))
+    @test children(second_parent) == [child]
+
+    rechildren!(second_parent, typeof(children(second_parent))())
+    @test parent(child) === nothing
+    @test isempty(children(second_parent))
+end
+
 # From a file:
 file = "files/simple_plant.mtg"
 mtg = read_mtg(file)

test/test-read_mtg.jl

@@ -35,8 +35,10 @@ end
 @testset "test mtg mutation" begin
     @test (mtg[:scales] .= [0, 1, 2, 3, 4]) == [0, 1, 2, 3, 4]
     @test MultiScaleTreeGraph.node_mtg!(mtg, MultiScaleTreeGraph.NodeMTG(:<, :Leaf, 2, 0)) == MultiScaleTreeGraph.NodeMTG(:<, :Leaf, 2, 0)
-    reparent!(mtg[1], nothing)
-    @test parent(mtg[1]) === nothing
+    child = mtg[1]
+    reparent!(child, nothing)
+    @test parent(child) === nothing
+    @test isempty(children(mtg))
 end
 
 @testset "test mtg with empty lines" begin