Fix tons of bugs relating to null snapshot.values and improve perf for good citizens (#871)

Author: SUPERCILEX

app/src/main/java/com/firebase/uidemo/database/ChatIndexActivity.java

@@ -1,6 +1,5 @@
 package com.firebase.uidemo.database;
 
-import android.os.Bundle;
 import android.view.View;
 
 import com.firebase.ui.database.FirebaseIndexRecyclerAdapter;
@@ -13,12 +12,6 @@
 public class ChatIndexActivity extends ChatActivity {
     private DatabaseReference mChatIndicesRef;
 
-    @Override
-    protected void onCreate(Bundle savedInstanceState) {
-        super.onCreate(savedInstanceState);
-        mChatIndicesRef = FirebaseDatabase.getInstance().getReference().child("chatIndices");
-    }
-
     @Override
     public void onClick(View v) {
         String uid = FirebaseAuth.getInstance().getCurrentUser().getUid();
@@ -34,6 +27,11 @@ public void onClick(View v) {
 
     @Override
     protected FirebaseRecyclerAdapter<Chat, ChatHolder> getAdapter() {
+        mChatIndicesRef = FirebaseDatabase.getInstance()
+                .getReference()
+                .child("chatIndices")
+                .child(FirebaseAuth.getInstance().getCurrentUser().getUid());
+
         return new FirebaseIndexRecyclerAdapter<Chat, ChatHolder>(
                 Chat.class,
                 R.layout.message,

database/src/main/java/com/firebase/ui/database/FirebaseArray.java

@@ -110,13 +110,11 @@ public void onChildMoved(DataSnapshot snapshot, String previousChildKey) {
         int oldIndex = getIndexForKey(snapshot.getKey());
         mSnapshots.remove(oldIndex);
 
-        int newIndex = previousChildKey == null ? 0 : (getIndexForKey(previousChildKey) + 1);
+        int newIndex = previousChildKey == null ? 0 : getIndexForKey(previousChildKey) + 1;
         mSnapshots.add(newIndex, snapshot);
 
-        notifyChangeEventListeners(ChangeEventListener.EventType.MOVED,
-                                   snapshot,
-                                   newIndex,
-                                   oldIndex);
+        notifyChangeEventListeners(
+                ChangeEventListener.EventType.MOVED, snapshot, newIndex, oldIndex);
     }
 
     @Override

database/src/main/java/com/firebase/ui/database/FirebaseIndexArray.java

@@ -140,18 +140,32 @@ protected List<DataSnapshot> getSnapshots() {
         return mDataSnapshots;
     }
 
-    private int getIndexForKey(String key) {
-        int dataCount = size();
-        int index = 0;
-        for (int keyIndex = 0; index < dataCount; keyIndex++) {
-            String superKey = mKeySnapshots.getObject(keyIndex);
-            if (key.equals(superKey)) {
-                break;
-            } else if (mDataSnapshots.get(index).getKey().equals(superKey)) {
-                index++;
+    private int returnOrFindIndexForKey(int index, String key) {
+        int realIndex;
+        if (isKeyAtIndex(key, index)) {
+            // To optimize this query, if the expected item position is accurate, we simply return
+            // it instead of searching for it in our keys all over again. This ensures developers
+            // correctly indexing their data (i.e. no null values) don't take a performance hit.
+            realIndex = index;
+        } else {
+            int dataCount = size();
+            int dataIndex = 0;
+            int keyIndex = 0;
+
+            while (dataIndex < dataCount && keyIndex < mKeySnapshots.size()) {
+                String superKey = mKeySnapshots.getObject(keyIndex);
+                if (key.equals(superKey)) {
+                    break;
+                } else if (mDataSnapshots.get(dataIndex).getKey().equals(superKey)) {
+                    // Only increment the data index if we aren't passing over a null value snapshot.
+                    dataIndex++;
+                }
+                keyIndex++;
             }
+
+            realIndex = dataIndex;
         }
-        return index;
+        return realIndex;
     }
 
     /**
@@ -173,11 +187,15 @@ protected void onKeyAdded(DataSnapshot data) {
     protected void onKeyMoved(DataSnapshot data, int index, int oldIndex) {
         String key = data.getKey();
 
+        // We can't use `returnOrFindIndexForKey(...)` for `oldIndex` or it might find the updated
+        // index instead of the old one. Unfortunately, this does mean move events will be
+        // incorrectly ignored if our list is a subset of the key list e.g. a key has null data.
         if (isKeyAtIndex(key, oldIndex)) {
             DataSnapshot snapshot = removeData(oldIndex);
+            int realIndex = returnOrFindIndexForKey(index, key);
             mHasPendingMoveOrDelete = true;
-            mDataSnapshots.add(index, snapshot);
-            notifyChangeEventListeners(EventType.MOVED, snapshot, index, oldIndex);
+            mDataSnapshots.add(realIndex, snapshot);
+            notifyChangeEventListeners(EventType.MOVED, snapshot, realIndex, oldIndex);
         }
     }
 
@@ -186,10 +204,11 @@ protected void onKeyRemoved(DataSnapshot data, int index) {
         ValueEventListener listener = mRefs.remove(mDataRef.getRef().child(key));
         if (listener != null) mDataRef.child(key).removeEventListener(listener);
 
-        if (isKeyAtIndex(key, index)) {
-            DataSnapshot snapshot = removeData(index);
+        int realIndex = returnOrFindIndexForKey(index, key);
+        if (isKeyAtIndex(key, realIndex)) {
+            DataSnapshot snapshot = removeData(realIndex);
             mHasPendingMoveOrDelete = true;
-            notifyChangeEventListeners(EventType.REMOVED, snapshot, index);
+            notifyChangeEventListeners(EventType.REMOVED, snapshot, realIndex);
         }
     }
 
@@ -225,10 +244,13 @@ public String toString() {
      * A ValueEventListener attached to the joined child data.
      */
     protected class DataRefListener implements ValueEventListener {
+        /** Cached index to skip searching for the current index on each update */
+        private int currentIndex;
+
         @Override
         public void onDataChange(DataSnapshot snapshot) {
             String key = snapshot.getKey();
-            int index = getIndexForKey(key);
+            int index = currentIndex = returnOrFindIndexForKey(currentIndex, key);
 
             if (snapshot.getValue() != null) {
                 if (isKeyAtIndex(key, index)) {