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

Collection API Proposal #1

Merged
merged 2 commits into from
Jan 29, 2017
Merged

Collection API Proposal #1

merged 2 commits into from
Jan 29, 2017

Conversation

miho
Copy link
Contributor

@miho miho commented Jan 19, 2017

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:

/**
 * TODO
 * @author Hendrik Ebbers
 */
public interface ObservableCollection<E, C extends ObservableCollection<E, C, ?>, L extends CollectionChange<E, C>> extends Collection<E> {

    Subscription onChanged(CollectionChangeListener<? super E, C, L> listener);

}

class MyList<T> extends AbstractList<T> implements ObservableCollection<T, ObservableList<T>, CollectionChange<T, ObservableList<T>>> {

    @Override
    public Subscription onChanged(CollectionChangeListener<? super T, ObservableList<T>, CollectionChange<T, ObservableList<T>>> listener) {
        throw new UnsupportedOperationException("Bad, since it only supports list listeners and not collection listeners");
    }

    @Override
    public T get(int index) {
        throw new UnsupportedOperationException("Not supported yet.");
    }

    @Override
    public int size() {
        throw new UnsupportedOperationException("Not supported yet.");
    }    
}

TODOs

  • think about current limitations of lower bounds (only wildcards possible)
  • is there an alternative design that prevents wildcards but allows lower bounds?

miho added 2 commits January 17, 2017 18:07
- 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?
@miho
Copy link
Contributor Author

miho commented Jan 20, 2017

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.

Copy link
Member

@hendrikebbers hendrikebbers left a 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();
Copy link
Member

@hendrikebbers hendrikebbers Jan 21, 2017

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Member

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()?

Copy link
Contributor Author

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>> {
Copy link
Member

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> ?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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();
Copy link
Member

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>> {
Copy link
Member

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.

Copy link
Contributor Author

@miho miho Jan 21, 2017

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.

Copy link
Member

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?

Copy link
Contributor Author

@miho miho Jan 21, 2017

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.

Copy link
Member

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.

Copy link
Member

@hendrikebbers hendrikebbers Jan 21, 2017

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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();
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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.

@hendrikebbers
Copy link
Member

think about current limitations of lower bounds (only wildcards possible)
is there an alternative design that prevents wildcards but allows lower bounds?

@miho can you describe this more deeply? I do not really understand what you mean...

@miho
Copy link
Contributor Author

miho commented Jan 21, 2017

Hi Hendrik,

<lang:german> ich hoffe, dir geht's wieder gut :) </lang:german>

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,
Michael

@HanSolo
Copy link

HanSolo commented Jan 21, 2017

I would personally prefer value instead of newValue...just my 0.02€ ;)

@hendrikebbers
Copy link
Member

@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 willSet(...) and didSet(...) while in the willSet(...) you can access newValue, in didSet(...) it is simply called value. Maybe we should think about something like a willSet(...) listener that can be used for validation etc. Based on some discussions I would currently prefer:

  • 1 listener type that will be called before the change. This listener will be called always (even if new value is equals old value). This listeners needs to have access to the old and new values. Maybe this can return a boolean value to interrupt the change based on validation etc.
  • 1 listener type that will be called after the change. This listener will only be called if the value really changed (old != new). Here you can access the value and the old value.

What do you think. And yes, this is not the right please for this discussion and based on this I created #2
So should discuss this point in the issue ;)

@hendrikebbers
Copy link
Member

@miho
<lang:german> Naja, Erkältung halt. Was soll man da machen... Aber danke der Nachfrage :) </lang:german>

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.

3 participants