Fix cursor crash in ConversationSettings.

Best way to fix a cursor crash it to... stop using cursors. Fairly confident the crash was caused by us closing the cursor while it was read. And there just isn't a good way to avoid that with how it was written. So this ended up being a great excuse to move over to models.
2025-12-24 04:58:45 +00:00 · 2023-11-02 11:57:58 -04:00
parent b5c1051506
commit 99d0ee6725
7 changed files with 52 additions and 56 deletions
--- a/app/src/main/java/org/thoughtcrime/securesms/components/ThreadPhotoRailView.java
+++ b/app/src/main/java/org/thoughtcrime/securesms/components/ThreadPhotoRailView.java
@@ -17,13 +17,15 @@ import androidx.recyclerview.widget.RecyclerView;

 import org.signal.core.util.logging.Log;
 import org.thoughtcrime.securesms.R;
-import org.thoughtcrime.securesms.database.CursorRecyclerViewAdapter;
 import org.thoughtcrime.securesms.database.MediaTable;
 import org.thoughtcrime.securesms.mediapreview.MediaPreviewCache;
 import org.thoughtcrime.securesms.mms.GlideRequests;
 import org.thoughtcrime.securesms.mms.Slide;
 import org.thoughtcrime.securesms.util.MediaUtil;

+import java.util.ArrayList;
+import java.util.List;
+
 public class ThreadPhotoRailView extends FrameLayout {

  @NonNull  private final RecyclerView          recyclerView;
@@ -56,11 +58,11 @@ public class ThreadPhotoRailView extends FrameLayout {
    }
  }

