ReactFX icon indicating copy to clipboard operation
ReactFX copied to clipboard

Add class ObservableSortedArraySet.

Open argv-minus-one opened this issue 8 years ago • 4 comments

This PR adds a class ObservableSortedArraySet, an observable set that is sorted and has an observable list view.

Note that it adds a dependency on Guava, which is used to implement some SortedSet methods. Not sure if that's acceptable or not.

argv-minus-one avatar Sep 17 '15 22:09 argv-minus-one

Thanks for the PR.

My main concern is that the implementation is not safe for recursive changes, i.e. changes made from within a change listener. I realize that observable collections shipped with JavaFX do not support recursive changes either. I find it very unfortunate, though, because it defies local reasoning about the code—when you want to make changes to a collection, you have to be aware whether the code is already executing from within a change listener.

ReactFX's LiveList allows recursive changes.

I would prefer not adding a dependency on Guava, but that's not the biggest issue now (especially when there are only a few uses of it that could be replaced easily).

TomasMikula avatar Sep 18 '15 18:09 TomasMikula

I've added some tests of recursive changes, and changed ObservableSortedArraySet to be based on LiveList. Recursive changes seem to work correctly now. (I tried using FXCollections.observableArrayList, but you were right—recursive changes did not fire an event like they should have.)

I've also pushed a couple of bug fixes (with their own branches), which my changes depend on. The change with ListIteratorImpl is needed because LiveList::sort doesn't work without a mutable ListIterator.

This leaves the Guava issue. Do you want me to change the subset methods to not depend on it?

argv-minus-one avatar Sep 19 '15 02:09 argv-minus-one

Do you want me to change the subset methods to not depend on it?

Adding a dependency to Guava seems an overkill here. Seems to be very easy to replace com.google.common.collect.Sets.filter() with a pure Java equivalent or maybe some stuff Stream provides.

hastebrot avatar Sep 24 '15 04:09 hastebrot

Here is a test case that fails. It tests that if a change reports that an element was added, the added element should be in the set.

    @Test
    public void testRecursiveChanges() {
        ObservableSortedArraySet<Integer> set = new ObservableSortedArraySet<>(
                Integer::compareTo, i -> Collections.emptyList());
        set.addListener((SetChangeListener.Change<?> change) -> {
            if(change.wasAdded()) {
                set.remove(change.getElementAdded());
            }
        });
        set.addListener((SetChangeListener.Change<?> change) -> {
            if(change.wasAdded()) {
                // if an element was added, it should be in the set
                assertThat(set, contains(change.getElementAdded()));
            }
        });
        set.add(5);
    }

TomasMikula avatar Sep 24 '15 18:09 TomasMikula