Refactor Event Listeners (#289)

* Move ExecutorEventListener to core

* Rename ExecutorEventListener to AsyncEventListener

Make it match iOS.

* Move ListenerRegistrationImpl to core

* Refactor ListenerRegistration creation

  * Pull activity-scoping out ListenerRegistrationImpl--it's a totally
    separate concern.
  * Make the relationship between the various levels of listener clearer
    in the places where we construct them.

Author: wilhuff

firebase-firestore/src/main/java/com/google/firebase/firestore/DocumentReference.java

@@ -26,7 +26,10 @@
 import com.google.android.gms.tasks.Tasks;
 import com.google.firebase.annotations.PublicApi;
 import com.google.firebase.firestore.FirebaseFirestoreException.Code;
+import com.google.firebase.firestore.core.ActivityScope;
+import com.google.firebase.firestore.core.AsyncEventListener;
 import com.google.firebase.firestore.core.EventManager.ListenOptions;
+import com.google.firebase.firestore.core.ListenerRegistrationImpl;
 import com.google.firebase.firestore.core.QueryListener;
 import com.google.firebase.firestore.core.UserData.ParsedSetData;
 import com.google.firebase.firestore.core.UserData.ParsedUpdateData;
@@ -37,9 +40,7 @@
 import com.google.firebase.firestore.model.mutation.DeleteMutation;
 import com.google.firebase.firestore.model.mutation.Precondition;
 import com.google.firebase.firestore.util.Assert;
-import com.google.firebase.firestore.util.ExecutorEventListener;
 import com.google.firebase.firestore.util.Executors;
-import com.google.firebase.firestore.util.ListenerRegistrationImpl;
 import com.google.firebase.firestore.util.Util;
 import java.util.Map;
 import java.util.concurrent.ExecutionException;
@@ -311,6 +312,7 @@ private Task<DocumentSnapshot> getViaSnapshotListener(Source source) {
     options.includeDocumentMetadataChanges = true;
     options.includeQueryMetadataChanges = true;
     options.waitForSyncWhenOnline = true;
+
     ListenerRegistration listenerRegistration =
         addSnapshotListenerInternal(
             // No need to schedule, we just set the task result directly
@@ -479,49 +481,58 @@ public ListenerRegistration addSnapshotListener(
    *
    * <p>Will be Activity scoped if the activity parameter is non-null.
    *
-   * @param executor The executor to use to call the listener.
+   * @param userExecutor The executor to use to call the listener.
    * @param options The options to use for this listen.
    * @param activity Optional activity this listener is scoped to.
-   * @param listener The event listener that will be called with the snapshots.
+   * @param userListener The user-supplied event listener that will be called with document
+   *     snapshots.
    * @return A registration object that can be used to remove the listener.
    */
   private ListenerRegistration addSnapshotListenerInternal(
-      Executor executor,
+      Executor userExecutor,
       ListenOptions options,
       @Nullable Activity activity,
-      EventListener<DocumentSnapshot> listener) {
-    ExecutorEventListener<ViewSnapshot> wrappedListener =
-        new ExecutorEventListener<>(
-            executor,
-            (snapshot, error) -> {
-              if (snapshot != null) {
-                Assert.hardAssert(
-                    snapshot.getDocuments().size() <= 1,
-                    "Too many documents returned on a document query");
-                Document document = snapshot.getDocuments().getDocument(key);
-                DocumentSnapshot documentSnapshot;
-                if (document != null) {
-                  boolean hasPendingWrites = snapshot.getMutatedKeys().contains(document.getKey());
-                  documentSnapshot =
-                      DocumentSnapshot.fromDocument(
-                          firestore, document, snapshot.isFromCache(), hasPendingWrites);
-                } else {
-                  // We don't raise `hasPendingWrites` for deleted documents.
-                  boolean hasPendingWrites = false;
-                  documentSnapshot =
-                      DocumentSnapshot.fromNoDocument(
-                          firestore, key, snapshot.isFromCache(), hasPendingWrites);
-                }
-                listener.onEvent(documentSnapshot, null);
-              } else {
-                Assert.hardAssert(error != null, "Got event without value or error set");
-                listener.onEvent(null, error);
-              }
-            });
+      EventListener<DocumentSnapshot> userListener) {
+
+    // Convert from ViewSnapshots to DocumentSnapshots.
+    EventListener<ViewSnapshot> viewListener =
+        (snapshot, error) -> {
+          if (error != null) {
+            userListener.onEvent(null, error);
+            return;
+          }
+
+          Assert.hardAssert(snapshot != null, "Got event without value or error set");
+          Assert.hardAssert(
+              snapshot.getDocuments().size() <= 1,
+              "Too many documents returned on a document query");
+
+          Document document = snapshot.getDocuments().getDocument(key);
+          DocumentSnapshot documentSnapshot;
+          if (document != null) {
+            boolean hasPendingWrites = snapshot.getMutatedKeys().contains(document.getKey());
+            documentSnapshot =
+                DocumentSnapshot.fromDocument(
+                    firestore, document, snapshot.isFromCache(), hasPendingWrites);
+          } else {
+            // We don't raise `hasPendingWrites` for deleted documents.
+            documentSnapshot =
+                DocumentSnapshot.fromNoDocument(
+                    firestore, key, snapshot.isFromCache(), /* hasPendingWrites= */ false);
+          }
+          userListener.onEvent(documentSnapshot, null);
+        };
+
+    // Call the viewListener on the userExecutor.
+    AsyncEventListener<ViewSnapshot> asyncListener =
+        new AsyncEventListener<>(userExecutor, viewListener);
+
     com.google.firebase.firestore.core.Query query = asQuery();
-    QueryListener queryListener = firestore.getClient().listen(query, options, wrappedListener);
-    return new ListenerRegistrationImpl(
-        firestore.getClient(), queryListener, activity, wrappedListener);
+    QueryListener queryListener = firestore.getClient().listen(query, options, asyncListener);
+
+    return ActivityScope.bind(
+        activity,
+        new ListenerRegistrationImpl(firestore.getClient(), queryListener, asyncListener));
   }
 
   @Override

firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java

@@ -25,10 +25,13 @@
 import com.google.android.gms.tasks.Tasks;
 import com.google.firebase.annotations.PublicApi;
 import com.google.firebase.firestore.FirebaseFirestoreException.Code;
+import com.google.firebase.firestore.core.ActivityScope;
+import com.google.firebase.firestore.core.AsyncEventListener;
 import com.google.firebase.firestore.core.Bound;
 import com.google.firebase.firestore.core.EventManager.ListenOptions;
 import com.google.firebase.firestore.core.Filter;
 import com.google.firebase.firestore.core.Filter.Operator;
+import com.google.firebase.firestore.core.ListenerRegistrationImpl;
 import com.google.firebase.firestore.core.OrderBy;
 import com.google.firebase.firestore.core.QueryListener;
 import com.google.firebase.firestore.core.RelationFilter;
@@ -39,9 +42,7 @@
 import com.google.firebase.firestore.model.value.FieldValue;
 import com.google.firebase.firestore.model.value.ReferenceValue;
 import com.google.firebase.firestore.model.value.ServerTimestampValue;
-import com.google.firebase.firestore.util.ExecutorEventListener;
 import com.google.firebase.firestore.util.Executors;
-import com.google.firebase.firestore.util.ListenerRegistrationImpl;
 import com.google.firebase.firestore.util.Util;
 import java.util.ArrayList;
 import java.util.List;
@@ -891,30 +892,37 @@ public ListenerRegistration addSnapshotListener(
    * @param executor The executor to use to call the listener.
    * @param options The options to use for this listen.
    * @param activity Optional activity this listener is scoped to.
-   * @param listener The event listener that will be called with the snapshots.
+   * @param userListener The user-supplied event listener that will be called with the snapshots.
    * @return A registration object that can be used to remove the listener.
    */
   private ListenerRegistration addSnapshotListenerInternal(
       Executor executor,
       ListenOptions options,
       @Nullable Activity activity,
-      EventListener<QuerySnapshot> listener) {
-    ExecutorEventListener<ViewSnapshot> wrappedListener =
-        new ExecutorEventListener<>(
-            executor,
-            (@Nullable ViewSnapshot snapshot, @Nullable FirebaseFirestoreException error) -> {
-              if (snapshot != null) {
-                QuerySnapshot querySnapshot = new QuerySnapshot(this, snapshot, firestore);
-                listener.onEvent(querySnapshot, null);
-              } else {
-                hardAssert(error != null, "Got event without value or error set");
-                listener.onEvent(null, error);
-              }
-            });
-
-    QueryListener queryListener = firestore.getClient().listen(query, options, wrappedListener);
-    return new ListenerRegistrationImpl(
-        firestore.getClient(), queryListener, activity, wrappedListener);
+      EventListener<QuerySnapshot> userListener) {
+
+    // Convert from ViewSnapshots to QuerySnapshots.
+    EventListener<ViewSnapshot> viewListener =
+        (@Nullable ViewSnapshot snapshot, @Nullable FirebaseFirestoreException error) -> {
+          if (error != null) {
+            userListener.onEvent(null, error);
+            return;
+          }
+
+          hardAssert(snapshot != null, "Got event without value or error set");
+
+          QuerySnapshot querySnapshot = new QuerySnapshot(this, snapshot, firestore);
+          userListener.onEvent(querySnapshot, null);
+        };
+
+    // Call the viewListener on the userExecutor.
+    AsyncEventListener<ViewSnapshot> asyncListener =
+        new AsyncEventListener<>(executor, viewListener);
+
+    QueryListener queryListener = firestore.getClient().listen(query, options, asyncListener);
+    return ActivityScope.bind(
+        activity,
+        new ListenerRegistrationImpl(firestore.getClient(), queryListener, asyncListener));
   }
 
   @Override

firebase-firestore/src/main/java/com/google/firebase/firestore/util/ListenerRegistrationImpl.java renamed to firebase-firestore/src/main/java/com/google/firebase/firestore/core/ActivityScope.java

@@ -12,22 +12,19 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package com.google.firebase.firestore.util;
+package com.google.firebase.firestore.core;
 
 import static com.google.firebase.firestore.util.Assert.hardAssert;
 
 import android.app.Activity;
 import android.support.v4.app.FragmentActivity;
 import com.google.firebase.firestore.ListenerRegistration;
-import com.google.firebase.firestore.core.FirestoreClient;
-import com.google.firebase.firestore.core.QueryListener;
-import com.google.firebase.firestore.core.ViewSnapshot;
 import java.util.ArrayList;
 import java.util.List;
 import javax.annotation.Nullable;
 
 /**
- * Implements the ListenerRegistration interface by removing a query from the listener.
+ * Scopes the lifetime of a ListenerRegistration to an Activity.
  *
  * <p>Regarding activity-scoped listeners, Android provides lifecycle callbacks (eg onStop()) that
  * custom `Activity`s can implement via subclassing. But we can't take advantage of that, since we
@@ -48,17 +45,9 @@
  *       for lifecycle callbacks instead of creating/attaching a Fragment.
  * </ol>
  */
-public class ListenerRegistrationImpl implements ListenerRegistration {
+public class ActivityScope {
 
-  private final FirestoreClient client;
-
-  /** The internal query listener object that is used to unlisten from the query. */
-  private final QueryListener queryListener;
-
-  /** The event listener for the query that raises events asynchronously. */
-  private final ExecutorEventListener<ViewSnapshot> asyncEventListener;
-
-  static class CallbackList {
+  private static class CallbackList {
     void run() {
       for (Runnable callback : callbacks) {
         if (callback != null) {
@@ -142,7 +131,7 @@ private static <T> T castFragment(Class<T> fragmentClass, @Nullable Object fragm
    * everything in this function is deprecated.
    */
   @SuppressWarnings("deprecation")
-  private void onActivityStopCallOnce(Activity activity, Runnable callback) {
+  private static void onActivityStopCallOnce(Activity activity, Runnable callback) {
     hardAssert(
         !(activity instanceof FragmentActivity),
         "onActivityStopCallOnce must be called with a *non*-FragmentActivity Activity.");
@@ -170,7 +159,7 @@ private void onActivityStopCallOnce(Activity activity, Runnable callback) {
         });
   }
 
-  private void onFragmentActivityStopCallOnce(FragmentActivity activity, Runnable callback) {
+  private static void onFragmentActivityStopCallOnce(FragmentActivity activity, Runnable callback) {
     activity.runOnUiThread(
         () -> {
           StopListenerSupportFragment fragment =
@@ -194,28 +183,16 @@ private void onFragmentActivityStopCallOnce(FragmentActivity activity, Runnable
         });
   }
 
-  /** Creates a new ListenerRegistration. Is activity-scoped if and only if activity is non-null. */
-  public ListenerRegistrationImpl(
-      FirestoreClient client,
-      QueryListener queryListener,
-      @Nullable Activity activity,
-      ExecutorEventListener<ViewSnapshot> asyncEventListener) {
-    this.client = client;
-    this.queryListener = queryListener;
-    this.asyncEventListener = asyncEventListener;
-
+  /** Binds the given registration to the lifetime of the activity. */
+  public static ListenerRegistration bind(
+      @Nullable Activity activity, ListenerRegistration registration) {
     if (activity != null) {
       if (activity instanceof FragmentActivity) {
-        onFragmentActivityStopCallOnce((FragmentActivity) activity, this::remove);
+        onFragmentActivityStopCallOnce((FragmentActivity) activity, registration::remove);
       } else {
-        onActivityStopCallOnce(activity, this::remove);
+        onActivityStopCallOnce(activity, registration::remove);
       }
     }
-  }
-
-  @Override
-  public void remove() {
-    asyncEventListener.mute();
-    client.stopListening(queryListener);
+    return registration;
   }
 }

firebase-firestore/src/main/java/com/google/firebase/firestore/util/ExecutorEventListener.java renamed to firebase-firestore/src/main/java/com/google/firebase/firestore/core/AsyncEventListener.java

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package com.google.firebase.firestore.util;
+package com.google.firebase.firestore.core;
 
 import com.google.firebase.firestore.EventListener;
 import com.google.firebase.firestore.FirebaseFirestoreException;
@@ -23,13 +23,13 @@
  * A wrapper event listener that uses an Executor to dispatch events. Exposes a mute() call to
  * immediately silence the event listener when events are dispatched on different threads.
  */
-public class ExecutorEventListener<T> implements EventListener<T> {
+public class AsyncEventListener<T> implements EventListener<T> {
   private final Executor executor;
   private final EventListener<T> eventListener;
 
   private volatile boolean muted = false;
 
-  public ExecutorEventListener(Executor executor, EventListener<T> eventListener) {
+  public AsyncEventListener(Executor executor, EventListener<T> eventListener) {
     this.executor = executor;
     this.eventListener = eventListener;
   }

firebase-firestore/src/main/java/com/google/firebase/firestore/core/ListenerRegistrationImpl.java

@@ -0,0 +1,44 @@
+// Copyright 2018 Google LLC
+//
+// 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 com.google.firebase.firestore.core;
+
+import com.google.firebase.firestore.ListenerRegistration;
+
+/** Implements the ListenerRegistration interface by removing a query from the listener. */
+public class ListenerRegistrationImpl implements ListenerRegistration {
+
+  private final FirestoreClient client;
+
+  /** The internal query listener object that is used to unlisten from the query. */
+  private final QueryListener queryListener;
+
+  /** The event listener for the query that raises events asynchronously. */
+  private final AsyncEventListener<ViewSnapshot> asyncEventListener;
+
+  public ListenerRegistrationImpl(
+      FirestoreClient client,
+      QueryListener queryListener,
+      AsyncEventListener<ViewSnapshot> asyncEventListener) {
+    this.client = client;
+    this.queryListener = queryListener;
+    this.asyncEventListener = asyncEventListener;
+  }
+
+  @Override
+  public void remove() {
+    asyncEventListener.mute();
+    client.stopListening(queryListener);
+  }
+}