-
Notifications
You must be signed in to change notification settings - Fork 7
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
Collection API Proposal #1
Conversation
- adresses issues with changed elements (lists get indices as well as elements) - fixed issues with generics (previously no lower bound was defined for listeners) - this API proposal is now used for the VCollections project TODO - think about current limitations of lower bounds (only wildcards possible) - is there an alternative design that prevents wildcards but allows lower bounds?
The current collection API proposal is now fully implemented for lists as part of VCollections. As soon as Maps and Sets are added, I will add support for them as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are on a good way :)
* @return the change that contains all elements that were added during this | ||
* event | ||
*/ | ||
CC added(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should return Optional<CC>
? In this case we do not even need the was added()
method..
We should think about this:
CC getAdded();
default Optional<CC> added() { return Optional.ofNullable(getAdded());}
default boolean wasAdded() { return added().isPresent(); }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my implementation added() and removed() are never null. There's only one static final empty object that is used for empty changes. No real need for Optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wasAdded() is not really necessary but I like to have it because the code shows clearly what I'm doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, can we add a default implementation for wasAdded()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Sorry for ignoring this comment. Will do another PR.
*/ | ||
public interface CollectionChangeEvent<E, C extends ObservableCollection<E, C, ?>, V extends CollectionChange<E, C>> { | ||
public interface CollectionChangeEvent<T, OC extends Collection<T>, CC extends CollectionChange<T>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not OC extends ObservableCollection<T>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a problem here:
OC extends ObservableCollection<T>
doesn't work, since the event could come from a collection that is not directly the observable. Just tried to change this in my implementation and things broke apart.
When deciding whether the source type should be the CollectionObservable
or the actual Collection
, I think the collection is more important and useful.
So I'm skeptical about this change.
* @return the change that contains all elements that were removed during | ||
* this event | ||
*/ | ||
CC removed(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment for added
* | ||
* @author Michael Hoffer ([email protected]) | ||
*/ | ||
public interface CollectionObservable<T, OC extends Collection<T>, CC extends CollectionChange<T>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should think about the name of the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like ObservableCollection and the like. The idea is that one can have the use case of observables that track changes of collections but are no collections themselves. But I'm fine with another name that expresses the same meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmh, let me think about this. Currently I do not see how this should work if the collection is not an ObservableCollection. Do I miss something? Can you describe the use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I don't like observer patterns where the observable is forced to be the object that is observed. In many cases it is. But not always.
Sometimes it is more efficient or elegant to have only one observable for many collections. A listener subscribes to the changes of all of them. I use this pattern for hierarchical data structures. Subscribing to all collections individually is just overkill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I understand the use case and in this scenario the name makes sense. At the end most people will simple add listener to the ObservableCollection but for more specific use cases you can create a CollectionObservable that observers several collections. This sounds like a good solution for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But in this case the bug question is if the API should support such use cases. Something like:
CollectionObservable o = ObservableFactory.createObservable(listA, listB, listC, listD);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good comment. I'll add this to VCollections right now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The VListObservable interface now has static methods for this:
VListObservable<T>.of(list1,list2,list3,...)
/** | ||
* @return the indices of the changed elements | ||
*/ | ||
int[] indices(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not really stream friendly. Maybe we should return a IntStream
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my case I'm working with large collections of data. int[] is just the most efficient way to carry them around. We could optionally provide a stream via indexStream()
but there's IntStream.of()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, makes sense. Maybe we can provide a default implementation that returns an IntStream
. I would love to have the API as stream, lambda, fluent friendy as possible.
@miho can you describe this more deeply? I do not really understand what you mean... |
Hi Hendrik,
I don't like the fact that lower bounds only work on wildcards. Sometimes one looses the type information, e.g., when using elements in a collection with lower bound wildcard argument. I just added the comment because I was not sure whether I missed a concept that would allow lower bounds without wildcards. But I think I didn't. Regards, |
I would personally prefer value instead of newValue...just my 0.02€ ;) |
@HanSolo interesting. I currently had a look at the property API of Swift (https://developer.apple.com/library/content/documentation/Swift/Conceptual/Swift_Programming_Language/Properties.html#//apple_ref/doc/uid/TP40014097-CH14-ID254). Here they provide a
What do you think. And yes, this is not the right please for this discussion and based on this I created #2 |
@miho |
Collection API Proposal
This proposal addresses the following issues:
1. CollectionChange & Changed Elements:
List changes get indices as well as elements,
getFrom()
,getTo()
were not sufficient. The event itself contains a merged set of changes. This supersedes the evt.changes()
stream/list that would allow to merge changes manually. There's just one change set for added elements and one for removed elements with the additional possibility to check whether elements were replaced.2. Generics & Lower Bounds
Previously, no lower bound was defined for listeners which did not support a plain collection change listener on observable lists. Maybe I don't get it right? To me it seems that a custom observable collection should be implemented like this:
Example:
TODOs