diff --git a/src/compute_MTG/delete_nodes.jl b/src/compute_MTG/delete_nodes.jl index d3afe0b..c4ac048 100644 --- a/src/compute_MTG/delete_nodes.jl +++ b/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) diff --git a/src/compute_MTG/node_funs.jl b/src/compute_MTG/node_funs.jl index 957d4c6..9a686cc 100644 --- a/src/compute_MTG/node_funs.jl +++ b/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 diff --git a/src/types/Node.jl b/src/types/Node.jl index e46ba56..b5fce9c 100644 --- a/src/types/Node.jl +++ b/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 diff --git a/test/test-descendants.jl b/test/test-descendants.jl index 35d9040..013b705 100644 --- a/test/test-descendants.jl +++ b/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) diff --git a/test/test-nodes.jl b/test/test-nodes.jl index 941abab..7b4bf42 100644 --- a/test/test-nodes.jl +++ b/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) diff --git a/test/test-read_mtg.jl b/test/test-read_mtg.jl index 6f61644..6ae1ee1 100644 --- a/test/test-read_mtg.jl +++ b/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