java - Shuffling ObservableList fires incorrect change notification

Question

Method FXCollections.shuffle() fires only wasRemoved change notification. As we may know, shuffling isn't only about removing, but removing and adding.

In the documentation we can see:

Shuffles all elements in the observable list. Fires only one change notification on the list.

If I'm not mistaken, a single change can contain both wasAdded and wasRemoved. What a shame wasPermutated isn't being fired with the default ObservableList FX api (or is it?).

Code to test it out:

public class SimpleMain {

    public static void main(String[] args) {
        ObservableList<Integer> integers = FXCollections.observableArrayList(1, 2, 3, 4);
        integers.addListener(initListener());
        FXCollections.shuffle(integers);
    }

    private static ListChangeListener<Integer> initListener() {
        return change -> {
            while (change.next()) {
                if (change.wasPermutated()) {
                    System.out.println("wasPermutated");
                } else if (change.wasRemoved()) {
                    System.out.println("wasRemoved");
                } else if (change.wasAdded()) {
                    System.out.println("wasAdded");
                }
            }
        };
    }

}

Answer 1

Problem

Your code assumes that wasAdded() and wasRemoved() are mutually exclusive, whether you intended that or not. That assumption is wrong. If one or more contiguous elements are replaced then both those methods will return true.

Keep in mind there's a difference between a Change object and a "change". A single Change instance can carry multiple changes. When the documentation says:

Fires only one change notification on the list.

It is not saying only one Change object will be sent to the ListChangeListener. What it's saying is that the Change object will only carry one change. In other words, the Change#next() method will only return true for the first invocation and thus your while loop will only loop once.

---

Solution

You need to rewrite your code with the knowledge that both wasAdded() and wasRemoved() can both be true. For instance, here's a listener that checks for all types of changes:

private static ListChangeListener<Integer> initListener() {
  return change -> {
    while (change.next()) {
      if (change.wasPermutated()) {
        System.out.println("wasPermutated");
      } else if (change.wasUpdated()) {
        System.out.println("wasUpdated");
      } else if (change.wasReplaced()) {
        System.out.println("wasReplaced");
      } else if (change.wasRemoved()) {
        System.out.println("wasRemoved");
      } else { // only other change type is "added"
        System.out.println("wasAdded");
      }
    }
  };
}

The above uses wasReplaced() which is the same as wasAdded() && wasRemoved(). Note that a check for wasReplaced() must happen before either wasRemoved() or wasAdded() if you use an if-else-if structure. Otherwise the above will suffer the same problem your code has.

If you insert the above into your code and run it you'll see the following output:

wasReplaced

The documentation of ListChangeListener.Change gives a more general explanation (and example) of how to implement a ListChangeListener.

Note the example in the documentation does not check for wasReplaced() specifically. Instead, it processes both removed and added elements in a final else block (after wasPermutated() and wasUpdated() return false). This is possible because getRemoved() and getAddedSubList() will return empty lists if no element was removed or added, respectively. And it's typically the same effect if you process any removed elements and then any added elements as it is if you specially handle replacements. Depending on your use case, however, it may be beneficial to specially handle replacements.

---

Why not Permutation?

The way they've implemented the shuffle method is:

public static void shuffle(ObservableList list, Random rnd) {
    Object newContent[] = list.toArray();

    for (int i = list.size(); i > 1; i--) {
        swap(newContent, i - 1, rnd.nextInt(i));
    }

    list.setAll(newContent); 
}

^{Source: javafx.collections.FXCollections, JavaFX 15.}

As you can see, the elements are extracted out into an array, the array is shuffled, and then the elements in the list are replaced with the array. That results in a single "replacement change" being fired.