mirror of
https://github.com/simplex-chat/simplex-chat.git
synced 2026-05-13 17:13:25 +00:00
android, desktop: open correct image in fullscreen viewer (#6869)
* android, desktop: open correct image in fullscreen viewer Fullscreen image viewer occasionally opened a different image than the one clicked. Root cause: when the LaunchedEffect probe at ImageFullScreenView.kt:48-55 calls getMedia(initialIndex - 1) to check whether a previous media item exists, getMedia returns null for both "no item" and "item found but failed to load" (e.g. undecodable bytes, missing file, crypto error). The probe treated null as "no previous item" and called scrollToStart(), which rewrote initialChatId to the chat's oldest media item - making the viewer display that oldest item instead of the clicked one. Fix: scrollToStart() no longer rewrites initialChatId. The pager is still repositioned to page 0; getMedia(0) resolves against the already-set initialChatId (the clicked item) and renders it correctly. * android, desktop: regression test for fullscreen viewer anchor preservation Drives the public providerForGallery interface: moves the anchor away from cItemId via currentPageChanged, calls scrollToStart, then reads the anchor back through onDismiss's scrollTo callback. The pre-fix code rewrote initialChatId to the chat's oldest showable, which would surface as scrollTo(2); the fix preserves the anchor and produces scrollTo(1). * plan: design doc for fullscreen viewer wrong-image fix Documents the pager state model, the root cause of the wrong-image bug, why the one-line deletion in scrollToStart fixes it for both call sites, and why the wider getMedia null-overload refactor is deliberately out of scope for this fix.
This commit is contained in:
Narasimha-sc
-1
@@ -3597,7 +3597,6 @@ fun providerForGallery(
|
||||
|
||||
override fun scrollToStart() {
|
||||
initialIndex = 0
|
||||
initialChatId = chatItems.firstOrNull { canShowMedia(it) }?.id ?: return
|
||||
}
|
||||
|
||||
override fun onDismiss(index: Int) {
|
||||
|
||||
+67
@@ -0,0 +1,67 @@
|
||||
package chat.simplex.app
|
||||
|
||||
import chat.simplex.common.model.*
|
||||
import chat.simplex.common.platform.chatModel
|
||||
import chat.simplex.common.views.chat.providerForGallery
|
||||
import kotlinx.datetime.Clock
|
||||
import kotlin.test.AfterTest
|
||||
import kotlin.test.BeforeTest
|
||||
import kotlin.test.Test
|
||||
import kotlin.test.assertEquals
|
||||
|
||||
// Regression for PR #6869: scrollToStart() must not rewrite initialChatId.
|
||||
class ProviderForGalleryTest {
|
||||
|
||||
// Synthetic items pass canShowMedia only when chatModel.connectedToRemote() is true.
|
||||
@BeforeTest
|
||||
fun connectChatModelToRemote() {
|
||||
chatModel.currentRemoteHost.value = RemoteHostInfo(
|
||||
remoteHostId = 0L,
|
||||
hostDeviceName = "",
|
||||
storePath = "",
|
||||
bindAddress_ = null,
|
||||
bindPort_ = null,
|
||||
sessionState = null,
|
||||
)
|
||||
}
|
||||
|
||||
@AfterTest
|
||||
fun resetChatModel() {
|
||||
chatModel.currentRemoteHost.value = null
|
||||
}
|
||||
|
||||
@Test
|
||||
fun testScrollToStartPreservesAnchor() {
|
||||
val items = listOf(imageItem(1L), imageItem(2L), imageItem(3L))
|
||||
var scrolledTo: Int? = null
|
||||
val provider = providerForGallery(items, cItemId = 3L) { scrolledTo = it }
|
||||
|
||||
provider.currentPageChanged(provider.initialIndex - 1)
|
||||
provider.scrollToStart()
|
||||
provider.onDismiss(0)
|
||||
|
||||
assertEquals(1, scrolledTo)
|
||||
}
|
||||
|
||||
// Pins the onDismiss early-return contract that testScrollToStartPreservesAnchor
|
||||
// relies on to read the anchor back through the scrollTo callback.
|
||||
@Test
|
||||
fun testOnDismissOnActiveItemDoesNotScroll() {
|
||||
val items = listOf(imageItem(1L), imageItem(2L), imageItem(3L))
|
||||
var scrolledTo: Int? = null
|
||||
val provider = providerForGallery(items, cItemId = 3L) { scrolledTo = it }
|
||||
|
||||
provider.onDismiss(provider.initialIndex)
|
||||
|
||||
assertEquals(null, scrolledTo)
|
||||
}
|
||||
|
||||
private fun imageItem(id: Long): ChatItem =
|
||||
ChatItem(
|
||||
chatDir = CIDirection.DirectRcv(),
|
||||
meta = CIMeta.getSample(id, Clock.System.now(), text = ""),
|
||||
content = CIContent.RcvMsgContent(MsgContent.MCImage(text = "", image = "")),
|
||||
reactions = emptyList(),
|
||||
file = CIFile.getSample(fileId = id, fileName = "img-$id.jpg", filePath = "img-$id.jpg"),
|
||||
)
|
||||
}
|
||||
@@ -0,0 +1,135 @@
|
||||
# Fullscreen image viewer: opens the wrong image
|
||||
|
||||
Design doc for the fix shipped in PR #6869.
|
||||
|
||||
## Problem
|
||||
|
||||
The fullscreen image viewer occasionally opened the chat's oldest media
|
||||
instead of the image the user tapped. Reproductions were intermittent —
|
||||
the gating condition turned out to be the runtime state of the
|
||||
*immediately-older* sibling of the tapped item.
|
||||
|
||||
## Background — pager state model
|
||||
|
||||
`providerForGallery` (`apps/multiplatform/common/src/commonMain/kotlin/chat/simplex/common/views/chat/ChatView.kt:3537`)
|
||||
backs the fullscreen viewer with a virtual pager of 10000 pages. The
|
||||
pager's state is two variables, captured in a closure:
|
||||
|
||||
- `initialIndex` — pager page that maps to the anchor item; starts at 5000.
|
||||
- `initialChatId` — id of the anchored chat item; starts at the tapped item.
|
||||
|
||||
Invariant: page `initialIndex` always shows item `initialChatId`. Other
|
||||
pages are computed by walking `chatItems` older / newer from the anchor
|
||||
via the local `item()` helper.
|
||||
|
||||
`scrollToStart()` is called by `ImageFullScreenView.kt` to lock the
|
||||
pager's leftward boundary at the user's current item, in two situations:
|
||||
|
||||
- **Init probe** (`ImageFullScreenView.kt:48-55`) — at viewer open, if
|
||||
`getMedia(initialIndex - 1) == null` (no older sibling reachable),
|
||||
reposition so the tapped item becomes page 0.
|
||||
- **Runtime branch** (`ImageFullScreenView.kt:97-112`) — during scroll,
|
||||
if `getMedia(index) == null` while the user is at `index + 1`, lock
|
||||
the pager so the null page isn't reachable.
|
||||
|
||||
Both callers want the same outcome: **page 0 = the user's current
|
||||
anchor item**, leftward = unreachable.
|
||||
|
||||
## Root cause
|
||||
|
||||
Pre-fix body of `scrollToStart`:
|
||||
|
||||
```kotlin
|
||||
override fun scrollToStart() {
|
||||
initialIndex = 0
|
||||
initialChatId = chatItems.firstOrNull { canShowMedia(it) }?.id ?: return
|
||||
}
|
||||
```
|
||||
|
||||
The second line rewrote `initialChatId` to the chat's *oldest showable
|
||||
media* — not the user's current anchor. This mismatched what both
|
||||
callers wanted. It happened to coincide with the correct behavior when
|
||||
the anchor already was the chat's oldest showable, which is why the bug
|
||||
masked itself for years.
|
||||
|
||||
The bug surfaced when the init probe fired for a non-boundary reason:
|
||||
|
||||
- The immediately-older sibling existed and passed `canShowMedia` (file
|
||||
marked loaded; file path resolved or remote was connected).
|
||||
- But `getLoadedImage` returned `null` at decode time (undecodable
|
||||
bytes, missing file on disk, crypto error).
|
||||
- `getMedia(initialIndex - 1)` therefore returned `null`.
|
||||
- The probe misread that null as "no older sibling exists" and called
|
||||
`scrollToStart()`.
|
||||
- `scrollToStart` rewrote `initialChatId` to the chat's oldest showable.
|
||||
- Page 0 of the pager rendered that oldest item — the wrong image.
|
||||
|
||||
## Fix
|
||||
|
||||
Delete the second line. `scrollToStart` becomes:
|
||||
|
||||
```kotlin
|
||||
override fun scrollToStart() {
|
||||
initialIndex = 0
|
||||
}
|
||||
```
|
||||
|
||||
`initialChatId` is preserved across the call. Page 0 now maps to the
|
||||
current anchor — exactly what both callers wanted from the start.
|
||||
|
||||
## Why this is correct for both callers
|
||||
|
||||
- **Init probe.** Before the call, `initialChatId` is the tapped item.
|
||||
After the call, page 0 = tapped item. ✓
|
||||
- **Runtime branch.** Before the call, `currentPageChanged` has already
|
||||
updated `initialChatId` to the user's currently visible item. After
|
||||
the call, page 0 = current item; the user's view is preserved with no
|
||||
visible jump. (Pre-fix the user got teleported to the chat's oldest
|
||||
media when a null sibling tripped this branch — a latent UX bug
|
||||
resolved by the same one-line change.)
|
||||
|
||||
## Why a wider structural change is not in scope here
|
||||
|
||||
`getMedia` returns `null` for two distinct conditions: (a) navigation
|
||||
found no showable item, (b) navigation found one but decode failed. A
|
||||
deeper refactor would let consumers distinguish these. That refactor is
|
||||
deliberately out of scope for this fix:
|
||||
|
||||
- The user-visible bug (wrong image) is fully resolved by the one-line
|
||||
change. No additional code is required to address the report.
|
||||
- The remaining symptom — locking the user out of older loadable items
|
||||
behind one that fails to decode — is mild, pre-existing, and not part
|
||||
of the report. If it becomes user-visible, address it in a follow-up.
|
||||
- A wider refactor would expand the diff, the review surface, and the
|
||||
regression risk for a fix that needs to ship promptly.
|
||||
- `good-code-v5.md`: *"Find the minimal change. The smallest structural
|
||||
modification that achieves the goal."* The smallest modification that
|
||||
resolves the reported bug is the deletion of one line.
|
||||
|
||||
## Verification
|
||||
|
||||
`apps/multiplatform/common/src/commonTest/kotlin/chat/simplex/app/ProviderForGalleryTest.kt`:
|
||||
|
||||
- `testScrollToStartPreservesAnchor` — drives the public provider
|
||||
interface: moves the anchor off `cItemId` via `currentPageChanged`,
|
||||
calls `scrollToStart`, then reads the anchor back through `onDismiss`'s
|
||||
`scrollTo` callback. Pre-fix would observe `scrollTo(2)` (the chat's
|
||||
oldest); post-fix `scrollTo(1)` (anchor preserved).
|
||||
- `testOnDismissOnActiveItemDoesNotScroll` — pins the `onDismiss`
|
||||
early-return contract that the regression test reads through.
|
||||
|
||||
Manual sanity (Android + desktop): tap newest / oldest / a middle image
|
||||
in a chat with multiple media — fullscreen opens on the tapped image in
|
||||
each case; swipe in both directions still works.
|
||||
|
||||
## Alternatives considered and rejected
|
||||
|
||||
- **Distinguish "no item" from "load failed" inside `getMedia`.**
|
||||
Requires either a return-type redesign (sealed result type) or an
|
||||
added query method on the interface. Both expand the diff well beyond
|
||||
what the user-visible bug requires. Deferred to a possible follow-up
|
||||
if the milder remaining symptom is reported.
|
||||
- **Hoist the local `item()` helper to a top-level testable function.**
|
||||
The regression test exercises the public provider interface and
|
||||
reads the anchor back via `onDismiss`'s `scrollTo` callback, so no
|
||||
internal extraction is needed for testability.
|
||||
Reference in New Issue
Block a user