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.

Sign up for GitHub

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

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

Merged
merged 7 commits into from
Oct 15, 2014
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
583c2ed
Add OperatorAdapterViewOnItemClick to observe OnItemClick events in A…
Oct 5, 2014
f2ddd6a
Move OperatorAdaperViewOnItemClick to rx.android.operators package
Oct 8, 2014
c26f2e2
Remove useless generics regarding OperatorAdapterViewOnItemClick
Oct 8, 2014
46ea1a0
Make OnItemClickEvent class non-final
Oct 8, 2014
6b7cfe2
Break up tests from OperatorAdapterViewOnItemClickTest into separated…
Oct 11, 2014
e800920
Merge branch '0.x' to 'naghtarr/itemclicks'
Oct 15, 2014
db454db
Move OperatorSharedPreferenceChange to rx.android.operators package
Oct 15, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/main/java/rx/android/events/OnItemClickEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package rx.android.events;

import android.view.View;
import android.widget.Adapter;
import android.widget.AdapterView;

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

public OnItemClickEvent(AdapterView<?> parent, View view, int position, long id) {
this.parent = parent;
this.view = view;
this.position = position;
this.id = id;
}
}
20 changes: 10 additions & 10 deletions src/main/java/rx/android/observables/AndroidObservable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,23 @@
*/
package rx.android.observables;

import static rx.android.schedulers.AndroidSchedulers.mainThread;

import android.content.SharedPreferences;
import rx.Observable;
import rx.functions.Func1;
import rx.android.operators.OperatorBroadcastRegister;
import rx.android.operators.OperatorConditionalBinding;
import rx.android.operators.OperatorLocalBroadcastRegister;

import android.app.Activity;
import android.app.Fragment;
import android.content.Context;
import android.content.Intent;
import android.content.IntentFilter;
import android.content.SharedPreferences;
import android.os.Build;
import android.os.Handler;
import rx.operators.OperatorSharedPreferenceChange;

import rx.Observable;
import rx.android.operators.OperatorBroadcastRegister;
import rx.android.operators.OperatorConditionalBinding;
import rx.android.operators.OperatorLocalBroadcastRegister;
import rx.android.operators.OperatorSharedPreferenceChange;
import rx.functions.Func1;

import static rx.android.schedulers.AndroidSchedulers.mainThread;


public final class AndroidObservable {
Expand Down
15 changes: 11 additions & 4 deletions src/main/java/rx/android/observables/ViewObservable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,21 @@
*/
package rx.android.observables;

import android.view.View;
import android.widget.AdapterView;
import android.widget.CompoundButton;
import android.widget.TextView;

import rx.Observable;
import rx.android.events.OnCheckedChangeEvent;
import rx.android.events.OnClickEvent;
import rx.android.events.OnItemClickEvent;
import rx.android.events.OnTextChangeEvent;
import rx.android.operators.OperatorAdapterViewOnItemClick;
import rx.android.operators.OperatorCompoundButtonInput;
import rx.android.operators.OperatorTextViewInput;
import rx.android.operators.OperatorViewClick;

import android.view.View;
import android.widget.CompoundButton;
import android.widget.TextView;

public class ViewObservable {

public static Observable<OnClickEvent> clicks(final View view) {
Expand All @@ -53,4 +56,8 @@ public static Observable<OnCheckedChangeEvent> input(final CompoundButton button
return Observable.create(new OperatorCompoundButtonInput(button, emitInitialValue));
}

public static Observable<OnItemClickEvent> itemClicks(final AdapterView<?> adapterView) {
return Observable.create(new OperatorAdapterViewOnItemClick(adapterView));
}

}
103 changes: 103 additions & 0 deletions src/main/java/rx/android/operators/OperatorAdapterViewOnItemClick.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
/**
* Copyright 2014 Netflix, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package rx.android.operators;

import android.view.View;
import android.widget.AbsListView;
import android.widget.AdapterView;

import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.WeakHashMap;

import rx.Observable;
import rx.Subscriber;
import rx.Subscription;
import rx.android.events.OnItemClickEvent;
import rx.android.observables.Assertions;
import rx.android.subscriptions.AndroidSubscriptions;
import rx.functions.Action0;

public class OperatorAdapterViewOnItemClick implements Observable.OnSubscribe<OnItemClickEvent> {

private final AdapterView<?> adapterView;

public OperatorAdapterViewOnItemClick(final AdapterView<?> adapterView) {
this.adapterView = adapterView;
}

@Override
public void call(final Subscriber<? super OnItemClickEvent> observer) {
Assertions.assertUiThread();
final CompositeOnClickListener composite = CachedListeners.getFromViewOrCreate(adapterView);

final AbsListView.OnItemClickListener listener = new AdapterView.OnItemClickListener() {
@Override
public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
observer.onNext(new OnItemClickEvent(parent, view, position, id));
}
};

final Subscription subscription = AndroidSubscriptions.unsubscribeInUiThread(new Action0() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this? You're asserting that you're subscribing from the main UI thread already. AFAIK subscribeOn and observeOn ensure that the subscription is unsubscribed from on the same thread? So you either are already on that thread (as asserted above) or observeOn does that for you if you observeOn(mainThread())

Keep in mind that using the HandlerThreadScheduler means another roundtrip through the message loop, so this will unnecessarily delay unsubscription unless I'm missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this?

The original ViewObservable operators don't have this check. I'm not sure why it was added after.

AFAIK subscribeOn and observeOn ensure that the subscription is unsubscribed from on the same thread

There is an Observable.unsubscribeOn(Scheduler) operator.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, didn't know unsubscribeOn, thanks! What I was referring to is this bit:

https://github.com/ReactiveX/RxJava/blob/1.x/src/main/java/rx/internal/operators/OperatorObserveOn.java#L228

@Override
public void call() {
composite.removeOnClickListener(listener);
}
});

composite.addOnClickListener(listener);
observer.add(subscription);
}

private static class CompositeOnClickListener implements AbsListView.OnItemClickListener {
private final List<AbsListView.OnItemClickListener> listeners = new ArrayList<AbsListView.OnItemClickListener>();

public boolean addOnClickListener(final AbsListView.OnItemClickListener listener) {
return listeners.add(listener);
}

public boolean removeOnClickListener(final AbsListView.OnItemClickListener listener) {
return listeners.remove(listener);
}

@Override
public void onItemClick(AdapterView<?> parent, View view, int position, long id) {
for (AdapterView.OnItemClickListener listener : listeners) {
listener.onItemClick(parent, view, position, id);
}
}
}

private static class CachedListeners {
private static final Map<AdapterView<?>, CompositeOnClickListener> sCachedListeners = new WeakHashMap<AdapterView<?>, CompositeOnClickListener>();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


public static CompositeOnClickListener getFromViewOrCreate(final AdapterView<?> view) {
final CompositeOnClickListener cached = sCachedListeners.get(view);

if (cached != null) {
return cached;
}

final CompositeOnClickListener listener = new CompositeOnClickListener();

sCachedListeners.put(view, listener);
view.setOnItemClickListener(listener);

return listener;
}
}
}
2 changes: 1 addition & 1 deletion ...ators/OperatorSharedPreferenceChange.java → ...ators/OperatorSharedPreferenceChange.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package rx.operators;
package rx.android.operators;

import android.content.SharedPreferences;
import rx.Observable;
Expand Down
Loading