Clean up conversation list data loading sequence.

- The Paging library was giving us empty paged lists when loading was invalidated, but only *sometimes*. This library, man. Fixed it by ignoring invalid lists, which you'd think the library would do for us... - Noticed we were doing a ton of list refreshes because of how we were listening to archive count. Switched from combine to switchMap. - Noticed that we could become double-subscribed to LiveDatas in the ConversationListFragment if you went to archived. Fixed by observing on the fragment's view lifecycle. Fixes #9803
signalapp · Jul 30, 2020 · e504ffa · e504ffa
1 parent 9c63b37
commit e504ffa
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 28 deletions.
diff --git a/app/src/main/java/org/thoughtcrime/securesms/conversation/ConversationDataSource.java b/app/src/main/java/org/thoughtcrime/securesms/conversation/ConversationDataSource.java
@@ -14,7 +14,6 @@
 import org.thoughtcrime.securesms.database.MmsSmsDatabase;
 import org.thoughtcrime.securesms.database.model.MessageRecord;
 import org.thoughtcrime.securesms.logging.Log;
-import org.thoughtcrime.securesms.util.Util;
 import org.thoughtcrime.securesms.util.concurrent.SignalExecutors;
 import org.thoughtcrime.securesms.util.paging.Invalidator;
 import org.thoughtcrime.securesms.util.paging.SizeFixResult;
@@ -83,9 +82,10 @@ public void loadInitial(@NonNull LoadInitialParams params, @NonNull LoadInitialC
                                               .toList();
 
       callback.onResult(items, params.requestedStartPosition, result.getTotal());
+      Log.d(TAG, "[Initial Load] " + (System.currentTimeMillis() - start) + " ms | thread: " + threadId + ", start: " + params.requestedStartPosition + ", requestedSize: " + params.requestedLoadSize + ", actualSize: " + result.getItems().size() + ", totalCount: " + result.getTotal());
+    } else {
+      Log.d(TAG, "[Initial Load] " + (System.currentTimeMillis() - start) + " ms | thread: " + threadId + ", start: " + params.requestedStartPosition + ", requestedSize: " + params.requestedLoadSize + ", totalCount: " + totalCount + " -- invalidated");
     }
-
-    Log.d(TAG, "[Initial Load] " + (System.currentTimeMillis() - start) + " ms | thread: " + threadId + ", start: " + params.requestedStartPosition + ", size: " + params.requestedLoadSize + (isInvalid() ? " -- invalidated" : ""));
   }
 
   @Override

diff --git a/...src/main/java/org/thoughtcrime/securesms/conversationlist/ConversationListDataSource.java b/...src/main/java/org/thoughtcrime/securesms/conversationlist/ConversationListDataSource.java
@@ -17,6 +17,7 @@
 import org.thoughtcrime.securesms.logging.Log;
 import org.thoughtcrime.securesms.recipients.Recipient;
 import org.thoughtcrime.securesms.util.ThrottledDebouncer;
+import org.thoughtcrime.securesms.util.Util;
 import org.thoughtcrime.securesms.util.concurrent.SignalExecutors;
 import org.thoughtcrime.securesms.util.paging.Invalidator;
 import org.thoughtcrime.securesms.util.paging.SizeFixResult;
@@ -84,11 +85,11 @@ public final void loadInitial(@NonNull LoadInitialParams params, @NonNull LoadIn
 
     if (!isInvalid()) {
       SizeFixResult<Conversation> result = SizeFixResult.ensureMultipleOfPageSize(conversations, params.requestedStartPosition, params.pageSize, totalCount);
-
       callback.onResult(result.getItems(), params.requestedStartPosition, result.getTotal());
+      Log.d(TAG, "[Initial Load] " + (System.currentTimeMillis() - start) + " ms | start: " + params.requestedStartPosition + ", requestedSize: " + params.requestedLoadSize + ", actualSize: " + result.getItems().size() + ", totalCount: " + result.getTotal() + ", class: " + getClass().getSimpleName());
+    } else {
+      Log.d(TAG, "[Initial Load] " + (System.currentTimeMillis() - start) + " ms | start: " + params.requestedStartPosition + ", requestedSize: " + params.requestedLoadSize + ", totalCount: " + totalCount + ", class: " + getClass().getSimpleName() + " -- invalidated");
     }
-
-    Log.d(TAG, "[Initial Load] " + (System.currentTimeMillis() - start) + " ms | start: " + params.requestedStartPosition + ", size: " + params.requestedLoadSize + ", totalCount: " + totalCount + ", class: " + getClass().getSimpleName() + (isInvalid() ? " -- invalidated" : ""));
   }
 
   @Override

