Parent is still set in Node after removing it from the tree

A key original goal of NodeList was to provide GPath support which is typically read only. Adding nodes was originally envisaged to be done via appendNode() which correctly adjusts the parent. Given the greater usage of these classes for update scenarios, we should provide the correct semantics for add and removing from the nodelist.

I am currently thinking of just adding append(Node) and remove(Node) methods to Node. In many GPath scenarios NodeLists are used containing references to the Nodes from a hierarchy rather than clones of those nodes. In that scenario you may want to work with the GPath nodelists without affecting the original hierarchy.

Ok - I just thought that in principal the 'in place' modification features (esp. using subscript op) are super nice, like replacing an <include> node

int includeIndex = includeNode.parent().children().indexOf(includeNode) 
includeNode.parent().children()[includeIndex..includeIndex] = includeModel.children()

Regarding your concern about the resulting need for cloning nodes. Wouldn't it rather be a matter of cloning the list?

Say children() is a view (as in subList of List interface) on the tree as it is now and all other methods return 'copies' on which modifications don't affect the coresponding tree. So children could return something like ChildrenView extends NodeList which would know the parent node and override all modifier methods so that

• remove methods would set the node's parent to null
• add/set methods would set the node's parent to the respective parent after removing it from its former parent

Because of the latter it would be necessary to supply a clone(boolean deep) method for nodes (since you don't always want to remove a node from the source tree)

If one would like to work on the cildren() list without modifying the tree structure he could clone the list as in children().collect

.

End christmas wish list

I added in the basic append(Node) and remove(Node) capabilities as I think they are useful regardless of whatever else we consider to do as well. The changes were in changeset 10815. I meant to add them as a separate changeset but accidentally bundled them with another change. Details here:
http://svn.groovy.codehaus.org/changelog/groovy/trunk/groovy/groovy-core?cs=10815

I am going to mark this as fixed. The added remove behavior allows most use cases to be readily covered, e.g.:

def xml = '''
<foo>
    <bar id="1" />
    <bar id="2" />
    <bar id="3" />
    <bar id="4" />
    <bar id="5" />
    <bar id="6" />
    <bar id="7" />
</foo>'''
Node foo = new XmlParser().parseText(xml)

assert foo.bar.size() == 7

Node bar2 = foo.bar.find {it.@id == '2'}
bar2.parent().remove(bar2)

assert foo.bar.size() == 6
assert ! foo.bar.contains(bar2)
assert bar2.parent() == null

NodeList otherEvens = foo.bar.findAll {it.@id.toInteger() % 2 == 0}
otherEvens.each{ n ->
    n.parent().remove(n)
}

assert foo.bar.size() == 4

We could create a view only semantics layer as well but I think that deserves its own issue and discussion if we decide it is worth adding into core Groovy.