ComponentEntity.addComponent() doesn't copy the component

[MichaelClerx]

The docstring for ComponentEntity.addComponent(const ComponentPtr &c) says a copy of the component argument will be made

https://github.com/cellml/libcellml/blob/develop/src/api/libcellml/componententity.h#L51-L58

This doesn't seem to be the case:

    x = ComponentEntity()
    y = Component()
    z = Component()
    y.addComponent(z)
    self.assertTrue(y.containsComponent(z)) --> True!
    x.addComponent(y)
    x.removeComponent(z, True)
    self.assertTrue(y.containsComponent(z)) --> False!

[MichaelClerx]

There's other methods that claim to make copies. We should check these all and add tests for this.

[hsorby]

After having a discussion with @nickerso we think the following behaviour should be observed:

    parent = Component()
    child1 = Component()
    child2 = Component()
    child3 = Component()

    parent.addComponent(child1);
    parent.addComponent(child2);

    self.assertTrue(2 == parent.componentCount())
    self.assertTrue(0 == child1.componentCount())
    self.assertTrue(0 == child2.componentCount())

    # Expect child3 to be added no problem.
    self.assertTrue(child1.addComponent(child3))

    self.assertTrue(2 == parent.componentCount())
    self.assertTrue(1 == child1.componentCount())
    self.assertTrue(0 == child2.componentCount())

    # child3 already has a parent so cannot be added to
    # another component.
    self.assertFalse(child2.addComponent(child3)

    # Hierarchy of the model has not changed after attempting
    # to add child3 to child2.
    self.assertTrue(2 == parent.componentCount())
    self.assertTrue(1 == child1.componentCount())
    self.assertTrue(0 == child2.componentCount())

[agarny]

Hmm... doesn't the following go against what we have recently agreed on and implemented?

# child3 already has a parent so cannot be added to
# another component.
self.assertFalse(child2.addComponent(child3))

I would expect child2.addComponent(child3) to return true and therefore have:

self.assertTrue(2 == parent.componentCount())
self.assertTrue(0 == child1.componentCount())
self.assertTrue(1 == child2.componentCount())

[hsorby]

Yes it does go against what has just been implemented. On reflection I don't like the current implementation. To me it is doing a move but we are calling it an add.

I feel it is more straight forward to add components this way. We can then add convenience methods to help with moving components or multiple additions of say an imported component.

[agarny]

Hmm... fair points. Otherwise, yes, it might be nice to have convenience methods.