Bug 561691

Description Gerhard Kreuzer 2020-04-02 08:23:50 EDT

Preface: I discovered this deficiency in Zest, but the cause is rooted in MVC. In Zest the Graph model consists of Nodes and Edges. When fetching the list of children (parts) of a parent (part), these two kinds of children are concatenated into the resulting list starting with the Node parts followed by the Edge parts (observation, not verifed in code).

Adding content children to a part (in MVC),  is finally controlled by the ContentBehavior [org.eclipse.gef.mvc.fx.behaviors.ContentBehavior]. There (in addAll()) the chilren of the given parent are cached in the list called "childContentParts" (code fragment only reformatted, original lines 169 to 177):

@SuppressWarnings("unchecked")
private void addAll(IVisualPart<? extends Node> parent,
                    List<? extends Object> contentChildren,
                    List<IContentPart<? extends Node>> added,
                    LinkedHashMap<IVisualPart<? extends Node>, 
                    HashMultimap<Integer, IContentPart<? extends Node>>> addsPerParent,
                    List<ReorderData> reorders) {
   List<IContentPart<? extends Node>> childContentParts = 
             PartUtils.filterParts(parent.getChildrenUnmodifiable(), IContentPart.class);

 Further, if addAll() detects, that a certain content part is at an incorrect location in this list, a reordering is initiated (=> AbstractVisualPart.reorderChild()).

Now, when a collection of Node parts is added, after the first addition for all existing Edges a reorder request is detected (and executed) because:

a) Edge parts are behind Node parts in the list of children

b) the list of children to check the order is cached and does not know about prior additions

This leads to UI freezes up to many seconds depending on the total number of child parts.

To avoid this, I see two possibilities:

1) do not cache the list of children, but operate on a fresh one after each addition (ContentBehavior.addAll()), or

2) execute the reordering only if oldIndex != index in reorderChild() (again, only reformatted, original lines 746 to 757):

@Override
public void reorderChild(IVisualPart<? extends Node> child, int index) {
  int oldIndex = getChildrenUnmodifiable().indexOf(child);
  if (oldIndex < 0) {
    throw new IllegalArgumentException("Cannot reorder child " + child + " because it is no child.");
  }
  // TODO: this could be made more performant (reordering the children and
  // visuals)
  removeChild(child);
  addChild(child, index);
}

I don't know which solutions is more performant, but I could imagine that 2) is a fertile option, because "oldIndex" is computed anyway.