tobiasdiez / EasyBind

Custom JavaFX bindings made easy with lambdas.
BSD 2-Clause "Simplified" License
30 stars 6 forks source link

EasyBind.concat() doesn't work for (initially) empty lists #62

Open Soamid opened 2 years ago

Soamid commented 2 years ago

First of all - thanks for maintaining this project, it's a really helpful set of tools. :)

Recently I've needed concat operator for observable lists with the ability to observe changes in each of the combined lists. I've noticed EasyBind has exactly such an operator, but my app doesn't work properly in one corner case: if I concat 2 lists that are empty at the beginning and are filled later, updates don't work for one of the lists.

When I looked at the source code of this operator I've noticed FlattenedList makes a hash set of all lists at the beginning and after that, it adds listeners only to unique lists:

// We make a Unique set of source lists, otherwise the event gets called multiple
// times if there are duplicate lists.
Set<ObservableList<? extends E>> sourcesSet = new HashSet<>(sourceLists);
sourcesSet.forEach(source -> source.addListener(this::onSourceChanged));

sourceLists.addListener(this::onSourcesListChanged);

I'm not sure it's the best solution assuming that source lists are observable and - in consequence - modifiable by design. We can have two similar lists at the beginning (in particular: empty), but they might become different after adding/removing elements. If uniqueness is required, maybe you should consider comparing list references instead of equality of their contents (which is what hash set does)?

tobiasdiez commented 2 years ago

Good observation! The HashSet construction is only there to filter out multiple occurrences of the same list. So, yes, filtering by reference is the better idea. Would you like to submit a PR?