Incremental Update breaks Source Model Tracking

[NiklasRentzCAU]

When updating a diagram with the incremental update, the SourceModelTrackingAdapter of the ViewContext breaks, the map used in the ViewContext:getSourceElement method only contains a single node. Somewhere in the last updates to the incremental update this must have confused the automatic adapter propagation that the mapping is not restored correctly anymore.

[NiklasRentzCAU]

Digging though the code I noticed that this reverted commit is the main thing that differs and causes the SourceModelTrackingAdapter to be removed whenever the incremental update copies the properties and alters the model element.
@sailingKieler any idea why this commit was reverted in the first place? re-reverting that would fix this issue and I don't see any immedeate reason why setting a new model element should remove the adapter entirely.

[sailingKieler]

What exactly is the problem? Please outline the erroneous scenario.

I think it is wrong to not care about the former target of some single-value reference of an EObject while adding the new one to the maps. IMO this way you will end up with a memory leak.

However, one might need to distinguish containment and non-containment cross references at this point, which hasn't been done yet. That might be a mistake but I'm not sure.
Of course, I don't remember everything in detail, but I do remember that I spend lots of effort in fine-tuning the implementation. For example, note that EMaps are internally organized as lists of entries.

Have a look at https://github.com/kieler/KLighD/blob/master/test/de.cau.cs.kieler.klighd.test/src/de/cau/cs/kieler/klighd/test/ViewContextSourceModelTrackingTest.java before doing any changes, and add further test cases justifying changes, if necessary.

[NiklasRentzCAU]

I got a simple KGraph that is drawn by KLighD:

kgraph simple

knode a {
	knode i
}

At this point, the SourceModelTrackingAdapter contains mapping for all nodes shown to the source elements individually. Then I force an update using the IncremetalUpdateStrategy (for example by altering any diagram option). This causes the KGraphMerger of the incremental update to merge all properties from the new view model to the base view model, ultimately causing at SET notification for setting the model element of the new model to the base view model (which in this case is the just the same source element). That SET notification currently however removes the source model tracking adapter entirely from the shown base view model, removing also all mappings of the children.

You are right, just reverting again will cause test10 to fail. I got two different solutions however which I wish you to review first:

1. Replacing the line
   removeAdapter(notifier);
   with the line
   removeTracedElement(notifier);.
   This way test10 will still succeed and this problem will be fixed as well.

2. Adding a
   && oldValue != newValue
   in the check for the SET notification to remove only change the notifier if the new value set really is new. This will also let all test cases pass and fix this ticket.

Which one would you prefer @sailingKieler ?

[sailingKieler]

That SET notification currently however removes the source model tracking adapter entirely from the shown base view model, removing also all mappings of the children.

This would be indeed unfortunate.
I feel like a more comprehensive review is required, as some fundamentals have changed with the revised KGraph structure. In the past properties were set on some KLayoutData object that was attached to e.g. the root node, whereas in the meantime KNodes are KLayoutData.

I tend to advocate option two, although I'm wondering why is the KGraphMerger setting those properties repeatedly if the values are identical? And why is the Adapter notified on "no-change" changes?

[NiklasRentzCAU]

It uses the copyProperties of the IPropertyHolder interface to copy all properties from the new element to the base element and therefore uses the EMapPropertyHolder implementation, which does not check if the value has already been set. This is also why the adapter is notified although the change does not do anything. So an alternative would be to check for equality there already. If you think that is better, I will make that change on master or as a pull request, as you wish.

[sailingKieler]

So an alternative would be to check for equality there already. If you think that is better, I will make that change on master or as a pull request, as you wish.

That would be great! :-)

[NiklasRentzCAU]

I created a PR for you to review, if you think there is anything we still have to do before merging, please comment. Otherwise I see this ticket to be fixed now.

[sailingKieler]

Does the change really fix this issue?
I would really appreciate, if you could add some tests for the incremental update strategy.
Doesn't need to be done shortly, but should be done mid-term to avoid this kind of problems in future. If you need any help on that let me know.

[NiklasRentzCAU]

I will add tests at some point, the incremental update caused mutiple problems over the time for me and I would myself like to see that in a stable and tested state. Currently I just needed a quick fix for myself, testing will be done more mid-term.

[NiklasRentzCAU]

I added a test that tests the exact scenario I had before and succeeds only with this fix. More tests for the incremental will follow later, but this at least confirms this fix can be merged into master.