Refactoring OperatorViewDetachedFromWindowFirst

1. Renaming to OnSubscribeViewDetachedFromWindowFirst.java
2. Adding tests to assert the desired behaviour
3. Refactoring to avoid the "instantiate and forget" approach
within OnSubscribe.call(..)

Signed-off-by: David Marques <[email protected]>

Author: dpsm

rxandroid/src/main/java/rx/android/view/OperatorViewDetachedFromWindowFirst.java renamed to rxandroid/src/main/java/rx/android/view/OnSubscribeViewDetachedFromWindowFirst.java

@@ -20,32 +20,33 @@
 import rx.Subscription;
 
 /**
- * An internal class that is used from #{@link ViewObservable#bindView}.
+ * An internal class that is used from #{@link rx.android.view.ViewObservable#bindView}.
  * This emits an event when the given #{@code View} is detached from the window for the first time.
  */
-final class OperatorViewDetachedFromWindowFirst implements Observable.OnSubscribe<View> {
+public class OnSubscribeViewDetachedFromWindowFirst implements Observable.OnSubscribe<View> {
     private final View view;
 
-    public OperatorViewDetachedFromWindowFirst(View view) {
+    public OnSubscribeViewDetachedFromWindowFirst(View view) {
         this.view = view;
     }
 
     @Override
     public void call(final Subscriber<? super View> subscriber) {
-        new ListenerSubscription(subscriber, view);
+        final SubscriptionAdapter adapter = new SubscriptionAdapter(subscriber, view);
+        subscriber.add(adapter);
+        view.addOnAttachStateChangeListener(adapter);
     }
 
     // This could be split into a couple of anonymous classes.
     // We pack it into one for the sake of memory efficiency.
-    private static class ListenerSubscription implements View.OnAttachStateChangeListener, Subscription {
+    private static class SubscriptionAdapter implements View.OnAttachStateChangeListener,
+        Subscription {
         private Subscriber<? super View> subscriber;
         private View view;
 
-        public ListenerSubscription(Subscriber<? super View> subscriber, View view) {
+        public SubscriptionAdapter(Subscriber<? super View> subscriber, View view) {
             this.subscriber = subscriber;
             this.view = view;
-            view.addOnAttachStateChangeListener(this);
-            subscriber.add(this);
         }
 
         @Override
@@ -56,7 +57,7 @@ public void onViewAttachedToWindow(View v) {
         public void onViewDetachedFromWindow(View v) {
             if (!isUnsubscribed()) {
                 Subscriber<? super View> originalSubscriber = subscriber;
-                clear();
+                unsubscribe();
                 originalSubscriber.onNext(v);
                 originalSubscriber.onCompleted();
             }
@@ -65,19 +66,15 @@ public void onViewDetachedFromWindow(View v) {
         @Override
         public void unsubscribe() {
             if (!isUnsubscribed()) {
-                clear();
+                view.removeOnAttachStateChangeListener(this);
+                view = null;
+                subscriber = null;
             }
         }
 
         @Override
         public boolean isUnsubscribed() {
             return view == null;
         }
-
-        private void clear() {
-            view.removeOnAttachStateChangeListener(this);
-            view = null;
-            subscriber = null;
-        }
     }
 }

rxandroid/src/main/java/rx/android/view/ViewObservable.java

@@ -51,6 +51,6 @@ public static <T> Observable<T> bindView(View view, Observable<T> source) {
         if (view == null || source == null)
             throw new IllegalArgumentException("View and Observable must be given");
         Assertions.assertUiThread();
-        return source.takeUntil(Observable.create(new OperatorViewDetachedFromWindowFirst(view))).observeOn(mainThread());
+        return source.takeUntil(Observable.create(new OnSubscribeViewDetachedFromWindowFirst(view))).observeOn(mainThread());
     }
 }

rxandroid/src/test/java/rx/android/operators/OnSubscribeViewDetachedFromWindowFirstTest.java

@@ -0,0 +1,86 @@
+/**
+ * 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 junit.framework.Assert;
+
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.Matchers;
+import org.robolectric.RobolectricTestRunner;
+
+import rx.Observable;
+import rx.Subscriber;
+import rx.android.view.OnSubscribeViewDetachedFromWindowFirst;
+import rx.observers.TestSubscriber;
+
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.verify;
+
+@RunWith(RobolectricTestRunner.class)
+public class OnSubscribeViewDetachedFromWindowFirstTest {
+
+    @Test
+    public void testGivenSubscriptionWhenViewDetachedThenUnsubscribesAndRemovesListener() {
+        final Subscriber<View> subscriber = spy(new TestSubscriber<View>());
+        final View view = mock(View.class);
+        final Observable<View> observable = Observable.create(new OnSubscribeViewDetachedFromWindowFirst(view));
+        observable.subscribe(subscriber);
+
+        verify(subscriber, never()).onNext(view);
+        verify(subscriber, never()).onError(Matchers.any(Throwable.class));
+        verify(subscriber, never()).onCompleted();
+
+        final ArgumentCaptor<View.OnAttachStateChangeListener> captor =
+            ArgumentCaptor.forClass(View.OnAttachStateChangeListener.class);
+        verify(view).addOnAttachStateChangeListener(captor.capture());
+
+        final View.OnAttachStateChangeListener listener = captor.getValue();
+        Assert.assertNotNull("Should have added listener on subscription.", listener);
+
+        listener.onViewDetachedFromWindow(view);
+
+        verify(subscriber, never()).onError(Matchers.any(Throwable.class));
+        verify(subscriber).onNext(view);
+        verify(subscriber).onCompleted();
+
+        verify(view).removeOnAttachStateChangeListener(listener);
+    }
+
+    @Test
+    public void testGivenSubscriptionWhenUnsubscribedThenStateListenerRemoved() {
+        final Subscriber<View> subscriber = spy(new TestSubscriber<View>());
+        final View view = mock(View.class);
+        final Observable<View> observable = Observable.create(new OnSubscribeViewDetachedFromWindowFirst(view));
+        observable.subscribe(subscriber).unsubscribe();
+
+        verify(subscriber, never()).onNext(view);
+        verify(subscriber, never()).onError(Matchers.any(Throwable.class));
+        verify(subscriber, never()).onCompleted();
+
+        final ArgumentCaptor<View.OnAttachStateChangeListener> captor =
+            ArgumentCaptor.forClass(View.OnAttachStateChangeListener.class);
+        verify(view).addOnAttachStateChangeListener(captor.capture());
+
+        final View.OnAttachStateChangeListener listener = captor.getValue();
+        Assert.assertNotNull("Should have added listener on subscription.", listener);
+
+        verify(view).removeOnAttachStateChangeListener(listener);
+    }
+}