Attempt to squelch errant gallery refresh.

2026-09-20 16:54:28 +01:00 · 2026-08-25 09:11:01 -03:00
parent 499b42135a
commit 30e0a8040e
3 changed files with 165 additions and 33 deletions
@@ -96,6 +96,7 @@ class MediaPreviewFragment :
    requireActivity()
  })
  private val debouncer = Debouncer(2, TimeUnit.SECONDS)
+  private val refetchDebouncer = Debouncer(REFETCH_DEBOUNCE_MS)
  private val args: MediaIntentFactory.MediaPreviewArgs by lazy { MediaIntentFactory.requireArguments(requireArguments()) }

  private lateinit var pagerAdapter: MediaPreviewAdapter
@@ -165,7 +166,11 @@ class MediaPreviewFragment :
    val appContext = requireContext().applicationContext
    viewModel.fetchInitialAttachment(args)
    viewModel.fetchAttachments(appContext, startingAttachmentId, threadId, sorting)
-    val dbObserver = DatabaseObserver.Observer { viewModel.refetchAttachments(appContext, startingAttachmentId, threadId, sorting) }
+    // A single attachment's insert/upload/archive lifecycle notifies many times over a couple of seconds, and each
+    // notification would otherwise re-run the whole gallery query.
+    val dbObserver = DatabaseObserver.Observer {
+      refetchDebouncer.publish { viewModel.refetchAttachments(appContext, startingAttachmentId, threadId, sorting) }
+    }
    AppDependencies.databaseObserver.registerAttachmentUpdatedObserver(dbObserver)
    this.dbChangeObserver = dbObserver
  }
