Problem in ContentBehavior with an suboptimal datastructure

[BenJanus]

Hello GEF-Team,

we encountered a problem in the class org.eclipse.gef.mvc.fx.behaviors.ContentBehavior. We are on version 5.2.0.
The exception:
java.lang.IndexOutOfBoundsException: Index: 32, Size: 28 at java.base/java.util.ArrayList.rangeCheckForAdd(ArrayList.java:787) at java.base/java.util.ArrayList.addAll(ArrayList.java:731) at com.google.common.collect.ForwardingList.addAll(ForwardingList.java:71) at org.eclipse.gef.common.collections.ObservableListWrapperEx.addAll(ObservableListWrapperEx.java:122) at org.eclipse.gef.mvc.fx.parts.AbstractVisualPart.addChildren(AbstractVisualPart.java:200) at org.eclipse.gef.mvc.fx.behaviors.ContentBehavior.lambda$1(ContentBehavior.java:501) at com.google.common.collect.Maps$KeySet.lambda$forEach$0(Maps.java:3705) at java.base/java.util.HashMap.forEach(HashMap.java:1336) at com.google.common.collect.Maps$KeySet.forEach(Maps.java:3705) at org.eclipse.gef.mvc.fx.behaviors.ContentBehavior.synchronizeContentPartChildren(ContentBehavior.java:498) at org.eclipse.gef.mvc.fx.behaviors.ContentBehavior$2.onChanged(ContentBehavior.java:123) at org.eclipse.gef.common.collections.ListListenerHelperEx.notifyListChangeListeners(ListListenerHelperEx.java:650) at org.eclipse.gef.common.beans.binding.ListExpressionHelperEx.fireValueChangedEvent(ListExpressionHelperEx.java:109) at org.eclipse.gef.common.beans.property.ReadOnlyListWrapperEx$ReadOnlyPropertyImpl.fireValueChangedEvent(ReadOnlyListWrapperEx.java:91) at org.eclipse.gef.common.beans.property.ReadOnlyListWrapperEx$ReadOnlyPropertyImpl.access$2(ReadOnlyListWrapperEx.java:87) at org.eclipse.gef.common.beans.property.ReadOnlyListWrapperEx.fireValueChangedEvent(ReadOnlyListWrapperEx.java:234) at javafx.base/javafx.beans.property.ListPropertyBase.lambda$new$0(ListPropertyBase.java:57) at javafx.base/com.sun.javafx.collections.ListListenerHelper$SingleChange.fireValueChangedEvent(ListListenerHelper.java:164) at javafx.base/com.sun.javafx.collections.ListListenerHelper.fireValueChangedEvent(ListListenerHelper.java:73) at javafx.base/javafx.collections.ObservableListBase.fireChange(ObservableListBase.java:233) at javafx.base/javafx.collections.FXCollections$UnmodifiableObservableListImpl.lambda$new$0(FXCollections.java:955) at javafx.base/javafx.collections.WeakListChangeListener.onChanged(WeakListChangeListener.java:88) at org.eclipse.gef.common.collections.ListListenerHelperEx.notifyListChangeListeners(ListListenerHelperEx.java:650) at org.eclipse.gef.common.collections.ListListenerHelperEx.fireValueChangedEvent(ListListenerHelperEx.java:600) at org.eclipse.gef.common.collections.ObservableListWrapperEx.setAll(ObservableListWrapperEx.java:355) at org.eclipse.gef.mvc.fx.parts.AbstractContentPart.refreshContentChildren(AbstractContentPart.java:424) at com.initka.nui.linedisplay.parts.LineGefPart.refreshContentChildren(LineGefPart.java:128) at com.initka.nui.linedisplay.parts.AbstractGefContentPart.lambda$0(AbstractGefContentPart.java:48)

The root of the problem is located here: ContentBehavior.addAll(...)
For the local variable childrenToAdd a HashMultimap is used.
This is not a good choice because in ContentBehavior.synchronizeContentPartChildren(...) you rely on the correct sequence of the keys of the map. (You expect to get the keys in the sequence of their introduction into the map.) But this is unfortunately not the case. And because you use IVisualPart.addChildren(List children, int index) the mentioned exception occurs.
If you see this snippet:

void testMap() {
    HashMultimap<Integer, String> childrenToAdd = HashMultimap.create();
    childrenToAdd.put(30, "one");
    childrenToAdd.put(31, "two");
    childrenToAdd.put(32, "three");
    System.out.println(childrenToAdd);  // => {32=[three], 30=[one], 31=[two]}
  }

It would be nice if you could fix this problem. For us it is quite a problem and we see no way to workaround this.

[miklossy]

Thanks for providing a bug report @BenJanus ! Do you have a proper fix in your mind?

[nyssen]

I took a look at ContentBehavior. Usage of HashMultimap does not seem to be a problem, as the multi map provides the children via "index" keys. The problematic part is that the childContentParts are then traversed via the keyset instead of the correct "index":

childContentParts.keySet().forEach(cp -> {
				ArrayList<IContentPart<? extends Node>> children = Lists
						.newArrayList(childContentParts.get(cp));