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

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

Open
Soamid opened this issue Jun 15, 2022 · 1 comment
Open

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

Soamid opened this issue Jun 15, 2022 · 1 comment
Labels
status: help wanted Extra attention is needed type: bug Something isn't working

Comments

@Soamid
Copy link

Soamid commented Jun 15, 2022

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
Copy link
Owner

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?

@tobiasdiez tobiasdiez added type: bug Something isn't working status: help wanted Extra attention is needed labels Sep 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: help wanted Extra attention is needed type: bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants