TASK-025 follow-up: fix diff-view horizontal-scroll jitter
Sharing one ScrollState across every row in the diff panels made each row's
layout pass overwrite the shared max-scroll bound with its own line width,
so the visible offset snapped back and forth mid-drag ("글자가 춤을 추네").
Replace it with a manually-driven offset clamped against one bound computed
once per file, instead of recomputed per row.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
8cf7acd9ac
commit
e2c25d8662
@ -1,17 +1,22 @@
|
||||
package com.ubisam.ubgitea.feature.history
|
||||
|
||||
import androidx.activity.compose.BackHandler
|
||||
import androidx.compose.foundation.gestures.Orientation
|
||||
import androidx.compose.foundation.gestures.ScrollableState
|
||||
import androidx.compose.foundation.gestures.rememberScrollableState
|
||||
import androidx.compose.foundation.gestures.scrollable
|
||||
import androidx.compose.foundation.horizontalScroll
|
||||
import androidx.compose.foundation.layout.Arrangement
|
||||
import androidx.compose.foundation.layout.Box
|
||||
import androidx.compose.foundation.layout.BoxWithConstraints
|
||||
import androidx.compose.foundation.layout.Column
|
||||
import androidx.compose.foundation.layout.ColumnScope
|
||||
import androidx.compose.foundation.layout.Row
|
||||
import androidx.compose.foundation.layout.fillMaxSize
|
||||
import androidx.compose.foundation.layout.fillMaxWidth
|
||||
import androidx.compose.foundation.layout.offset
|
||||
import androidx.compose.foundation.layout.padding
|
||||
import androidx.compose.foundation.clickable
|
||||
import androidx.compose.foundation.ScrollState
|
||||
import androidx.compose.foundation.rememberScrollState
|
||||
import androidx.compose.foundation.lazy.LazyColumn
|
||||
import androidx.compose.foundation.lazy.items
|
||||
@ -29,17 +34,23 @@ import androidx.compose.material3.Text
|
||||
import androidx.compose.material3.VerticalDivider
|
||||
import androidx.compose.runtime.Composable
|
||||
import androidx.compose.runtime.LaunchedEffect
|
||||
import androidx.compose.runtime.State
|
||||
import androidx.compose.runtime.getValue
|
||||
import androidx.compose.runtime.mutableFloatStateOf
|
||||
import androidx.compose.runtime.mutableStateOf
|
||||
import androidx.compose.runtime.remember
|
||||
import androidx.compose.runtime.saveable.rememberSaveable
|
||||
import androidx.compose.runtime.setValue
|
||||
import androidx.compose.runtime.snapshotFlow
|
||||
import androidx.compose.ui.Alignment
|
||||
import androidx.compose.ui.Modifier
|
||||
import androidx.compose.ui.draw.clipToBounds
|
||||
import androidx.compose.ui.graphics.Color
|
||||
import androidx.compose.ui.res.stringResource
|
||||
import androidx.compose.ui.semantics.Role
|
||||
import androidx.compose.ui.text.font.FontFamily
|
||||
import androidx.compose.ui.text.rememberTextMeasurer
|
||||
import androidx.compose.ui.unit.IntOffset
|
||||
import androidx.compose.ui.unit.dp
|
||||
import com.ubisam.ubgitea.R
|
||||
import com.ubisam.ubgitea.domain.history.CommitDetail
|
||||
@ -50,6 +61,7 @@ import com.ubisam.ubgitea.domain.history.FileChangeStatus
|
||||
import com.ubisam.ubgitea.domain.model.CommitSummary
|
||||
import com.ubisam.ubgitea.domain.model.RepositoryRef
|
||||
import com.ubisam.ubgitea.ui.formatInstant
|
||||
import kotlin.math.roundToInt
|
||||
|
||||
@Composable
|
||||
fun HistoryScreen(
|
||||
@ -250,10 +262,34 @@ private fun FileDiffView(state: HistoryUiState.SelectedFileDiff) {
|
||||
// keeps the two panels showing the same rows without needing per-line height math.
|
||||
val beforeState = rememberLazyListState()
|
||||
val afterState = rememberLazyListState()
|
||||
// One shared horizontal scroll position for every line in both panels: dragging any
|
||||
// line scrolls the whole diff sideways together, matching the synced vertical scroll,
|
||||
// instead of each line only revealing its own clipped text independently.
|
||||
val horizontalScrollState = rememberScrollState()
|
||||
// One shared horizontal scroll offset for every line in both panels, driven manually
|
||||
// instead of via Modifier.horizontalScroll per row: sharing a single ScrollState across
|
||||
// many differently-sized Text rows makes each row's layout pass overwrite the shared
|
||||
// max-scroll bound with its own width, so as the list recomposes different-length lines
|
||||
// the bound (and therefore the visible offset) kept getting stomped mid-drag, snapping
|
||||
// the text back and forth. Computing one fixed bound up front and applying a plain
|
||||
// Modifier.offset (clamped by hand) avoids that thrashing entirely.
|
||||
val diffLineStyle = MaterialTheme.typography.bodySmall.copy(fontFamily = FontFamily.Monospace)
|
||||
val textMeasurer = rememberTextMeasurer()
|
||||
val maxHorizontalOffsetPx = remember(state.rows, diffLineStyle) {
|
||||
val longestLineChars = state.rows.maxOfOrNull { row ->
|
||||
maxOf(row.left?.text?.length ?: 0, row.right?.text?.length ?: 0)
|
||||
} ?: 0
|
||||
if (longestLineChars == 0) {
|
||||
0f
|
||||
} else {
|
||||
// Monospace, so any character stands in for the widest line's actual glyphs;
|
||||
// +2 accounts for the "- "/"+ "/" " prefix DiffSideLine prepends below.
|
||||
textMeasurer.measure("0".repeat(longestLineChars + 2), diffLineStyle).size.width.toFloat()
|
||||
}
|
||||
}
|
||||
val horizontalOffsetPx = remember(state.rows) { mutableFloatStateOf(0f) }
|
||||
val horizontalScrollableState = rememberScrollableState { delta ->
|
||||
val newOffset = (horizontalOffsetPx.floatValue + delta).coerceIn(0f, maxHorizontalOffsetPx)
|
||||
val consumed = newOffset - horizontalOffsetPx.floatValue
|
||||
horizontalOffsetPx.floatValue = newOffset
|
||||
consumed
|
||||
}
|
||||
LaunchedEffect(beforeState, afterState) {
|
||||
snapshotFlow { beforeState.firstVisibleItemIndex to beforeState.firstVisibleItemScrollOffset }
|
||||
.collect { (index, offset) ->
|
||||
@ -277,7 +313,7 @@ private fun FileDiffView(state: HistoryUiState.SelectedFileDiff) {
|
||||
)
|
||||
LazyColumn(Modifier.weight(1f), state = beforeState) {
|
||||
items(state.rows.size) { index ->
|
||||
DiffSideLine(state.rows[index].left, state.rows[index].isGap, horizontalScrollState)
|
||||
DiffSideLine(state.rows[index].left, state.rows[index].isGap, horizontalScrollableState, horizontalOffsetPx)
|
||||
}
|
||||
}
|
||||
HorizontalDivider(thickness = 2.dp)
|
||||
@ -288,7 +324,7 @@ private fun FileDiffView(state: HistoryUiState.SelectedFileDiff) {
|
||||
)
|
||||
LazyColumn(Modifier.weight(1f), state = afterState) {
|
||||
items(state.rows.size) { index ->
|
||||
DiffSideLine(state.rows[index].right, state.rows[index].isGap, horizontalScrollState)
|
||||
DiffSideLine(state.rows[index].right, state.rows[index].isGap, horizontalScrollableState, horizontalOffsetPx)
|
||||
}
|
||||
}
|
||||
}
|
||||
@ -299,7 +335,12 @@ private val DiffRow.isGap: Boolean
|
||||
get() = left == null && right == null
|
||||
|
||||
@Composable
|
||||
private fun DiffSideLine(line: DiffLine?, isGap: Boolean, horizontalScrollState: ScrollState) {
|
||||
private fun DiffSideLine(
|
||||
line: DiffLine?,
|
||||
isGap: Boolean,
|
||||
scrollableState: ScrollableState,
|
||||
horizontalOffsetPx: State<Float>,
|
||||
) {
|
||||
if (isGap) {
|
||||
Text(
|
||||
"⋯",
|
||||
@ -318,15 +359,23 @@ private fun DiffSideLine(line: DiffLine?, isGap: Boolean, horizontalScrollState:
|
||||
is DiffLine.Added -> "+ "
|
||||
else -> " "
|
||||
}
|
||||
Box(
|
||||
Modifier.fillMaxWidth().clipToBounds()
|
||||
.scrollable(state = scrollableState, orientation = Orientation.Horizontal)
|
||||
.padding(vertical = 2.dp),
|
||||
) {
|
||||
Text(
|
||||
prefix + (line?.text ?: ""),
|
||||
fontFamily = FontFamily.Monospace,
|
||||
style = MaterialTheme.typography.bodySmall,
|
||||
color = color,
|
||||
softWrap = false,
|
||||
modifier = Modifier.horizontalScroll(horizontalScrollState).padding(horizontal = 16.dp, vertical = 2.dp),
|
||||
modifier = Modifier
|
||||
.offset { IntOffset(-horizontalOffsetPx.value.roundToInt(), 0) }
|
||||
.padding(horizontal = 16.dp),
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
// Material3 has no built-in "success" role to match colorScheme.error, so this is hardcoded.
|
||||
private val DiffAddedColor = Color(0xFF2E7D32)
|
||||
|
||||
@ -372,3 +372,17 @@ This is the append-only implementation and handoff record shared by Codex and Cl
|
||||
- Known limitations: Full device confirmation of this whole batch together (tabs, swipe, overlap fix, status-bar padding, diff scroll, back-button removal, pull-to-refresh) in one pass is still pending as of this commit — the individual fixes were each verified against their own specific report, but not yet re-confirmed together. Two items from the queued list are still not started: an image-file viewer in the code browser, and numbered pagination with direct page-entry for the Issues/PR list.
|
||||
- Next recommended task: Get the human product owner's live confirmation of this batch, then pick up the two remaining queued items (image viewer, Issues/PR numbered pagination).
|
||||
- Next owner: Claude, continuing per the standing direction.
|
||||
|
||||
## 2026-08-27 — TASK-025 follow-up: fix diff-view horizontal-scroll jitter
|
||||
|
||||
- Agent and role: Claude, same live testing session, responding to a screenshot-reported bug immediately after the TASK-025 batch above was installed.
|
||||
- Commit/diff: continued on `claude/task-015-notifications`; `feature/history/HistoryScreen.kt` only.
|
||||
- Bug: after TASK-025's first diff-scroll fix (removing `fillMaxWidth()`, adding `softWrap = false`), the human product owner reported that scrolling the diff view no longer failed to scroll, but instead made the text visibly jitter/snap ("코드에서 스크롤하니까 좌우 스크롤이 되는게 아니라 무슨 글자가 춤을 추네" — "the letters are dancing"), shown via screenshot.
|
||||
- Root cause: `DiffSideLine` had every row in both diff panels sharing one `ScrollState` via `Modifier.horizontalScroll(state)`. A `ScrollState`'s max-scroll bound is set from whichever composable using it was most recently measured; with many rows of different line lengths in the same `LazyColumn`, each row's own layout pass overwrote that shared bound with its own (usually shorter or longer) width, so as different rows got remeasured during scrolling the bound — and therefore the effective clamp on the current scroll position — kept changing out from under the drag, snapping the visible text back and forth.
|
||||
- Fix: replaced the shared `ScrollState` with a hand-driven offset: a single `MutableFloatState` updated by `rememberScrollableState { delta -> ... }` and clamped against one bound computed once per selected file (via `rememberTextMeasurer()`, measuring the longest line's character count against the diff's monospace style) instead of recomputed per row on every layout pass. Each row now uses `Modifier.scrollable(...)` purely for gesture capture and a plain `Modifier.offset { ... }` for rendering, wrapped in `Box(Modifier.fillMaxWidth().clipToBounds())` since manual offset — unlike `horizontalScroll` — doesn't clip overflow by itself.
|
||||
- Files changed: `feature/history/HistoryScreen.kt`, `tasks/025-branch-detail-tabs.md` (evidence updated in place rather than as a new task file, since this is a direct continuation of that task's item 6).
|
||||
- Checks and results: `./gradlew compileDebugKotlin` BUILD SUCCESSFUL; `./gradlew assembleDebug testDebugUnitTest lintDebug` BUILD SUCCESSFUL, all unit tests passing, 0 lint errors. Reinstalled on the human product owner's device over wireless `adb`.
|
||||
- Review findings and resolutions: None from a separate review pass — self-diagnosed from the live bug report and Compose's documented `ScrollState`/layout-bound semantics.
|
||||
- Known limitations: The upper scroll bound is measured once per file (via a repeated single-character string of the longest line's length), not re-measured if the panel is resized mid-session (e.g., device rotation) — not expected to matter in practice since a file's selected diff is re-entered fresh on any real layout change, but noted for completeness. Live confirmation of this specific fix (does the jitter actually stop) is pending as of this commit, same as the rest of the TASK-025 batch.
|
||||
- Next recommended task: Get the human product owner's live confirmation of the diff view specifically (in addition to the rest of the TASK-025 batch), then continue to the two remaining queued items.
|
||||
- Next owner: Claude, continuing per the standing direction.
|
||||
|
||||
@ -16,7 +16,7 @@ Plus six more follow-up fixes/changes found or requested during the same live-te
|
||||
|
||||
4. **Overlap bug fix**: the narrow-layout (non-Z-Fold-unfolded) branch of `CommitDetailScreen`'s `BoxWithConstraints` had `Header(...)` and its content as bare siblings instead of children of a `Column`, so `BoxWithConstraints` (which behaves like `Box` — it does not arrange multi-child branches vertically on its own) rendered them stacked on top of each other. Reported twice by the human product owner via screenshot before being root-caused; fixed by wrapping each branch's contents in `Column(Modifier.fillMaxSize()) { ... }`.
|
||||
5. **Status-bar padding fix**: `BranchDetailScreen`'s `TabRow` was jammed against the status bar because, unlike every other top-level screen in the app, it wasn't wrapped in its own `Scaffold`. Fixed with `Modifier.statusBarsPadding()` on the outer `Column` (not a nested `Scaffold`, to avoid double-padding since `CodeBrowserScreen`/`HistoryScreen` already have their own `Scaffold`s internally).
|
||||
6. **Diff horizontal-scroll fix**: `DiffSideLine` (in the History tab's file-diff view) had `Modifier.fillMaxWidth().horizontalScroll(state)` and no `softWrap = false`, so long lines wrapped instead of overflowing — nothing to scroll. Fixed by removing `fillMaxWidth()` and adding `softWrap = false`, matching the already-working `PatchText` composable's modifier pattern.
|
||||
6. **Diff horizontal-scroll fix, in two passes**: `DiffSideLine` (in the History tab's file-diff view) originally had `Modifier.fillMaxWidth().horizontalScroll(state)` and no `softWrap = false`, so long lines wrapped instead of overflowing — nothing to scroll. First pass fixed that by removing `fillMaxWidth()` and adding `softWrap = false`, matching the already-working `PatchText` composable's modifier pattern — but that pattern shares one `ScrollState` across every row in the panel (so dragging any line moves them all together), and a single `ScrollState`'s scroll bound is set from whichever row's layout pass ran most recently; with many differently-wide lines in the same list, that bound kept getting overwritten mid-drag, snapping the visible offset back and forth ("글자가 춤을 추네" — reported live, with a screenshot, after the first pass had already been installed). Second pass replaced the shared `ScrollState` with a hand-rolled scroll: one `Modifier.scrollable(...)` per row feeding a single shared offset value, clamped against one bound computed once up front (via `TextMeasurer`, from the longest line in the file) instead of recomputed per row, then applied to each row with a plain `Modifier.offset { ... }` — so no per-row layout pass can stomp on another row's bound anymore.
|
||||
7. **Loading-text simplification**: the references-loading message on the repository screen previously interpolated the repository name into a longer sentence; simplified to a plain "로딩 중" per direct request, and the now-unused `%1$s` placeholder removed from `repositories_references_loading`.
|
||||
8. **In-app back buttons removed**: every screen-header back button (`CodeBrowserScreen`'s `BrowserHeader`, `HistoryScreen`'s `Header`, `NotificationsScreen`'s `FilterTabs`, `WorkItemsScreen`'s `ModeTabs` and `DetailHeader`, `RepositoryListScreen`'s `ReferencesContent` header) removed, since every screen already has a `BackHandler` wired to the system back gesture and the human product owner asked to rely on that instead. `WorkItemsScreen`'s `DetailHeader` composable (which consisted only of a back button) removed entirely along with all 5 call sites. Now-unused `history_back`/`notifications_back`/`workitems_back`/`codebrowser_back` string resources removed; `repositories_back` kept, since it's a distinct recovery action on the references-error state's secondary button, not a header back button.
|
||||
9. **Pull-to-refresh replaces manual refresh buttons**: `RepositoryListScreen`'s `RepositoryContent` and `ReferencesContent` each had a manual "새로고침" `OutlinedButton`; both replaced with `PullToRefreshBox` (Material3, `@OptIn(ExperimentalMaterial3Api::class)`) wrapping their `LazyColumn`s, so pulling down from the top refreshes instead.
|
||||
@ -65,13 +65,14 @@ Plus six more follow-up fixes/changes found or requested during the same live-te
|
||||
- `RouteEncodingTest.kt` updated: `code browser route round-trips` and `history route round-trips` tests replaced by one `branch detail route round-trips including a ref containing a slash` test; the truncated-route and full-back-stack tests updated to match.
|
||||
- `HistoryScreen.kt`'s `CommitDetailScreen`: each `BoxWithConstraints` branch now wraps `Header(...)` + content in `Column(Modifier.fillMaxSize()) { ... }` instead of leaving them as bare siblings.
|
||||
- `BranchDetailScreen.kt`: outer `Column` gained `Modifier.statusBarsPadding()`.
|
||||
- `HistoryScreen.kt`'s `DiffSideLine`: `Modifier.horizontalScroll(horizontalScrollState).padding(...)` with `softWrap = false`, no `fillMaxWidth()`.
|
||||
- `HistoryScreen.kt`'s `DiffSideLine` (first pass, superseded — see below): `Modifier.horizontalScroll(horizontalScrollState).padding(...)` with `softWrap = false`, no `fillMaxWidth()`.
|
||||
- `HistoryScreen.kt`'s `DiffSideLine`/`FileDiffView` (second pass, current): shared `ScrollState` replaced by a manually-driven offset — `rememberScrollableState { delta -> ... }` updates one `MutableFloatState` clamped to `[0, maxHorizontalOffsetPx]`, where `maxHorizontalOffsetPx` is computed once per selected file via `rememberTextMeasurer().measure(...)` against the longest line's character count (monospace, so any character stands in for the widest line's glyph width). Each row applies `Modifier.scrollable(sharedScrollableState, Orientation.Horizontal)` for gesture capture and `Modifier.offset { IntOffset(-horizontalOffsetPx.value.roundToInt(), 0) }` for rendering, wrapped in `Box(Modifier.fillMaxWidth().clipToBounds())` since a plain `offset` (unlike `horizontalScroll`) doesn't clip overflow on its own.
|
||||
- `strings_repositories.xml`: `repositories_references_loading` simplified to a fixed "로딩 중" string (no `%1$s` placeholder); call site in `RepositoryListScreen.kt` updated to match.
|
||||
- Back buttons removed from `CodeBrowserScreen.BrowserHeader`, `HistoryScreen.Header`, `NotificationsScreen.FilterTabs`, `WorkItemsScreen.ModeTabs`; `WorkItemsScreen.DetailHeader` composable and its 5 call sites removed entirely; `ReferencesContent`'s header back button removed. `history_back`/`notifications_back`/`workitems_back`/`codebrowser_back` string resources removed as now-unused.
|
||||
- `RepositoryListScreen.kt`: `RepositoryContent`/`ReferencesContent` wrapped in `PullToRefreshBox` (`@OptIn(ExperimentalMaterial3Api::class)`), manual "새로고침" `OutlinedButton`s removed from both.
|
||||
- `./gradlew compileDebugKotlin`: BUILD SUCCESSFUL (verified after adding the `@OptIn(ExperimentalMaterial3Api::class)` fix for `PullToRefreshBox`).
|
||||
- `./gradlew assembleDebug testDebugUnitTest lintDebug --rerun-tasks`: BUILD SUCCESSFUL in 3m 22s, 53 actionable tasks executed, all unit tests passing, 0 lint errors.
|
||||
- Live device: build installed over wireless `adb` (`adb-RFCYA0RD1CZ-ztOzAA._adb-tls-connect._tcp`, Galaxy Z Fold); human product owner asked to confirm tab switching, folder browsing, overlap fix, status-bar padding, diff horizontal scroll, back-button removal, and pull-to-refresh together — result pending as of this commit.
|
||||
- `./gradlew compileDebugKotlin`: BUILD SUCCESSFUL, both after the `@OptIn(ExperimentalMaterial3Api::class)` fix for `PullToRefreshBox` and again after the diff-scroll rewrite.
|
||||
- `./gradlew assembleDebug testDebugUnitTest lintDebug` (`--rerun-tasks` on the first full pass, incremental on the diff-scroll-rewrite follow-up): BUILD SUCCESSFUL both times, all unit tests passing, 0 lint errors.
|
||||
- Live device: build installed over wireless `adb` (`adb-RFCYA0RD1CZ-ztOzAA._adb-tls-connect._tcp`, Galaxy Z Fold) three times across this task — initial batch, then again after the diff-scroll jitter report and fix; human product owner asked to confirm the diff view specifically no longer jitters, alongside the rest of the batch — result pending as of this commit.
|
||||
|
||||
## Review findings
|
||||
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user