@@ -616,6 +621,7 @@ class MediaPreviewFragment :

  override fun onDestroy() {
    super.onDestroy()
+    refetchDebouncer.clear()
    val observer = dbChangeObserver
    if (observer != null) {
      AppDependencies.databaseObserver.unregisterObserver(observer)
@@ -783,6 +789,7 @@ class MediaPreviewFragment :
  companion object {
    private const val EXPANDED_CAPTION_HEIGHT_FALLBACK_DP = 400
    private const val EXPANDED_CAPTION_HEIGHT_PERCENT: Float = 0.7F
+    private const val REFETCH_DEBOUNCE_MS = 250L

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

@@ -7,6 +7,7 @@ import android.content.Context
 import android.content.Intent
 import android.content.pm.PackageManager
 import android.net.Uri
+import androidx.annotation.VisibleForTesting
 import androidx.core.app.NotificationCompat
 import androidx.core.app.NotificationManagerCompat
 import androidx.core.content.ContextCompat
@@ -16,6 +17,7 @@ import io.reactivex.rxjava3.core.Completable
 import io.reactivex.rxjava3.core.Flowable
 import io.reactivex.rxjava3.core.Single
 import io.reactivex.rxjava3.disposables.CompositeDisposable
+import io.reactivex.rxjava3.disposables.SerialDisposable
 import io.reactivex.rxjava3.kotlin.plusAssign
 import io.reactivex.rxjava3.schedulers.Schedulers
 import org.signal.core.models.database.AttachmentId
@@ -40,10 +42,27 @@ class MediaPreviewViewModel : ViewModel() {

  companion object {
    private val TAG = Log.tag(MediaPreviewViewModel::class)
+
+    /**
+     * The attachment window is rebuilt underneath a user who may have paged away, or had items shift under them, since
+     * the query was dispatched. Resolving against [oldState] — which is the latest state, not the one the query was
+     * anchored to — keeps whatever is on screen on screen instead of snapping the pager back to a stale page.
+     *
+     * [queryPosition] is the position the query itself landed on, used when the previously-visible attachment is not in
+     * the new window, which includes the first load, where there is nothing on screen to preserve.
+     */
+    @VisibleForTesting
+    fun resolvePosition(oldState: MediaPreviewState, records: List<MediaTable.MediaRecord>, queryPosition: Int): Int {
+      val visibleAttachmentId = oldState.mediaRecords.getOrNull(oldState.position)?.attachment?.attachmentId ?: return queryPosition
+      return records.indexOfFirst { it.attachment?.attachmentId == visibleAttachmentId }.takeIf { it >= 0 } ?: queryPosition
+    }
  }

  private val store = RxStore(MediaPreviewState())
  private val disposables = CompositeDisposable()
+
+  /** Only the newest refetch is worth keeping, so starting one cancels the last and stale windows never land. */
+  private val refetchDisposable = SerialDisposable().also { disposables += it }
  private val repository: MediaPreviewRepository = MediaPreviewRepository()

  val state: Flowable<MediaPreviewState> = store.stateFlowable.observeOn(AndroidSchedulers.mainThread())
@@ -109,42 +128,44 @@ class MediaPreviewViewModel : ViewModel() {
  }

  fun fetchAttachments(context: Context, startingAttachmentId: AttachmentId, threadId: Long, sorting: MediaTable.Sorting, forceRefresh: Boolean = false) {
-    if (store.state.loadState == MediaPreviewState.LoadState.INIT || forceRefresh) {
-      disposables += repository.getAttachments(context, startingAttachmentId, threadId, sorting).subscribe { result ->
-        store.update { oldState ->
-          val albums = result.records.fold(mutableMapOf()) { acc: MutableMap<Long, MutableList<Media>>, mediaRecord: MediaTable.MediaRecord ->
-            val attachment = mediaRecord.attachment
-            if (attachment != null) {
-              val convertedMedia = mediaRecord.toMedia() ?: return@fold acc
-              acc.getOrPut(attachment.mmsId) { mutableListOf() }.add(convertedMedia)
-            }
-            acc
-          }
-          // Never downgrade a MEDIA_READY state: the initial attachment may already have finished decoding.
-          val loadState = if (oldState.loadState == MediaPreviewState.LoadState.MEDIA_READY) {
-            MediaPreviewState.LoadState.MEDIA_READY
-          } else {
-            MediaPreviewState.LoadState.DATA_LOADED
-          }
+    if (store.state.loadState != MediaPreviewState.LoadState.INIT && !forceRefresh) {
+      return
+    }

-          if (oldState.leftIsRecent) {
-            oldState.copy(
-              position = result.initialPosition,
-              mediaRecords = result.records,
-              albums = albums,
-              loadState = loadState
-            )
-          } else {
-            oldState.copy(
-              position = result.records.size - result.initialPosition - 1,
-              mediaRecords = result.records.reversed(),
-              albums = albums.mapValues { it.value.reversed() },
-              loadState = loadState
-            )
+    val subscription = repository.getAttachments(context, startingAttachmentId, threadId, sorting).subscribe { result ->
+      store.update { oldState ->
+        val albums = result.records.fold(mutableMapOf()) { acc: MutableMap<Long, MutableList<Media>>, mediaRecord: MediaTable.MediaRecord ->
+          val attachment = mediaRecord.attachment
+          if (attachment != null) {
+            val convertedMedia = mediaRecord.toMedia() ?: return@fold acc
+            acc.getOrPut(attachment.mmsId) { mutableListOf() }.add(convertedMedia)
          }
+          acc
        }
-        fetchMessageBodies(context, result.records)
+        // Never downgrade a MEDIA_READY state: the initial attachment may already have finished decoding.
+        val loadState = if (oldState.loadState == MediaPreviewState.LoadState.MEDIA_READY) {
+          MediaPreviewState.LoadState.MEDIA_READY
+        } else {
+          MediaPreviewState.LoadState.DATA_LOADED
+        }
+
+        val records = if (oldState.leftIsRecent) result.records else result.records.reversed()
+        val queryPosition = if (oldState.leftIsRecent) result.initialPosition else result.records.size - result.initialPosition - 1
+
+        oldState.copy(
+          position = resolvePosition(oldState, records, queryPosition),
+          mediaRecords = records,
+          albums = if (oldState.leftIsRecent) albums else albums.mapValues { it.value.reversed() },
+          loadState = loadState
+        )
      }
+      fetchMessageBodies(context, result.records)
+    }
+
+    if (forceRefresh) {
+      refetchDisposable.set(subscription)
+    } else {
+      disposables += subscription
    }
  }

@@ -0,0 +1,104 @@
+/*
+ * Copyright 2026 Signal Messenger, LLC
+ * SPDX-License-Identifier: AGPL-3.0-only
+ */
+
+package org.thoughtcrime.securesms.mediapreview
+
+import android.app.Application
+import assertk.assertThat
+import assertk.assertions.isEqualTo
+import io.mockk.every
+import io.mockk.mockk
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.robolectric.RobolectricTestRunner
+import org.robolectric.annotation.Config
+import org.signal.core.models.database.AttachmentId
+import org.thoughtcrime.securesms.database.FakeMessageRecords
+import org.thoughtcrime.securesms.database.MediaTable
+
+@RunWith(RobolectricTestRunner::class)
+@Config(manifest = Config.NONE, application = Application::class)
+class MediaPreviewViewModelTest {
+
+  @Test
+  fun `resolvePosition uses the query position on a first load, when nothing is on screen yet`() {
+    val position = MediaPreviewViewModel.resolvePosition(
+      oldState = MediaPreviewState(),
+      records = recordsFor(1, 2, 3),
+      queryPosition = 1
+    )
+
+    assertThat(position).isEqualTo(1)
+  }
+
+  @Test
+  fun `resolvePosition follows the visible attachment when newly sent media shifts the window`() {
+    val position = MediaPreviewViewModel.resolvePosition(
+      oldState = stateFor(visiblePosition = 1, attachmentIds = longArrayOf(1, 2, 3)),
+      records = recordsFor(4, 1, 2, 3),
+      queryPosition = 1
+    )
+
+    assertThat(position).isEqualTo(2)
+  }
+
+  @Test
+  fun `resolvePosition ignores a query position anchored to a page the user has already swiped past`() {
+    val position = MediaPreviewViewModel.resolvePosition(
+      oldState = stateFor(visiblePosition = 2, attachmentIds = longArrayOf(1, 2, 3)),
+      records = recordsFor(1, 2, 3),
+      queryPosition = 1
+    )
+
+    assertThat(position).isEqualTo(2)
+  }
+
+  @Test
+  fun `resolvePosition falls back to the query position when the visible attachment left the window`() {
+    val position = MediaPreviewViewModel.resolvePosition(
+      oldState = stateFor(visiblePosition = 1, attachmentIds = longArrayOf(1, 2, 3)),
+      records = recordsFor(1, 3),
+      queryPosition = 0
+    )
+
+    assertThat(position).isEqualTo(0)
+  }
+
+  @Test
+  fun `resolvePosition falls back to the query position when the visible position is out of bounds`() {
+    val position = MediaPreviewViewModel.resolvePosition(
+      oldState = stateFor(visiblePosition = 5, attachmentIds = longArrayOf(1, 2)),
+      records = recordsFor(1, 2),
+      queryPosition = 1
+    )
+
+    assertThat(position).isEqualTo(1)
+  }
+
+  @Test
+  fun `resolvePosition does not match a record without an attachment to a visible record without one`() {
+    val position = MediaPreviewViewModel.resolvePosition(
+      oldState = MediaPreviewState(mediaRecords = listOf(recordFor(null)), position = 0),
+      records = listOf(recordFor(null), recordFor(2)),
+      queryPosition = 1
+    )
+
+    assertThat(position).isEqualTo(1)
+  }
+
+  private fun stateFor(visiblePosition: Int, attachmentIds: LongArray): MediaPreviewState {
+    return MediaPreviewState(mediaRecords = recordsFor(*attachmentIds), position = visiblePosition)
+  }
+
+  private fun recordsFor(vararg attachmentIds: Long): List<MediaTable.MediaRecord> {
+    return attachmentIds.map { recordFor(it) }
+  }
+
+  private fun recordFor(attachmentId: Long?): MediaTable.MediaRecord {
+    val record: MediaTable.MediaRecord = mockk()
+    every { record.attachment } returns attachmentId?.let { FakeMessageRecords.buildDatabaseAttachment(attachmentId = AttachmentId(it)) }
+    return record
+  }
+}