7

I thought that during a permutation in an ObservableList, added and removed changes should not be generated. However, calling FXCollections.reverse() showed that this is not the case. This is my code:

public class Permutation {

    public static void main(String[] args) {
        test("FXCollections.reverse()", FXCollections::reverse);
        test("sort(reverseOrder())", list -> list.sort(Comparator.reverseOrder()));
    }

    private static void test(String name, Consumer<ObservableList<Integer>> op) {
        ObservableList<Integer> list = FXCollections.observableArrayList(1, 2, 3);
        list.addListener((ListChangeListener<Integer>) change -> {
            while (change.next())
                System.out.printf("%s - Added: %s, Removed: %s, Permutated: %s%n",
                    name, change.wasAdded(), change.wasRemoved(), change.wasPermutated());
        });
        op.accept(list);
    }
}

And this is the output:

FXCollections.reverse() - Added: true, Removed: true, Permutated: false
sort(reverseOrder()) - Added: false, Removed: false, Permutated: true

Could you please tell me if this is a bug or expected behavior?

2
  • 5
    The API spec doesn't state conditions under which a permutation change must be fired, so it's not technically a bug. I agree that I would intuitively expect this to fire a permutation change, not a "replaced" (added+removed) change. To be robust, you should test for both (handling the "permutated" case is probably the more efficient path). Commented Nov 17 at 19:18
  • 2
    I'm sure you are aware of this, but it may be worth pointing out explicitly that the functionality of these two method calls is not the same in general. (One orders the elements in the reverse of the current order, the other orders elements in the reverse of the natural order.) Commented Nov 18 at 16:58

2 Answers 2

6

Change Types

Note that when a ListChangeListener.Change reports elements were both added and removed, then that change is a "replacement" change. At least some elements that were removed were replaced by a new element (even if that new element was previously at a different position in the same list).

A replacement change can be handled specially by checking if Change::wasReplaced() returns true.


Expected Change Type

The documentation of FXCollections::reverse(ObservableList) says:

Reverses the order in the list. Fires only one change notification on the list.

The only stipulation is that one change event will be fired. It doesn't say what kind of change event will be fired. Though given the API of ListChangeListener.Change there's really only two options. The event will either be a replacement change or a permutation change. Either change type would be a valid implementation, and reverse happens to be implemented in such a way that you get a replacement change.

Why not a Permutation Change?

The implementation of reverse is arguably the consequence of making it a static method of FXCollections instead of an instance method of ObservableList.

The only way reverse has to interact with the ObservableList is via the API of ObservableList. Combine that with reverse guaranteeing only one change event will be fired plus ObservableList not providing a way to fire events from the "outside", and the choice of implementation becomes rather limited. You can't call methods of ObservableList that might unduly fire change events and you have no way to explicitly fire a permutation change event. In fact, you really only have one choice that behaves the same regardless of implementation, and that's how the method is currently implemented:

@SuppressWarnings("unchecked")
public static void reverse(ObservableList list) {
    Object[] newContent = list.toArray();
    for (int i = 0; i < newContent.length / 2; ++i) {
        Object tmp = newContent[i];
        newContent[i] = newContent[newContent.length - i - 1];
        newContent[newContent.length -i - 1] = tmp;
    }
    list.setAll(newContent);
}

The method extracts the elements into an array, reverses the array, then sets the list's elements to the array via setAll. That final step is where the replacement change comes from. Though note that seems to be relying on implementation details of ObservableList::setAll(Object...), whose documentation says:

Clears the ObservableList and adds all the elements passed as var-args.

There's no guarantee that setAll will only fire one change event. The standard implementations1 fire a single replacement change, but I believe it would be legal for an implementation to fire a separate "remove" and "add" change (clear the list, fire remove change, add the elements, fire add change)3. Maybe I'm wrong here.

Then Why Sorting Gives a Permutation Change?

The List::sort(Comparator) method is an instance method inherited by ObservableList. This means the implementation has more control over what kind of change event it fires. It also means the implementation has direct access to the underlying data structure, giving it more freedom to do things which won't cause undue change events to be fired.

That said, the standard implementations of ObservableList1 are simple wrappers around a List. So currently they still end up extracting the elements into an array. The array is sorted (via a merge sort) while keeping track of the index changes. Then the backing List has its elements set from the sorted array (setting the elements doesn't go through the ObservableList so no change events are fired). Finally, a permutation change is fired with the appropriate int[].

However, be aware that the ObservableList interface does not guarantee that calling the instance sort method will result in a permutation change event being fired, let alone that only one change event will be fired4. If you want to guarantee a single change event then use one of the two FXCollections::sort static methods. But there's no way to guarantee the change event will be a permutation change. Currently, for standard implementations of ObservableList, you'll get a permutation change event whether you sort via List::sort or via FXCollections::sort. For non-standard implementations you'll get a replacement change for the latter method2; it depends on the implementation for the former method.


1. The instances returned by the FXCollections::observable[Array]List static factory methods are the "standard implementations" of ObservableList.

2. For FXCollections::sort, the difference is whether or not the ObservableList is also an instance of the internal SortableList interface. Technically, a non-standard implementation could implement that interface. Though again, that doesn't guarantee anything about the type of change event fired. Additionally, if the list is not a SortableList then the implementation copies the list, sorts the copy, then sets the elements of the original list via setAll, similar to reverse. Thus, the "single replacement change" again seems dependent on implementation details of setAll.

3. I would find it very odd if an implementation fired more than one event from setAll, at least when the number of "added" elements equals the number of "removed" elements.

4. I struggle to imagine a legitimate scenario where sort would fire more than one event.

Sign up to request clarification or add additional context in comments.

Comments

5

According to the source code, it looks like the FXCollections.reverse() method actually takes items from the source array and inserts them into a new array:

    public static void reverse(ObservableList list) {
        Object[] newContent = list.toArray();
        for (int i = 0; i < newContent.length / 2; ++i) {
            Object tmp = newContent[i];
            newContent[i] = newContent[newContent.length - i - 1];
            newContent[newContent.length -i - 1] = tmp;
        }
        list.setAll(newContent);
    }

The setAll() method is clearing the list and readding elements, so I think the behavior you are experiencing is the intended design.

The method is distinct from the reverseOrder() comparator, which doesn't seem to actually add/remove items.

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.