From 30e0a8040e77f2ce599ae27454e07cf6135a12ca Mon Sep 17 00:00:00 2001 From: Alex Hart Date: Tue, 25 Aug 2026 09:11:01 -0300 Subject: [PATCH] Attempt to squelch errant gallery refresh. --- .../mediapreview/MediaPreviewFragment.kt | 9 +- .../mediapreview/MediaPreviewViewModel.kt | 85 ++++++++------ .../mediapreview/MediaPreviewViewModelTest.kt | 104 ++++++++++++++++++ 3 files changed, 165 insertions(+), 33 deletions(-) create mode 100644 app/src/test/java/org/thoughtcrime/securesms/mediapreview/MediaPreviewViewModelTest.kt diff --git a/app/src/main/java/org/thoughtcrime/securesms/mediapreview/MediaPreviewFragment.kt b/app/src/main/java/org/thoughtcrime/securesms/mediapreview/MediaPreviewFragment.kt index e92c3d3edc..1e8afc7733 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/mediapreview/MediaPreviewFragment.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/mediapreview/MediaPreviewFragment.kt @@ -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) diff --git a/app/src/main/java/org/thoughtcrime/securesms/mediapreview/MediaPreviewViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/mediapreview/MediaPreviewViewModel.kt index 9e455e94d7..3b77d1af44 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/mediapreview/MediaPreviewViewModel.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/mediapreview/MediaPreviewViewModel.kt @@ -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, 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 = 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>, 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>, 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 } } diff --git a/app/src/test/java/org/thoughtcrime/securesms/mediapreview/MediaPreviewViewModelTest.kt b/app/src/test/java/org/thoughtcrime/securesms/mediapreview/MediaPreviewViewModelTest.kt new file mode 100644 index 0000000000..1327582567 --- /dev/null +++ b/app/src/test/java/org/thoughtcrime/securesms/mediapreview/MediaPreviewViewModelTest.kt @@ -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 { + 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 + } +}