OperatorWeakBinding (deprecates OperatorObserveFromAndroidComponent) #938

mttkay · 2014-03-02T12:13:56Z

Android UI operator that weakly binds to a fragment or activity. (see discussion in #899)

mttkay · 2014-03-02T12:14:32Z

rxjava-contrib/rxjava-android/src/main/java/rx/operators/OperatorWeakBinding.java

+
+        private void handleLostBinding(Subscriber<? super T> sub, String context) {
+            if (sub == null) {
+                Log.d(LOG_TAG, "subscriber gone; skipping " + context);


Should guard this log call

cloudbees-pull-request-builder · 2014-03-02T12:14:58Z

RxJava-pull-requests #877 FAILURE
Looks like there's a problem with this pull request

mttkay · 2014-03-02T13:21:36Z

rxjava-contrib/rxjava-android/src/main/java/rx/operators/OperatorWeakBinding.java

+        private final WeakReference<Subscriber<? super T>> subscriberRef;
+        private final WeakReference<R> boundRef;
+
+        private WeakSubscriber(Subscriber<? super T> op, WeakReference<R> boundRef) {


Should this be super(op)?

mttkay · 2014-03-06T11:22:20Z

rxjava-contrib/rxjava-android/src/main/java/rx/android/observables/AndroidObservable.java

@@ -95,4 +101,21 @@ private AndroidObservable() {}
            throw new IllegalArgumentException("Target fragment is neither a native nor support library Fragment");
        }
    }
+
+    public static <T> Observable<T> bindActivity(Activity activity, Observable<T> cachedSequence) {


we don't actually have to assume that the sequence is cached. A PublishSubject doesn't cache but still makes sense to weakly bind to when not calling unsubscribe explicitly

samueltardieu · 2014-03-06T19:20:37Z

Couldn't you, in addition, check Activity#isFinishing to prevent values from being delivered to a dying (or already dead) activity?

mttkay · 2014-03-06T20:45:23Z

That should not be necessary, and it is something that we only realized
after writing the very first version of the current operator.

Android guarantees that it will not process messages entering the main
looper while the current Activity is in the process of being recreated,
that is, between calls to onDestroy of instance A and onCreate of instance
B of the same class.

So as long as you use OperatorWeakBinding with the main thread scheduler,
any Rx notifications will be queued up until after onCreate should they
arrive while the Activity is gone.

bindActivity in AndroidObservable combines the two to achieve this.

Does that make sense?

samueltardieu · 2014-03-06T21:13:27Z

I am more concerned about what happens when the activity is gone (replaced by another one) but not yet reclaimed by the GC. Won't items be delivered to the UI thread which is now unrelated to the previous activity?

mttkay · 2014-03-07T06:50:01Z

No that's what I tried to say: Android stops processing the main looper
during a configuration change. Since HandlerThreadScheduler posts
notifications to a looper, they will queue up until your activity exists
again.

samueltardieu · 2014-03-07T08:42:03Z

We obviously don't understand each other :) I'll try again.

I'm not talking about the case where the activity is recreated, but where it is destroyed and another unrelated activity is started. The same thread can be reused for the UI of the new activity, and AFAIK (but I may be wrong) the looper is tied to a thread, not to the activity.

I understand that the looper will be suspended between the previous activity death and the new activity creation, but won't queued messages then be delivered to the new activity which would use the same looper as the previous one?

I have had cases where messages were delivered through a Handler to a destroyed activity, which manifested through an error while attempting to do UI operations on now-not-displayed components, and I wonder whether this could be the case here.

mttkay · 2014-03-07T08:45:26Z

Not if you unsubscribe from the Activity/subscriber in onDestroy or let the
weak binding do that for you. Am I missing something?
On Mar 7, 2014 9:42 AM, "Samuel Tardieu" notifications@github.com wrote:

We obviously don't understand each other :) I'll try again.

I'm not talking about the case where the activity is recreated, but where
it is destroyed and another unrelated activity is started. The same
thread can be reused for the UI of the new activity, and AFAIK (but I
may be wrong) the looper is tied to a thread, not to the activity.

I understand that the looper will be suspended between the previous
activity death and the new activity creation, but won't queued messages
then be delivered to the new activity which would use the same looper as
the previous one?

I have had cases where messages were delivered through a Handler to a
destroyed activity, which manifested through an error while attempting to
do UI operations on now-not-displayed components, and I wonder whether this
could be the case here.

Reply to this email directly or view it on GitHubhttps://github.com//pull/938#issuecomment-36977549
.

samueltardieu · 2014-03-07T08:49:54Z

The weak binding will unsubscribe when the activity is freed by the garbage collector. This may happen a long time after the activity is destroyed. This is not a synchronous process as it would be in Python where references are reference-counted. Here, we have to wait until the garbage collector decides to collect unreferenced objects for the weak reference to become null.

(of course, when unsubscribing explicitly, there is no problem :-)

mttkay · 2014-03-07T08:51:31Z

FWIW, I should add that the implementation you're now using does not check
for isFinishing and I wasn't able to reproduce this scenario in the tests I
ran with OperatorWeakBinding. I will give it another closer look though.

Moreover, if we find we do need this check after all, I suggest we follow
the advice from the first issue linked, which is to pass a predicate to the
operator that can perform such checks, or alternatively, call filter on the
result of the weak binding.

mttkay · 2014-03-12T13:59:55Z

@samueltardieu Sorry for the delay, finally got time to work on this. You were right, there is a case in which this can still happen, which is when exiting the activity via the back button. I still couldn't reproduce it when going through a rotation change, but that case is sufficient to warrant a change or extension to this.

My suggestion is then to extend this operator to also address what @tehmou asked for, which is to be able to pass in a predicate that tests for validity of the bound component. This could be isFinishing for an activity, or isAdded for a fragment. I would like to keep the operator itself to remain fairly generic, where it would perform the following things:

weakly bind a reference of R
weakly bind the subscriber
accept a predicate in the constructor that performs tests on the R instance for every notification

this test can then be customized, but the bindActivity and bindFragment helpers could still pass a default predicate implementing above mentioned checks.

Does that make sense? I'll implement this, run some tests and then push it for you guys to reviews.

cloudbees-pull-request-builder · 2014-03-12T17:37:20Z

RxJava-pull-requests #898 FAILURE
Looks like there's a problem with this pull request

cloudbees-pull-request-builder · 2014-03-12T17:48:20Z

RxJava-pull-requests #899 SUCCESS
This pull request looks good

zsxwing · 2014-03-13T06:59:28Z

rxjava-contrib/rxjava-android/src/main/java/rx/operators/OperatorWeakBinding.java

+        final WeakReference<Subscriber<? super T>> subscriberRef;
+
+        private WeakSubscriber(Subscriber<? super T> source) {
+            subscriberRef = new WeakReference<Subscriber<? super T>>(source);


I think you need to add super(source); here to chain the Subscription.

Good catch!

cloudbees-pull-request-builder · 2014-03-13T10:24:50Z

RxJava-pull-requests #900 SUCCESS
This pull request looks good

cloudbees-pull-request-builder · 2014-03-13T10:31:14Z

RxJava-pull-requests #901 SUCCESS
This pull request looks good

OperatorWeakBinding (deprecates OperatorObserveFromAndroidComponent)

First implementation of OperatorWeakBinding

955bc50

mttkay reviewed Mar 2, 2014
View reviewed changes

mttkay mentioned this pull request Mar 6, 2014

A number of improvements to OperatorObserveFromAndroidComponent #931

Merged

mttkay reviewed Mar 6, 2014
View reviewed changes

mttkay added 4 commits March 12, 2014 18:10

merge remote-tracking branch 'netflix/master' into operator-weak-binding

0ff4237

OperatorWeakBinding supports predicates now

d7037b5

Some cleaning up

135b6c1

Guard the log calls

2f0670e

mttkay mentioned this pull request Mar 12, 2014

OperatorObserveFromAndroidComponent to support Fragment.isVisible() #899

Closed

zsxwing reviewed Mar 13, 2014
View reviewed changes

mttkay added 2 commits March 13, 2014 11:14

Forward subscription of wrapped subscriber

45afeb9

Rewrite AndroidObservableTest to test the new operator

7ba5be9

benjchristensen added a commit that referenced this pull request Mar 13, 2014

Merge pull request #938 from soundcloud/operator-weak-binding

90d5978

OperatorWeakBinding (deprecates OperatorObserveFromAndroidComponent)

benjchristensen merged commit 90d5978 into ReactiveX:master Mar 13, 2014

mttkay deleted the operator-weak-binding branch March 13, 2014 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OperatorWeakBinding (deprecates OperatorObserveFromAndroidComponent) #938

OperatorWeakBinding (deprecates OperatorObserveFromAndroidComponent) #938

mttkay commented Mar 2, 2014

mttkay Mar 2, 2014

cloudbees-pull-request-builder commented Mar 2, 2014

mttkay Mar 2, 2014

mttkay Mar 6, 2014

samueltardieu commented Mar 6, 2014

mttkay commented Mar 6, 2014

samueltardieu commented Mar 6, 2014

mttkay commented Mar 7, 2014

samueltardieu commented Mar 7, 2014

mttkay commented Mar 7, 2014

samueltardieu commented Mar 7, 2014

mttkay commented Mar 7, 2014

mttkay commented Mar 12, 2014

cloudbees-pull-request-builder commented Mar 12, 2014

cloudbees-pull-request-builder commented Mar 12, 2014

zsxwing Mar 13, 2014

mttkay Mar 13, 2014

cloudbees-pull-request-builder commented Mar 13, 2014

cloudbees-pull-request-builder commented Mar 13, 2014

OperatorWeakBinding (deprecates OperatorObserveFromAndroidComponent) #938

OperatorWeakBinding (deprecates OperatorObserveFromAndroidComponent) #938

Conversation

mttkay commented Mar 2, 2014

mttkay Mar 2, 2014

Choose a reason for hiding this comment

cloudbees-pull-request-builder commented Mar 2, 2014

mttkay Mar 2, 2014

Choose a reason for hiding this comment

mttkay Mar 6, 2014

Choose a reason for hiding this comment

samueltardieu commented Mar 6, 2014

mttkay commented Mar 6, 2014

samueltardieu commented Mar 6, 2014

mttkay commented Mar 7, 2014

samueltardieu commented Mar 7, 2014

mttkay commented Mar 7, 2014

samueltardieu commented Mar 7, 2014

mttkay commented Mar 7, 2014

mttkay commented Mar 12, 2014

cloudbees-pull-request-builder commented Mar 12, 2014

cloudbees-pull-request-builder commented Mar 12, 2014

zsxwing Mar 13, 2014

Choose a reason for hiding this comment

mttkay Mar 13, 2014

Choose a reason for hiding this comment

cloudbees-pull-request-builder commented Mar 13, 2014

cloudbees-pull-request-builder commented Mar 13, 2014