Add OperatorAdapterViewOnItemClick to observe OnItemClick events in AdapterViews. Resolve #19.

[kboyarshinov]

• new OperatorAdapterViewOnItemClick to handle onItemClick events from AdapterViews;
• separate OnItemClickEvent class to observe it;
• corresponding itemClicks method at ViewObservable;
• tests for ListView and GridView, also with multiple subscriptions.

[nsk-mironov]

LGTM, but I think we should make an agreement on ViewObservable conventions. At the moment all ViewObservable methods are inconsistent with each other and emit different type of things:

• ViewObservable.clicks emits a view itself
• ViewObservable.input emits a boolean value
• ViewObservable.itemClicks emits a custom event type

I really like the idea of OnItemClickEvent and I think we should follow the same convention for other methods:

public static <T extends View> Observable<OnClickEvent<T>> clicks(final T view, final boolean emitInitialValue);
public static <T extends TextView> Observable<OnTextChangeEvent<T>> text(final T view, final boolean emitInitialValue);
public static <T extends CompoundButton> Observable<OnCheckedChangeEvent<T>> input(final T view, final boolean emitInitialValue);

public class OnClickEvent<T extends View> {
    public final T view;
}

public class OnTextChangeEvent<T extends TextView> {
    public final T view;
    public final CharSequence text;
}

public class OnCheckedChangeEvent<T extends CompoundButton> {
    public final T view;
    public final boolean value;
}

I know, it's a breaking change, but RxAndroid is still not even 1.0, so breaking changes should be expected. Also, it will be really easy to solve #7 using this approach.

Any thoughts?

/cc @mttkay

[mttkay]

@mironov-nsk that sounds sensible. I have to say we don't use RxJava on the view level very much, so I'll let you guys drive the discussion, but I think it's a good idea what you propose. I'm fine with breaking changes on the road to 1.0; the whole idea behind breaking out the contrib module from core was to have independent roadmaps. There's no urgency to rush any of this from my point of view. RxJava had many breaking changes over the last year and a half on its way to 1.0. That's only expected.

[kboyarshinov]

@mironov-nsk events convention looks better than view and primitive types. Agree with that.
Considering this OnItemClickEvent would look like

public final class OnItemClickEvent <T extends AdapterView<?>> {
    public final T parent;
    public final View view;
    public final int position;
    public final long id;

    public OnItemClickEvent(T parent, View view, int position, long id) {
        this.parent = parent;
        this.view = view;
        this.position = position;
        this.id = id;
    }
}

Also we should do something with AutoCompleteTextView suggestions list mentioned in #19. Corresponding operator would be the same as for AdapterView cause AutoCompleteTextView is just wrapping DropDownListView. But, not as testable. Adding just operator without tests is bad idea.

[nsk-mironov]

@naghtarr I'm also wondering if events should be generics or not. Observable<OnItemClickEvent<ListView>> and Observable<OnTextChangeEvent<TextView>> look a bit verbose to me, so it's probably better to keep a reference to a base type (as you did in your pull request) and make it Observable<OnItemClickEvent> and Observable<OnTextChangeEvent>.

[kboyarshinov]

@mironov-nsk Observable<OnItemClickEvent> and Observable<OnTextChangeEvent> are better. Also it allows to use these events in more complex views like AutoCompleteTextView. Generics will be a restriction.

[kboyarshinov]

Cleaned up generics as discussed. Moved to package rx.android.operators. Made OnItemClickEvent class non-final. Changes are consisted with #22.

[mttkay]

Just wondering, are we just being defensive here? As far as I understand, unsubscribing removes a listener already, so why use weak references?

Also, I don't think this should be static, as this means we're sharing mutable state with multiple instances of this class?

[nsk-mironov]

@mttkay We're not defensive here, this is the only way I found to make multiple subscriptions work. There are two problems here:

• Methods like setOnClickListener and setOnItemClickListener allow to set only one listener, making multiple subscriptions hard to implement. This is the reason we need classes like CompositeOnClickListener.
• There is no way to get a OnXXXListener attached to the View (since there are no getOnXXXListener methods), so we need to save listeners somewhere else. View.setTag(Object tag) is not an option here since it's heavily used by the end developers. View.setTag(int key, Object key) could be a solution, but it requires a key to be declared in the resources, making it impossible to ship the library as a jar.

The only working solution I found is to store CompositeXXXListeners in the global WeekHasMap (it have to be static). Any other ideas how to solve this problem are welcome.

[mttkay]

I don't understand. Are you expecting that this operator is called multiple times for different listeners, i.e. will call be executed more than once on the same observable?

[nsk-mironov]

Regarding the question why it have to be a WeakHashMap. Well, it could be just a usual HashMap, but in this case user have to unsubscribe from the Observable in order to prevent memory leaks, making the usage of ViewObservables a bit more complicated.

[nsk-mironov]

I don't understand. Are you expecting that this operator is called multiple times for different listeners, i.e. will call be executed more than once on the same observable?

Yes, call will be executed more than once if user subscribes to the Observable multiple times.

[mttkay]

Making it a hot observable? Why not deal with this using a built in operator like publish? Then you can have multiple subscribers for the same OnClick event

[nsk-mironov]

Yes, it could be a hot Observable, but we still need somehow to attach the Observable to the View

[mttkay]

From a naive standpoint, I would expect it to work like this:

ConnectableObservable source = sourceSequence.lift(new OperatorAdapterViewOnItemClick(listView)).publish();
source.connect();
source.subscribe(o1);
source.subscribe(o2);

but I might be missing something

[nsk-mironov]

The point here is to make all this complicity transparent to the end developers. I don't think it's a good idea to force them to use ConnectableObservable instead of providing the same functionality out of the box.

By the way, OperatorAdapterViewOnItemClick is not a real Operator, since it implements OnSubscribe interface, but not the Operator one. Probably we should follow the conventions used in RxJava and name it something like OnSubscribeViewOnItemClick (and rename all other "operators" as well).

[kboyarshinov]

Probably we should follow the conventions used in RxJava and name it something like OnSubscribeViewOnItemClick (and rename all other "operators" as well).

@mironov-nsk Agreed. But I think we should implement this in separate PR.

[mttkay]

Cool. Is this ready to land?

[kboyarshinov]

@mttkay Yes.

[mttkay]

Sorry, landing another PR made this unmergeable. Will merge after this is resolved!

[kboyarshinov]

@mttkay, Merged 0.x changes to this branch.

[mttkay]

👍