Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add class ObservableSortedArraySet. #28

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

argv-minus-one
Copy link

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.

@TomasMikula
Copy link
Owner

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).

This is needed because some algorithms, most notably Collections.binarySort, will behave differently (and by that I mean *slowly*) on a list that doesn't implement RandomAccess.
@argv-minus-one
Copy link
Author

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?

@hastebrot
Copy link
Contributor

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.

@TomasMikula
Copy link
Owner

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);
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants