Hold a row's top edge during layout, so nothing is drawn out of place
The correction ran in a coroutine, so it landed a frame or more after the layout it was correcting: the wrong position was drawn once and then fixed, which reads as a flick and gets worse the faster the screen refreshes. That is a race with the display rather than a bug that can be tuned out, so the fix is not a shorter delay but a different phase. It now happens in the layout phase. `Modifier.holdTopEdge` learns the row's new height from the measurement that produced it and asks the list to shift by exactly that much, before anything is drawn. `requestScrollToItem` is the form that may be asked for during layout; `dispatchRawDelta` is not -- it calls forceRemeasure and dies with "performMeasureAndLayout called during measure layout", which cost one crash to establish. The arming flag and the per-row height are deliberately not snapshot state. Both are written from layout, where a snapshot write that composition reads would schedule another recomposition -- another frame, which is the thing being removed. This also drops the machinery the previous attempt needed: no waiting on a size change, no timeout, no marking the rows above to find one that could still report the move. A row measures itself, so a row that shrinks out of the viewport is no longer a special case. Verified with ui-trace sampling at ~1kHz, where a single bad frame would show as ten to twenty samples: expanding and collapsing from a heading are each one step from old position to held position with nothing in between, collapsing from the foot bar holds the four rows below it, a drag 120ms after a tap is left alone, and scrolling back stays put for six seconds. ktfmt, lint and 85 tests clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
irisandClaude Opus 5 committed
1 parent
cab21442d6
commit
5eba6ec529
2 files changed
+173
-172
No files matched your search
@@ -5,7 +5,6 @@ import androidx.activity.compose.rememberLauncherForActivityResult
|
||||
import androidx.activity.result.PickVisualMediaRequest
|
||||
import androidx.activity.result.contract.ActivityResultContracts
|
||||
import androidx.compose.foundation.Image
|
||||
import androidx.compose.foundation.gestures.scrollBy
|
||||
import androidx.compose.foundation.layout.Arrangement
|
||||
import androidx.compose.foundation.layout.Box
|
||||
import androidx.compose.foundation.layout.Column
|
||||
@@ -18,7 +17,6 @@ import androidx.compose.foundation.layout.height
|
||||
import androidx.compose.foundation.layout.padding
|
||||
import androidx.compose.foundation.layout.width
|
||||
import androidx.compose.foundation.lazy.LazyColumn
|
||||
import androidx.compose.foundation.lazy.LazyListItemInfo
|
||||
import androidx.compose.foundation.lazy.items
|
||||
import androidx.compose.foundation.lazy.rememberLazyListState
|
||||
import androidx.compose.foundation.shape.CircleShape
|
||||
@@ -49,6 +47,7 @@ import androidx.compose.runtime.setValue
|
||||
import androidx.compose.runtime.snapshotFlow
|
||||
import androidx.compose.ui.Alignment
|
||||
import androidx.compose.ui.Modifier
|
||||
import androidx.compose.ui.layout.onSizeChanged
|
||||
import androidx.compose.ui.platform.LocalContext
|
||||
import androidx.compose.ui.semantics.contentDescription
|
||||
import androidx.compose.ui.semantics.semantics
|
||||
@@ -63,10 +62,8 @@ import java.util.concurrent.atomic.AtomicReference
|
||||
import kotlinx.coroutines.Dispatchers
|
||||
import kotlinx.coroutines.awaitCancellation
|
||||
import kotlinx.coroutines.delay
|
||||
import kotlinx.coroutines.flow.first
|
||||
import kotlinx.coroutines.launch
|
||||
import kotlinx.coroutines.withContext
|
||||
import kotlinx.coroutines.withTimeoutOrNull
|
||||
|
||||
private const val RECONNECT_DELAY_MS = 1500L
|
||||
|
||||
@@ -84,13 +81,50 @@ private const val RECONNECT_DELAY_MS = 1500L
|
||||
private const val HISTORY_LOOKAHEAD = 8
|
||||
|
||||
/**
|
||||
* How long to keep holding a row's top edge while the row settles to its new height.
|
||||
* Which row was asked to hold its top edge, and how tall it was when it last measured.
|
||||
*
|
||||
* Long enough for the second and third layout passes a row can take to reach its final height, and
|
||||
* short enough that it is over before the reader could have started scrolling for their own
|
||||
* reasons. It always runs to the end: there is no way to ask whether more passes are coming.
|
||||
* Deliberately *not* snapshot state, and that is the point of the whole class. Both fields are
|
||||
* written from the layout phase; a snapshot write there that composition reads would schedule
|
||||
* another recomposition, and the correction has to land inside the frame that is already being laid
|
||||
* out rather than in a later one. Nothing observes these, so nothing needs to.
|
||||
*
|
||||
* [key] is cleared by the resize it was set for, so it cannot be spent on an unrelated one.
|
||||
*/
|
||||
private const val ANCHOR_SETTLE_MS = 500L
|
||||
private class TopEdgeHold {
|
||||
var key: Any? = null
|
||||
}
|
||||
|
||||
/** One row's height between layouts, so a change in it can be noticed. See [holdTopEdge]. */
|
||||
private class LastHeight {
|
||||
var value: Int? = null
|
||||
}
|
||||
|
||||
/**
|
||||
* Keeps this row's top edge where it is when the row changes height, if it was asked to.
|
||||
*
|
||||
* This runs in the *layout* phase, from the measurement that discovers the new height, and that is
|
||||
* the whole reason it is a modifier rather than an effect. A correction posted to a coroutine
|
||||
* arrives a frame or more after the layout it is correcting, so the wrong position is drawn once
|
||||
* before the right one -- visible as a flick, and worse the faster the screen refreshes. Scrolling
|
||||
* from here happens before anything is drawn, so there is no frame to see and nothing that depends
|
||||
* on how quickly the correction is scheduled.
|
||||
*
|
||||
* [hold] is given the change in height. The row's bottom edge is held by the list, so a scroll of
|
||||
* exactly that much is what leaves the top edge where it was.
|
||||
*/
|
||||
@Composable
|
||||
private fun Modifier.holdTopEdge(key: Any, held: TopEdgeHold, hold: (Int) -> Unit): Modifier {
|
||||
val last = remember { LastHeight() }
|
||||
return onSizeChanged { size ->
|
||||
val previous = last.value
|
||||
last.value = size.height
|
||||
// A first measurement has no previous height to have moved from, and a row that came
|
||||
// back after being scrolled away is a first measurement again.
|
||||
if (previous == null || previous == size.height || held.key != key) return@onSizeChanged
|
||||
held.key = null
|
||||
hold(size.height - previous)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* What the transcript renders: the event stream folded into displayable rows (see [foldEvent]). The
|
||||
@@ -490,6 +524,7 @@ fun SessionScreen(
|
||||
onSettings: () -> Unit,
|
||||
) {
|
||||
val scope = rememberCoroutineScope()
|
||||
val topEdgeHeld = remember { TopEdgeHold() }
|
||||
var items by remember { mutableStateOf(listOf<TranscriptItem>()) }
|
||||
var status by remember { mutableStateOf(summary.status) }
|
||||
// Seeded from the row this screen was opened from, so a conversation already under way says
|
||||
@@ -692,72 +727,13 @@ fun SessionScreen(
|
||||
* up off the screen and fills the space above it, so the calls appear on the far side of the
|
||||
* control that produced them.
|
||||
*
|
||||
* So [RowEdge.Bottom] is the list's own behaviour and does nothing extra, and [RowEdge.Top]
|
||||
* scrolls back by however much the row grew. It has to be measured rather than worked out: only
|
||||
* the layout knows how tall an open group is, and it depends on the calls in it.
|
||||
*
|
||||
* What is measured is the row's *size*, and that is the whole reason this works. The obvious
|
||||
* thing to measure is where its top edge went, and an earlier version did -- but a top edge
|
||||
* also moves when the reader scrolls, so a correction still waiting for the layout would
|
||||
* instead wake on the reader's own drag, read their scroll distance as the row's growth, and
|
||||
* undo it. That was reported as the transcript jumping on every expand and refusing to scroll
|
||||
* back at all. A size does not change when anybody scrolls, so the two cannot be confused.
|
||||
*
|
||||
* The row's bottom edge is held by the list, so scrolling by the growth is what puts the top
|
||||
* edge back, and it is done for every step of that growth until [ANCHOR_SETTLE_MS] is up.
|
||||
* So [RowEdge.Bottom] is the list's own behaviour and needs nothing, while [RowEdge.Top] marks
|
||||
* the row as holding its top edge the next time it is measured. The correction itself belongs
|
||||
* to the measurement -- see [holdTopEdge].
|
||||
*/
|
||||
fun toggleAnchored(key: Any, edge: RowEdge, toggle: () -> Unit) {
|
||||
fun rowOf() = listState.layoutInfo.visibleItemsInfo.firstOrNull { it.key == key }
|
||||
val row = rowOf()
|
||||
if (edge == RowEdge.Top) topEdgeHeld.key = key
|
||||
toggle()
|
||||
// Not on screen when it was pressed, so there is no edge of it to hold.
|
||||
if (edge == RowEdge.Bottom || row == null) return
|
||||
// Read out now, as numbers: the layout hands back a reused object per slot, so holding
|
||||
// one and reading it later describes whatever item took that slot since.
|
||||
val rowIndex = row.index
|
||||
val rowSize = row.size
|
||||
|
||||
// How far the row's top edge moved, read off any row that moved with it. The row and
|
||||
// everything above it shift by exactly the row's growth, so they all give the same
|
||||
// answer -- and several are recorded because which of them can answer varies. A row
|
||||
// that shrinks slides down behind its own anchored bottom edge, and a group taller
|
||||
// than the screen goes under the bottom of it entirely, leaving nothing of itself to
|
||||
// measure.
|
||||
fun edgeOf(item: LazyListItemInfo) =
|
||||
if (item.index == rowIndex) item.offset + item.size else item.offset
|
||||
val marks =
|
||||
listState.layoutInfo.visibleItemsInfo
|
||||
.filter { it.index >= rowIndex }
|
||||
.associate { it.key to edgeOf(it) }
|
||||
|
||||
scope.launch {
|
||||
// Waited on by *size*, which is the one thing a scroll cannot change. An earlier
|
||||
// version waited on the top edge instead, so a correction still pending would wake
|
||||
// on the reader's own drag, read their scroll distance as the row's growth and undo
|
||||
// it -- reported as the transcript jumping on every expand and refusing to scroll
|
||||
// back at all.
|
||||
//
|
||||
// The wait answers `true` rather than the size it saw, because the size it saw is
|
||||
// sometimes `null` -- that is the row leaving the screen, which is a real answer and
|
||||
// the one this has to handle. Returned straight out of `withTimeoutOrNull` it would
|
||||
// be the same value that means "never happened", and the case needing the correction
|
||||
// most was the case silently skipped.
|
||||
val settled =
|
||||
withTimeoutOrNull(ANCHOR_SETTLE_MS) {
|
||||
snapshotFlow { rowOf()?.size }.first { it != rowSize }
|
||||
true
|
||||
}
|
||||
if (settled == null) return@launch
|
||||
// Never against a finger already on the screen: the reader is placing the list
|
||||
// themselves, and that is also the only way a mark could have moved for a reason
|
||||
// other than the row resizing.
|
||||
if (listState.isScrollInProgress) return@launch
|
||||
val moved =
|
||||
listState.layoutInfo.visibleItemsInfo.firstNotNullOfOrNull { item ->
|
||||
marks[item.key]?.let { edgeOf(item) - it }
|
||||
} ?: return@launch
|
||||
listState.scrollBy(moved.toFloat())
|
||||
}
|
||||
}
|
||||
|
||||
// A compaction reports nothing about its own progress -- measured against the CLI, which
|
||||
@@ -1211,107 +1187,132 @@ fun SessionScreen(
|
||||
// at the same end. Paging older history is the opposite insertion and was
|
||||
// already fine, and stays fine, because a key survives both.
|
||||
items(rows.asReversed(), key = { it.key }) { row ->
|
||||
when (row) {
|
||||
is TranscriptRow.Tools ->
|
||||
ToolGroup(
|
||||
group = row,
|
||||
expanded = row.id in expandedGroups,
|
||||
onToggle = { edge ->
|
||||
toggleAnchored(row.key, edge) {
|
||||
expandedGroups =
|
||||
if (row.id in expandedGroups) expandedGroups - row.id
|
||||
else expandedGroups + row.id
|
||||
}
|
||||
},
|
||||
isToolExpanded = { it in expandedTools },
|
||||
// Anchored on the group, not the call: opening one call makes
|
||||
// the whole group taller, and the heading the reader is under
|
||||
// is the group's.
|
||||
onToolToggle = { id ->
|
||||
toggleAnchored(row.key, RowEdge.Top) {
|
||||
expandedTools =
|
||||
if (id in expandedTools) expandedTools - id
|
||||
else expandedTools + id
|
||||
}
|
||||
},
|
||||
onAnswer = { questionId, answers ->
|
||||
act {
|
||||
answerQuestion(settings, summary.id, questionId, answers)
|
||||
}
|
||||
},
|
||||
image = { ref -> SessionImage(settings, summary.id, ref) },
|
||||
Box(
|
||||
Modifier.holdTopEdge(row.key, topEdgeHeld) { grew ->
|
||||
// Requested rather than scrolled. Scrolling forces a remeasure,
|
||||
// and forcing one from inside a measure throws; this is the form
|
||||
// built to be asked for during layout and applied in that pass.
|
||||
listState.requestScrollToItem(
|
||||
listState.firstVisibleItemIndex,
|
||||
listState.firstVisibleItemScrollOffset + grew,
|
||||
)
|
||||
is TranscriptRow.Single ->
|
||||
when (val item = row.item) {
|
||||
is TranscriptItem.UserMsg ->
|
||||
UserBubble(
|
||||
settings = settings,
|
||||
sessionId = summary.id,
|
||||
text = item.text,
|
||||
images = item.images,
|
||||
)
|
||||
is TranscriptItem.AssistantMsg -> AssistantMessage(item.text)
|
||||
is TranscriptItem.ToolRun ->
|
||||
ToolCard(
|
||||
tool = item,
|
||||
expanded = item.id in expandedTools,
|
||||
onToggle = {
|
||||
toggleAnchored(row.key, RowEdge.Top) {
|
||||
expandedTools =
|
||||
if (item.id in expandedTools)
|
||||
expandedTools - item.id
|
||||
else expandedTools + item.id
|
||||
}
|
||||
},
|
||||
onAnswer = { questionId, answers ->
|
||||
}
|
||||
) {
|
||||
when (row) {
|
||||
is TranscriptRow.Tools ->
|
||||
ToolGroup(
|
||||
group = row,
|
||||
expanded = row.id in expandedGroups,
|
||||
onToggle = { edge ->
|
||||
toggleAnchored(row.key, edge) {
|
||||
expandedGroups =
|
||||
if (row.id in expandedGroups)
|
||||
expandedGroups - row.id
|
||||
else expandedGroups + row.id
|
||||
}
|
||||
},
|
||||
isToolExpanded = { it in expandedTools },
|
||||
// Anchored on the group, not the call: opening one call makes
|
||||
// the whole group taller, and the heading the reader is under
|
||||
// is the group's.
|
||||
onToolToggle = { id ->
|
||||
toggleAnchored(row.key, RowEdge.Top) {
|
||||
expandedTools =
|
||||
if (id in expandedTools) expandedTools - id
|
||||
else expandedTools + id
|
||||
}
|
||||
},
|
||||
onAnswer = { questionId, answers ->
|
||||
act {
|
||||
answerQuestion(
|
||||
settings,
|
||||
summary.id,
|
||||
questionId,
|
||||
answers,
|
||||
)
|
||||
}
|
||||
},
|
||||
image = { ref -> SessionImage(settings, summary.id, ref) },
|
||||
)
|
||||
is TranscriptRow.Single ->
|
||||
when (val item = row.item) {
|
||||
is TranscriptItem.UserMsg ->
|
||||
UserBubble(
|
||||
settings = settings,
|
||||
sessionId = summary.id,
|
||||
text = item.text,
|
||||
images = item.images,
|
||||
)
|
||||
is TranscriptItem.AssistantMsg -> AssistantMessage(item.text)
|
||||
is TranscriptItem.ToolRun ->
|
||||
ToolCard(
|
||||
tool = item,
|
||||
expanded = item.id in expandedTools,
|
||||
onToggle = {
|
||||
toggleAnchored(row.key, RowEdge.Top) {
|
||||
expandedTools =
|
||||
if (item.id in expandedTools)
|
||||
expandedTools - item.id
|
||||
else expandedTools + item.id
|
||||
}
|
||||
},
|
||||
onAnswer = { questionId, answers ->
|
||||
act {
|
||||
answerQuestion(
|
||||
settings,
|
||||
summary.id,
|
||||
questionId,
|
||||
answers,
|
||||
)
|
||||
}
|
||||
},
|
||||
image = { ref ->
|
||||
SessionImage(settings, summary.id, ref)
|
||||
},
|
||||
)
|
||||
is TranscriptItem.QuestionCard ->
|
||||
QuestionRow(item) { answers ->
|
||||
act {
|
||||
answerQuestion(
|
||||
settings,
|
||||
summary.id,
|
||||
questionId,
|
||||
item.id,
|
||||
answers,
|
||||
)
|
||||
}
|
||||
},
|
||||
image = { ref -> SessionImage(settings, summary.id, ref) },
|
||||
)
|
||||
is TranscriptItem.QuestionCard ->
|
||||
QuestionRow(item) { answers ->
|
||||
act {
|
||||
answerQuestion(settings, summary.id, item.id, answers)
|
||||
}
|
||||
}
|
||||
is TranscriptItem.ErrorMsg ->
|
||||
Text(
|
||||
item.message,
|
||||
color = MaterialTheme.colorScheme.error,
|
||||
style = MaterialTheme.typography.bodyMedium,
|
||||
)
|
||||
is TranscriptItem.ImageItem ->
|
||||
SessionImage(settings, summary.id, item.ref)
|
||||
is TranscriptItem.Note ->
|
||||
Text(
|
||||
item.text,
|
||||
style = MaterialTheme.typography.bodySmall,
|
||||
color = MaterialTheme.colorScheme.onSurfaceVariant,
|
||||
)
|
||||
is TranscriptItem.CommandRow -> CommandBubble(item.text)
|
||||
is TranscriptItem.ClearedNote -> ClearedRow()
|
||||
is TranscriptItem.CompactedNote -> CompactedRow(item)
|
||||
is TranscriptItem.PeerNote ->
|
||||
PeerMessageRow(
|
||||
item = item,
|
||||
expanded = item.seq in expandedNotes,
|
||||
onToggle = {
|
||||
toggleAnchored(row.key, RowEdge.Top) {
|
||||
expandedNotes =
|
||||
if (item.seq in expandedNotes)
|
||||
expandedNotes - item.seq
|
||||
else expandedNotes + item.seq
|
||||
}
|
||||
},
|
||||
)
|
||||
}
|
||||
is TranscriptItem.ErrorMsg ->
|
||||
Text(
|
||||
item.message,
|
||||
color = MaterialTheme.colorScheme.error,
|
||||
style = MaterialTheme.typography.bodyMedium,
|
||||
)
|
||||
is TranscriptItem.ImageItem ->
|
||||
SessionImage(settings, summary.id, item.ref)
|
||||
is TranscriptItem.Note ->
|
||||
Text(
|
||||
item.text,
|
||||
style = MaterialTheme.typography.bodySmall,
|
||||
color = MaterialTheme.colorScheme.onSurfaceVariant,
|
||||
)
|
||||
is TranscriptItem.CommandRow -> CommandBubble(item.text)
|
||||
is TranscriptItem.ClearedNote -> ClearedRow()
|
||||
is TranscriptItem.CompactedNote -> CompactedRow(item)
|
||||
is TranscriptItem.PeerNote ->
|
||||
PeerMessageRow(
|
||||
item = item,
|
||||
expanded = item.seq in expandedNotes,
|
||||
onToggle = {
|
||||
toggleAnchored(row.key, RowEdge.Top) {
|
||||
expandedNotes =
|
||||
if (item.seq in expandedNotes)
|
||||
expandedNotes - item.seq
|
||||
else expandedNotes + item.seq
|
||||
}
|
||||
},
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user