search

vitruv-tools / Vitruv

View-based Development and Model Consistency Framework
http://vitruv.tools
Eclipse Public License 1.0
14 stars 19 forks source link

Record Move from Removed Element Containment #469

Closed HeikoKlare closed 3 years ago

HeikoKlare commented 3 years ago

The ChangeRecorder removes the recording adapter from elements as soon as they are removed from a containment. If, however, elements contained within the removed elements (or any of its transitively contained elements) are then moved to another container, only the insertion change is recorded without recording the change for removing the element from the previous container. In consequence, applying the changes backwards for assigning appropriate IDs fails, because the elements is not inserted into its original container. This was not recognized before as we have no situation in which we re-insert an element contained in the tree that was removed from a model containment. It first came up when re-enabling layout information in Java in #468.

This PR adapts the ChangeRecorder such that it defers the removal from recording to after the current transactions, such that further changes in the removed elements are still recorded. As soon as the transaction ends, elements will not be inserted again (the same assumption it made for identifying delete changes) and then the recording adapter is removed. It also adds according regression tests.

JanWittler commented 3 years ago

I can image that this also caused #425 and thus will resolve it.

HeikoKlare commented 3 years ago

Unfortunately, I think that this will not resolve #425. The changes of this PR only affect very rare cases. Only if an element that is contained in a removed one is reinserted somewhere else, there will be a difference after merging this PR. Related to the example in #425, it would only make a difference if you reinserted a parameter from a removed operation into another operation afterwards. And even then, it does not affect correspondences but only fixes a problem that was not present when we used UUIDs but came up when switching to the new ID mechanism.

But thanks to you bringing up the issue, I thought about a solution for it and wrote a proposal in #425. Maybe it's quite easy to solve, even if not implicitly resolved by this PR :-)