-  public void setCursor(@NonNull GlideRequests glideRequests, @Nullable Cursor cursor) {
-    this.recyclerView.setAdapter(new ThreadPhotoRailAdapter(getContext(), glideRequests, cursor, this.listener));
+  public void setMediaRecords(@NonNull GlideRequests glideRequests, @NonNull List<MediaTable.MediaRecord> mediaRecords) {
+    this.recyclerView.setAdapter(new ThreadPhotoRailAdapter(getContext(), glideRequests, mediaRecords, this.listener));
  }

-  private static class ThreadPhotoRailAdapter extends CursorRecyclerViewAdapter<ThreadPhotoRailAdapter.ThreadPhotoViewHolder> {
+  private static class ThreadPhotoRailAdapter extends RecyclerView.Adapter<ThreadPhotoRailAdapter.ThreadPhotoViewHolder> {

    @SuppressWarnings("unused")
    private static final String TAG = Log.tag(ThreadPhotoRailAdapter.class);
@@ -69,18 +71,27 @@ public class ThreadPhotoRailView extends FrameLayout {

    @Nullable private OnItemClickedListener clickedListener;

+    private final List<MediaTable.MediaRecord> mediaRecords = new ArrayList<>();
+
    private ThreadPhotoRailAdapter(@NonNull Context context,
                                   @NonNull GlideRequests glideRequests,
-                                   @Nullable Cursor cursor,
+                                   @NonNull List<MediaTable.MediaRecord> mediaRecords,
                                   @Nullable OnItemClickedListener listener)
    {
-      super(context, cursor);
      this.glideRequests   = glideRequests;
      this.clickedListener = listener;
+
+      this.mediaRecords.clear();
+      this.mediaRecords.addAll(mediaRecords);
    }

    @Override
-    public ThreadPhotoViewHolder onCreateItemViewHolder(ViewGroup parent, int viewType) {
+    public int getItemCount() {
+      return mediaRecords.size();
+    }
+
+    @Override
+    public @NonNull ThreadPhotoViewHolder onCreateViewHolder(@NonNull ViewGroup parent, int viewType) {
      View itemView = LayoutInflater.from(parent.getContext())
                                    .inflate(R.layout.recipient_preference_photo_rail_item, parent, false);

@@ -88,18 +99,14 @@ public class ThreadPhotoRailView extends FrameLayout {
    }

    @Override
-    public void onBindItemViewHolder(ThreadPhotoViewHolder viewHolder, @NonNull Cursor cursor) {
-      ThumbnailView          imageView   = viewHolder.imageView;
-      MediaTable.MediaRecord mediaRecord = MediaTable.MediaRecord.from(cursor);
+    public void onBindViewHolder(@NonNull ThreadPhotoViewHolder viewHolder, int position) {
+      MediaTable.MediaRecord mediaRecord = mediaRecords.get(position);
      Slide                  slide       = MediaUtil.getSlideForAttachment(mediaRecord.getAttachment());

-      if (slide != null) {
-        imageView.setImageResource(glideRequests, slide, false, false);
-      }
-
-      imageView.setOnClickListener(v -> {
-        MediaPreviewCache.INSTANCE.setDrawable(imageView.getImageDrawable());
-        if (clickedListener != null) clickedListener.onItemClicked(imageView, mediaRecord);
+      viewHolder.imageView.setImageResource(glideRequests, slide, false, false);
+      viewHolder.imageView.setOnClickListener(v -> {
+        MediaPreviewCache.INSTANCE.setDrawable(viewHolder.imageView.getImageDrawable());
+        if (clickedListener != null) clickedListener.onItemClicked(viewHolder.imageView, mediaRecord);
      });
    }

--- a/app/src/main/java/org/thoughtcrime/securesms/components/settings/conversation/ConversationSettingsFragment.kt
+++ b/app/src/main/java/org/thoughtcrime/securesms/components/settings/conversation/ConversationSettingsFragment.kt
@@ -555,7 +555,7 @@ class ConversationSettingsFragment : DSLSettingsFragment(
        }
      }

-      if (state.sharedMedia != null && state.sharedMedia.count > 0) {
+      if (state.sharedMedia.isNotEmpty()) {
        dividerPref()

        sectionHeaderPref(R.string.recipient_preference_activity__shared_media)
@@ -563,7 +563,7 @@ class ConversationSettingsFragment : DSLSettingsFragment(
        @Suppress("DEPRECATION")
        customPref(
          SharedMediaPreference.Model(
-            mediaCursor = state.sharedMedia,
+            mediaRecords = state.sharedMedia,
            mediaIds = state.sharedMediaIds,
            onMediaRecordClick = { view, mediaRecord, isLtr ->
              view.transitionName = "thumb"
--- a/app/src/main/java/org/thoughtcrime/securesms/components/settings/conversation/ConversationSettingsRepository.kt
+++ b/app/src/main/java/org/thoughtcrime/securesms/components/settings/conversation/ConversationSettingsRepository.kt
@@ -30,7 +30,6 @@ import org.thoughtcrime.securesms.recipients.RecipientId
 import org.thoughtcrime.securesms.recipients.RecipientUtil
 import org.thoughtcrime.securesms.util.FeatureFlags
 import java.io.IOException
-import java.util.Optional

 private val TAG = Log.tag(ConversationSettingsRepository::class.java)

@@ -56,11 +55,11 @@ class ConversationSettingsRepository(
  }

  @WorkerThread
-  fun getThreadMedia(threadId: Long): Optional<Cursor> {
-    return if (threadId <= 0) {
-      Optional.empty()
+  fun getThreadMedia(threadId: Long, limit: Int): Cursor? {
+    return if (threadId > 0) {
+      SignalDatabase.media.getGalleryMediaForThread(threadId, MediaTable.Sorting.Newest, limit)
    } else {
-      Optional.of(SignalDatabase.media.getGalleryMediaForThread(threadId, MediaTable.Sorting.Newest))
+      null
    }
  }

--- a/app/src/main/java/org/thoughtcrime/securesms/components/settings/conversation/ConversationSettingsState.kt
+++ b/app/src/main/java/org/thoughtcrime/securesms/components/settings/conversation/ConversationSettingsState.kt
@@ -1,9 +1,9 @@
 package org.thoughtcrime.securesms.components.settings.conversation

-import android.database.Cursor
 import org.thoughtcrime.securesms.components.settings.conversation.preferences.ButtonStripPreference
 import org.thoughtcrime.securesms.components.settings.conversation.preferences.CallPreference
 import org.thoughtcrime.securesms.components.settings.conversation.preferences.LegacyGroupPreference
+import org.thoughtcrime.securesms.database.MediaTable
 import org.thoughtcrime.securesms.database.model.IdentityRecord
 import org.thoughtcrime.securesms.database.model.StoryViewState
 import org.thoughtcrime.securesms.groups.GroupId
@@ -18,7 +18,7 @@ data class ConversationSettingsState(
  val buttonStripState: ButtonStripPreference.State = ButtonStripPreference.State(),
  val disappearingMessagesLifespan: Int = 0,
  val canModifyBlockedState: Boolean = false,
-  val sharedMedia: Cursor? = null,
+  val sharedMedia: List<MediaTable.MediaRecord> = emptyList(),
  val sharedMediaIds: List<Long> = listOf(),
  val displayInternalRecipientDetails: Boolean = false,
  val calls: List<CallPreference.Model> = emptyList(),
--- a/app/src/main/java/org/thoughtcrime/securesms/components/settings/conversation/ConversationSettingsViewModel.kt
+++ b/app/src/main/java/org/thoughtcrime/securesms/components/settings/conversation/ConversationSettingsViewModel.kt
@@ -13,13 +13,13 @@ import io.reactivex.rxjava3.disposables.CompositeDisposable
 import io.reactivex.rxjava3.kotlin.plusAssign
 import io.reactivex.rxjava3.subjects.PublishSubject
 import io.reactivex.rxjava3.subjects.Subject
-import org.signal.core.util.CursorUtil
 import org.signal.core.util.ThreadUtil
 import org.signal.core.util.concurrent.SignalExecutors
+import org.signal.core.util.readToList
 import org.thoughtcrime.securesms.components.settings.conversation.preferences.ButtonStripPreference
 import org.thoughtcrime.securesms.components.settings.conversation.preferences.CallPreference
 import org.thoughtcrime.securesms.components.settings.conversation.preferences.LegacyGroupPreference
-import org.thoughtcrime.securesms.database.AttachmentTable
+import org.thoughtcrime.securesms.database.MediaTable
 import org.thoughtcrime.securesms.database.RecipientTable
 import org.thoughtcrime.securesms.database.model.StoryViewState
 import org.thoughtcrime.securesms.dependencies.ApplicationDependencies
@@ -34,7 +34,6 @@ import org.thoughtcrime.securesms.util.FeatureFlags
 import org.thoughtcrime.securesms.util.TextSecurePreferences
 import org.thoughtcrime.securesms.util.livedata.LiveDataUtil
 import org.thoughtcrime.securesms.util.livedata.Store
-import java.util.Optional

 sealed class ConversationSettingsViewModel(
  private val callMessageIds: LongArray,
@@ -42,8 +41,6 @@ sealed class ConversationSettingsViewModel(
  specificSettingsState: SpecificSettingsState
 ) : ViewModel() {

-  private val openedMediaCursors = HashSet<Cursor>()
-
  @Volatile
  private var cleared = false

@@ -66,37 +63,26 @@ sealed class ConversationSettingsViewModel(
    val threadId: LiveData<Long> = state.map { it.threadId }.distinctUntilChanged()
    val updater: LiveData<Long> = LiveDataUtil.combineLatest(threadId, sharedMediaUpdateTrigger) { tId, _ -> tId }

-    val sharedMedia: LiveData<Optional<Cursor>> = LiveDataUtil.mapAsync(SignalExecutors.BOUNDED, updater) { tId ->
-      repository.getThreadMedia(tId)
+    val sharedMedia: LiveData<List<MediaTable.MediaRecord>> = LiveDataUtil.mapAsync(SignalExecutors.BOUNDED, updater) { tId ->
+      repository.getThreadMedia(threadId = tId, limit = 100)?.readToList { cursor ->
+        MediaTable.MediaRecord.from(cursor)
+      } ?: emptyList()
    }

    store.update(repository.getCallEvents(callMessageIds).toObservable()) { callRecords, state ->
      state.copy(calls = callRecords.map { (call, messageRecord) -> CallPreference.Model(call, messageRecord) })
    }

-    store.update(sharedMedia) { cursor, state ->
+    store.update(sharedMedia) { mediaRecords, state ->
      if (!cleared) {
-        if (cursor.isPresent) {
-          openedMediaCursors.add(cursor.get())
-        }
-
-        val ids: List<Long> = cursor.map<List<Long>> {
-          val result = mutableListOf<Long>()
-          while (it.moveToNext()) {
-            result.add(CursorUtil.requireLong(it, AttachmentTable.ROW_ID))
-          }
-          result
-        }.orElse(listOf())
-
        state.copy(
-          sharedMedia = cursor.orElse(null),
-          sharedMediaIds = ids,
+          sharedMedia = mediaRecords,
+          sharedMediaIds = mediaRecords.mapNotNull { it.attachment?.attachmentId?.rowId },
          sharedMediaLoaded = true,
          displayInternalRecipientDetails = repository.isInternalRecipientDetailsEnabled()
        )
      } else {
-        cursor.orElse(null).ensureClosed()
-        state.copy(sharedMedia = null)
+        state.copy(sharedMedia = emptyList())
      }
    }
  }
@@ -123,7 +109,6 @@ sealed class ConversationSettingsViewModel(

  override fun onCleared() {
    cleared = true
-    openedMediaCursors.forEach { it.ensureClosed() }
    store.clear()
    disposable.clear()
  }
--- a/app/src/main/java/org/thoughtcrime/securesms/components/settings/conversation/preferences/SharedMediaPreference.kt
+++ b/app/src/main/java/org/thoughtcrime/securesms/components/settings/conversation/preferences/SharedMediaPreference.kt
@@ -1,6 +1,5 @@
 package org.thoughtcrime.securesms.components.settings.conversation.preferences

-import android.database.Cursor
 import android.view.View
 import org.thoughtcrime.securesms.R
 import org.thoughtcrime.securesms.components.ThreadPhotoRailView
@@ -22,7 +21,7 @@ object SharedMediaPreference {
  }

  class Model(
-    val mediaCursor: Cursor,
+    val mediaRecords: List<MediaTable.MediaRecord>,
    val mediaIds: List<Long>,
    val onMediaRecordClick: (View, MediaTable.MediaRecord, Boolean) -> Unit
  ) : PreferenceModel<Model>() {
@@ -41,7 +40,7 @@ object SharedMediaPreference {
    private val rail: ThreadPhotoRailView = itemView.findViewById(R.id.rail_view)

    override fun bind(model: Model) {
-      rail.setCursor(GlideApp.with(rail), model.mediaCursor)
+      rail.setMediaRecords(GlideApp.with(rail), model.mediaRecords)
      rail.setListener { v, m ->
        model.onMediaRecordClick(v, m, ViewUtil.isLtr(rail))
      }
--- a/app/src/main/java/org/thoughtcrime/securesms/database/MediaTable.kt
+++ b/app/src/main/java/org/thoughtcrime/securesms/database/MediaTable.kt
@@ -143,13 +143,19 @@ class MediaTable internal constructor(context: Context?, databaseHelper: SignalD
    }
  }

-  fun getGalleryMediaForThread(threadId: Long, sorting: Sorting): Cursor {
-    val query = if (FeatureFlags.instantVideoPlayback()) {
+  @JvmOverloads
+  fun getGalleryMediaForThread(threadId: Long, sorting: Sorting, limit: Int = 0): Cursor {
+    var query = if (FeatureFlags.instantVideoPlayback()) {
      sorting.applyToQuery(applyEqualityOperator(threadId, GALLERY_MEDIA_QUERY_INCLUDING_TEMP_VIDEOS))
    } else {
      sorting.applyToQuery(applyEqualityOperator(threadId, GALLERY_MEDIA_QUERY))
    }
    val args = arrayOf(threadId.toString() + "")
+
+    if (limit > 0) {
+      query = "$query LIMIT $limit"
+    }
+
    return readableDatabase.rawQuery(query, args)
  }