diff --git a/app/src/main/java/org/thoughtcrime/securesms/conversationlist/ConversationListFragment.java b/app/src/main/java/org/thoughtcrime/securesms/conversationlist/ConversationListFragment.java
@@ -500,9 +500,9 @@ protected boolean isArchived() {
   private void initializeViewModel() {
     viewModel = ViewModelProviders.of(this, new ConversationListViewModel.Factory(isArchived())).get(ConversationListViewModel.class);
 
-    viewModel.getSearchResult().observe(this, this::onSearchResultChanged);
-    viewModel.getMegaphone().observe(this, this::onMegaphoneChanged);
-    viewModel.getConversationList().observe(this, this::onSubmitList);
+    viewModel.getSearchResult().observe(getViewLifecycleOwner(), this::onSearchResultChanged);
+    viewModel.getMegaphone().observe(getViewLifecycleOwner(), this::onMegaphoneChanged);
+    viewModel.getConversationList().observe(getViewLifecycleOwner(), this::onSubmitList);
 
     ProcessLifecycleOwner.get().getLifecycle().addObserver(new DefaultLifecycleObserver() {
       @Override
@@ -751,6 +751,7 @@ private void handleCreateConversation(long threadId, Recipient recipient, int di
 
   private void onSubmitList(@NonNull ConversationListViewModel.ConversationList conversationList) {
     if (conversationList.isEmpty()) {
+      Log.i(TAG, "Received an empty data set.");
       list.setVisibility(View.INVISIBLE);
       emptyState.setVisibility(View.VISIBLE);
       emptyImage.setImageResource(EMPTY_IMAGES[(int) (Math.random() * EMPTY_IMAGES.length)]);

diff --git a/app/src/main/java/org/thoughtcrime/securesms/conversationlist/ConversationListViewModel.java b/app/src/main/java/org/thoughtcrime/securesms/conversationlist/ConversationListViewModel.java
@@ -8,6 +8,7 @@
 import androidx.annotation.NonNull;
 import androidx.lifecycle.LiveData;
 import androidx.lifecycle.MutableLiveData;
+import androidx.lifecycle.Transformations;
 import androidx.lifecycle.ViewModel;
 import androidx.lifecycle.ViewModelProvider;
 import androidx.paging.DataSource;
@@ -19,6 +20,7 @@
 import org.thoughtcrime.securesms.database.DatabaseContentProviders;
 import org.thoughtcrime.securesms.database.DatabaseFactory;
 import org.thoughtcrime.securesms.dependencies.ApplicationDependencies;
+import org.thoughtcrime.securesms.logging.Log;
 import org.thoughtcrime.securesms.megaphone.Megaphone;
 import org.thoughtcrime.securesms.megaphone.MegaphoneRepository;
 import org.thoughtcrime.securesms.megaphone.Megaphones;
@@ -31,10 +33,11 @@
 
 class ConversationListViewModel extends ViewModel {
 
+  private static final String TAG = Log.tag(ConversationListViewModel.class);
+
   private final Application                   application;
   private final MutableLiveData<Megaphone>    megaphone;
   private final MutableLiveData<SearchResult> searchResult;
-  private final MutableLiveData<Integer>      archivedCount;
   private final LiveData<ConversationList>    conversationList;
   private final SearchRepository              searchRepository;
   private final MegaphoneRepository           megaphoneRepository;
@@ -48,7 +51,6 @@ private ConversationListViewModel(@NonNull Application application, @NonNull Sea
     this.application         = application;
     this.megaphone           = new MutableLiveData<>();
     this.searchResult        = new MutableLiveData<>();
-    this.archivedCount       = new MutableLiveData<>();
     this.searchRepository    = searchRepository;
     this.megaphoneRepository = ApplicationDependencies.getMegaphoneRepository();
     this.debouncer           = new Debouncer(300);
@@ -59,10 +61,6 @@ public void onChange(boolean selfChange) {
         if (!TextUtils.isEmpty(getLastQuery())) {
           searchRepository.query(getLastQuery(), searchResult::postValue);
         }
-
-        if (!isArchived) {
-          updateArchivedCount();
-        }
       }
     };
 
@@ -77,15 +75,27 @@ public void onChange(boolean selfChange) {
                                                                                                     .setInitialLoadKey(0)
                                                                                                     .build();
 
-    if (isArchived) {
-      this.archivedCount.setValue(0);
-    } else {
-      updateArchivedCount();
-    }
-
     application.getContentResolver().registerContentObserver(DatabaseContentProviders.ConversationList.CONTENT_URI, true, observer);
 
-    this.conversationList = LiveDataUtil.combineLatest(conversationList, this.archivedCount, ConversationList::new);
+    this.conversationList = Transformations.switchMap(conversationList, conversation -> {
+      if (conversation.getDataSource().isInvalid()) {
+        Log.w(TAG, "Received an invalid conversation list. Ignoring.");
+        return new MutableLiveData<>();
+      }
+
+      MutableLiveData<ConversationList> updated = new MutableLiveData<>();
+
+      if (isArchived) {
+        updated.postValue(new ConversationList(conversation, 0));
+      } else {
+        SignalExecutors.BOUNDED.execute(() -> {
+          int archiveCount = DatabaseFactory.getThreadDatabase(application).getArchivedConversationListCount();
+          updated.postValue(new ConversationList(conversation, archiveCount));
+        });
+      }
+
+      return updated;
+    });
   }
 
   @NonNull LiveData<SearchResult> getSearchResult() {
@@ -140,12 +150,6 @@ protected void onCleared() {
     application.getContentResolver().unregisterContentObserver(observer);
   }
 
-  private void updateArchivedCount() {
-    SignalExecutors.BOUNDED.execute(() -> {
-      archivedCount.postValue(DatabaseFactory.getThreadDatabase(application).getArchivedConversationListCount());
-    });
-  }
-
   public static class Factory extends ViewModelProvider.NewInstanceFactory {
 
     private final boolean isArchived;