diff --git a/TODO.md b/TODO.md index b62edb4..ab249fe 100644 --- a/TODO.md +++ b/TODO.md @@ -5,16 +5,29 @@ one in place when it turns out to need a decision. ## App — transcript -- [ ] Tapping an expanded agent message card should close it. — a tap that - clears a selection now spends itself on that and nothing else - (`SessionScreen.expanding`); still to check on the emulator whether a tap - with *no* selection reaches an opened peer card at all. -- [ ] Text inside code blocks does not highlight when selected (selection - itself works — only the highlight is missing). Waiting on the app-c7 - session, which is replacing the highlighter (`HIGHLIGHTER_PLAN.md`) and - owns `CodeFence.kt` until it pushes; it confirmed this item is ours. +- [ ] Text inside code blocks does not highlight when selected. **Measured, and + it does** — the selection is drawn, but over the near-black surface a code + block and a tool's output sit on, Material's default 40%-alpha tint + composites to a barely-there smudge, much weaker than the same selection + over a reply. The app now states its own selection colours + (`AiAppSelectionColors`), which took the fill from #5B4C73 to #776394 on + that surface. Worth confirming this was the complaint rather than a + selection that draws *nothing* on the phone. +- [ ] Text inside an opened peer message or memory note cannot be selected at + all — the heading of the same card can, and so can a tool call's output, + so it is the markdown text specifically. Pre-existing (measured against + the build before this session's changes, by stashing them). It + contradicts AGENTS.md's "all transcript text is selectable". - [ ] Messages received from other agents are inconsistent — sometimes they - appear, sometimes they don't. + appear, sometimes they don't. **Needs a rig.** Read the code rather than + measured: a live Claude session only learns of a peer message from the + `origin` object on a turn's `result` + (`session/claude/translate.rs`), which the CLI attaches to a turn the + message *started*. So a message that arrives mid-turn, or a second one + within one turn, has nowhere to be reported — while an imported session, + which syncs from the CLI's own file, picks up every one of them. That + would show exactly as "sometimes". Confirming it means driving a real + stream-json session and sending it messages in both states. ## Session settings diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/Ansi.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/Ansi.kt index a252c48..2e47cc7 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/Ansi.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/Ansi.kt @@ -71,8 +71,10 @@ fun ansiStyled(text: String, palette: AnsiPalette): AnnotatedString { if (final == 'm') sgr = sgr.apply(params, palette) } } - // A bare carriage return rewrites the line; one before a newline is the other half of - // a Windows line ending and has nothing to rewrite. + // A bare carriage return rewrites the line. One before a newline is the other half + // of a Windows line ending: it rewrites nothing, and it is dropped rather than kept, + // since that pair is one line break and the return itself would draw as a stray + // control character. c == '\r' && text.getOrNull(at + 1) != '\n' -> { flush() dropLine(runs) diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/MainActivity.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/MainActivity.kt index f298ac2..c998e89 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/MainActivity.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/MainActivity.kt @@ -13,8 +13,10 @@ import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.foundation.layout.navigationBarsPadding import androidx.compose.foundation.layout.statusBarsPadding +import androidx.compose.foundation.text.selection.LocalTextSelectionColors import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Surface +import androidx.compose.runtime.CompositionLocalProvider import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableIntStateOf import androidx.compose.runtime.mutableStateOf @@ -92,58 +94,64 @@ class MainActivity : ComponentActivity() { NotificationService.sync(this) setContent { + // Selection colours with the theme rather than at each place text is drawn: the + // transcript is one selection container, and a selection that ran from a reply into + // the code block under it would otherwise change colour halfway. See + // [AiAppSelectionColors]. MaterialTheme(colorScheme = AiAppColors) { - Surface(modifier = Modifier.fillMaxSize()) { - Box( - modifier = - // Timed like the transcript times itself, and for the same reason: - // the frame's draw phase is where Compose's measurement lands, and - // a report saying "draw is high" cannot otherwise say whether the - // cost is the transcript or the chrome around it. The keyboard is - // the case that made it matter -- every frame of the IME animation - // relays out and re-records this whole box. - Modifier.layout { measurable, constraints -> - val started = System.nanoTime() - val placeable = measurable.measure(constraints) - DebugStats.record( - "measure: the app root", - System.nanoTime() - started, - ) - layout(placeable.width, placeable.height) { - val placing = System.nanoTime() - placeable.place(0, 0) + CompositionLocalProvider(LocalTextSelectionColors provides AiAppSelectionColors) { + Surface(modifier = Modifier.fillMaxSize()) { + Box( + modifier = + // Timed like the transcript times itself, and for the same reason: + // the frame's draw phase is where Compose's measurement lands, and + // a report saying "draw is high" cannot otherwise say whether the + // cost is the transcript or the chrome around it. The keyboard is + // the case that made it matter -- every frame of the IME animation + // relays out and re-records this whole box. + Modifier.layout { measurable, constraints -> + val started = System.nanoTime() + val placeable = measurable.measure(constraints) DebugStats.record( - "place: the app root", - System.nanoTime() - placing, + "measure: the app root", + System.nanoTime() - started, + ) + layout(placeable.width, placeable.height) { + val placing = System.nanoTime() + placeable.place(0, 0) + DebugStats.record( + "place: the app root", + System.nanoTime() - placing, + ) + } + } + .drawWithContent { + val started = System.nanoTime() + drawContent() + DebugStats.record( + "record: the app root", + System.nanoTime() - started, ) } - } - .drawWithContent { - val started = System.nanoTime() - drawContent() - DebugStats.record( - "record: the app root", - System.nanoTime() - started, - ) - } - .fillMaxSize() - .statusBarsPadding() - // The gesture strip at the bottom of most - // phones. Without it the send row sits under - // the swipe area, where a tap is as likely to - // navigate away as to press a button. - // - // No imePadding here, deliberately: applied at the root it - // resizes this whole box on every frame of the keyboard - // animation, which re-measures, re-places and re-records every - // screen's entire tree per frame -- measured above as most of - // the frame budget. Each screen takes the keyboard itself - // (AppRoot wraps the ordinary ones; the session screen moves - // only its composer and transcript), so the per-frame cost is - // scoped to what actually moves. - .navigationBarsPadding() - ) { - AppRoot(settingsVersion, openRequest, shareRequest) + .fillMaxSize() + .statusBarsPadding() + // The gesture strip at the bottom of most + // phones. Without it the send row sits under + // the swipe area, where a tap is as likely to + // navigate away as to press a button. + // + // No imePadding here, deliberately: applied at the root it + // resizes this whole box on every frame of the keyboard + // animation, which re-measures, re-places and re-records every + // screen's entire tree per frame -- measured above as most of + // the frame budget. Each screen takes the keyboard itself + // (AppRoot wraps the ordinary ones; the session screen moves + // only its composer and transcript), so the per-frame cost is + // scoped to what actually moves. + .navigationBarsPadding() + ) { + AppRoot(settingsVersion, openRequest, shareRequest) + } } } } diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/MarkdownLinks.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/MarkdownLinks.kt index 25fac33..c4cdc8c 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/MarkdownLinks.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/MarkdownLinks.kt @@ -1,9 +1,13 @@ package com.example.aiapp -import androidx.compose.foundation.gestures.detectTapGestures +import androidx.compose.foundation.gestures.awaitEachGesture +import androidx.compose.foundation.gestures.awaitFirstDown +import androidx.compose.foundation.gestures.waitForUpOrCancellation import androidx.compose.foundation.text.BasicText import androidx.compose.runtime.Composable +import androidx.compose.runtime.compositionLocalOf import androidx.compose.runtime.remember +import androidx.compose.runtime.rememberUpdatedState import androidx.compose.ui.Modifier import androidx.compose.ui.geometry.Offset import androidx.compose.ui.graphics.isSpecified @@ -88,16 +92,40 @@ fun LinkedText(content: String, node: ASTNode, style: TextStyle, modifier: Modif content.buildMarkdownAnnotatedString(node, style, settings) } val uriHandler = LocalUriHandler.current + val onPlainTap = LocalMarkdownTap.current val layout = remember { Ref() } // The renderer's own rule for a style that names no colour: the theme's text colour. val color = if (style.color.isSpecified) style.color else LocalMarkdownColors.current.text BasicText( text = text, modifier = - modifier.pointerInput(text) { - detectTapGestures { position -> - val url = text.linkAt(layout.value, position) ?: return@detectTapGestures - uriHandler.openUri(url) + // A tap here is either a link or the card's; see [LocalMarkdownTap] for why the + // second one has to be answered from inside the text rather than left to the card. + modifier.pointerInput(text, onPlainTap) { + awaitEachGesture { + // Unconsumed is not required: something outside may already be tracking this + // press, and it is still the press that may land on a link. + awaitFirstDown(requireUnconsumed = false) + // A tap and nothing else. Null when the gesture became something somebody + // else's -- a scroll, or a press held past the long-press timeout, which is + // how a selection starts. The timeout is the load-bearing half: without it a + // press held for a second and released was still an up with nothing consumed, + // so holding a peer message to select from it shut the card instead. + val up = + withTimeoutOrNull(viewConfiguration.longPressTimeoutMillis) { + waitForUpOrCancellation() + } ?: return@awaitEachGesture + val url = text.linkAt(layout.value, up.position) + when { + url != null -> { + up.consume() + uriHandler.openUri(url) + } + onPlainTap != null -> { + up.consume() + onPlainTap() + } + } } }, style = style, @@ -106,6 +134,39 @@ fun LinkedText(content: String, node: ASTNode, style: TextStyle, modifier: Modif ) } +/** + * What a tap on markdown text means when it lands on no link -- shutting the card it is drawn in, + * usually -- or null where a plain tap means nothing. + * + * A composition local because there is nowhere else to put it. The paragraphs of a message are + * composed by the renderer's own dispatch out of its component table, so nothing between a card and + * the text inside it is ours to pass a parameter through; the renderer already hands its colours, + * its typography and its components down the same way. + * + * It exists because a pointer-input node over the glyphs takes the tap and the card's own click + * handler never sees it. Measured on the emulator against an opened peer message: with a handler on + * the text -- consuming or not -- a tap on its words did nothing at all, and with the handler + * removed entirely the same tap shut the card. So a card whose body is markdown cannot be shut by + * pressing its words unless the words do the shutting, and "nothing happens when I press it" is + * indistinguishable from a card that has stopped working. + * + * Provided as a value that outlives a recomposition (see [rememberMarkdownTap]), since a fresh + * lambda per composition would invalidate every paragraph reading it. + */ +val LocalMarkdownTap = compositionLocalOf<(() -> Unit)?> { null } + +/** + * [onTap] as a stable value to provide for [LocalMarkdownTap]. + * + * The identity stays put while the behaviour follows the latest [onTap], which is what keeps + * providing it from invalidating the text under it on every recomposition of the card. + */ +@Composable +fun rememberMarkdownTap(onTap: () -> Unit): () -> Unit { + val latest = rememberUpdatedState(onTap) + return remember { { latest.value() } } +} + /** * The address under [position], if a link's glyph is there rather than merely nearest to it. * diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/MemoryNote.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/MemoryNote.kt index 6123517..5def147 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/MemoryNote.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/MemoryNote.kt @@ -12,6 +12,7 @@ import androidx.compose.material3.Card import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Text import androidx.compose.runtime.Composable +import androidx.compose.runtime.CompositionLocalProvider import androidx.compose.runtime.remember import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier @@ -123,7 +124,12 @@ fun MemoryNote( ) } } - if (expanded) MarkdownText(note.text, replies, Modifier.padding(top = 4.dp)) + // The words shut the card too, and have to do it themselves -- see [LocalMarkdownTap]. + if (expanded) { + CompositionLocalProvider(LocalMarkdownTap provides rememberMarkdownTap(onToggle)) { + MarkdownText(note.text, replies, Modifier.padding(top = 4.dp)) + } + } } } } diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/PeerMessage.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/PeerMessage.kt index a62aa46..0da0622 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/PeerMessage.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/PeerMessage.kt @@ -13,6 +13,7 @@ import androidx.compose.material3.CardDefaults import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Text import androidx.compose.runtime.Composable +import androidx.compose.runtime.CompositionLocalProvider import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.draw.clip @@ -87,9 +88,14 @@ fun PeerBlockRow(unit: TranscriptUnit.PeerBlock, replies: ParsedReplies, onToggl onPress = onToggle, ) ) { - // The gap the card's own column used to provide between its heading and its prose, and - // between one block and the next -- inside the piece, so the card's fill runs through it. - MarkdownPiece(unit.text, unit.piece, replies, Modifier.padding(top = unit.spacing)) + // The words shut the card too, and have to do it themselves -- see [LocalMarkdownTap]. + // Without this the card closes everywhere except on the text, which is most of it. + CompositionLocalProvider(LocalMarkdownTap provides rememberMarkdownTap(onToggle)) { + // The gap the card's own column used to provide between its heading and its prose, + // and between one block and the next -- inside the piece, so the card's fill runs + // through it. + MarkdownPiece(unit.text, unit.piece, replies, Modifier.padding(top = unit.spacing)) + } } } diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt index 617a439..78834f6 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt @@ -281,6 +281,13 @@ fun SessionScreen( // have to ask whether anything is selected before they treat a tap as their own -- see // [expanding]. val selection = rememberSelectionState() + // Read here, at composition, rather than inside [expanding] at the moment of the click. The + // container clears the selection from the very press that a card then reads as its own, a few + // milliseconds earlier and in the same event -- so a card asking the live state at its click + // always hears "nothing is selected", and a tap meant to put a selection away also shut the + // tool call the words were in. This value is whatever was true as of the last frame, which is + // what the reader was looking at when they touched the screen. + val selecting = selection.selectedTexts.isNotEmpty() var expandedTools by remember { mutableStateOf(setOf()) } // Which runs of adjacent tool calls are open. Keyed by the first call's // id, so a group survives more calls arriving after it. @@ -509,7 +516,7 @@ fun SessionScreen( * hold for only some of them. */ fun expanding(toggle: () -> Unit) { - if (selection.selectedTexts.isNotEmpty()) { + if (selecting) { selection.clear() return } diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/Theme.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/Theme.kt index edee135..66c1e8f 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/Theme.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/Theme.kt @@ -1,5 +1,6 @@ package com.example.aiapp +import androidx.compose.foundation.text.selection.TextSelectionColors import androidx.compose.material3.ButtonColors import androidx.compose.material3.ButtonDefaults import androidx.compose.material3.MaterialTheme @@ -257,6 +258,26 @@ fun ansiPalette(): AnsiPalette = background = Mocha.Crust, ) +/** + * What a selection looks like, stated rather than left to Material's default. + * + * The default is `primary` at 40% alpha, which is a tint of whatever is behind it -- and this app + * draws text on surfaces two full steps apart. Over a reply, on Base, that reads clearly. Over a + * code block or a tool's output, on Crust, the same 40% composites to a barely-there smudge, so + * selecting a line of code looks like nothing happened even though the selection is there and + * copies correctly. + * + * Fixed and stronger, because "this is selected" is a meaning rather than decoration: a colour that + * means something must carry its own contrast instead of borrowing it from the surface it happens + * to land on. Raised only as far as it takes to read on the darkest of them -- past this the fill + * starts competing with the syntax colours it sits behind, which are the thing being read. + */ +val AiAppSelectionColors = + TextSelectionColors( + handleColor = Mocha.Mauve, + backgroundColor = Mocha.Mauve.copy(alpha = 0.55f), + ) + /** * A link. Blue is what a link is on every Catppuccin surface, and the one colour to leave alone. */ diff --git a/app/androidApp/src/test/kotlin/com/example/aiapp/AnsiTest.kt b/app/androidApp/src/test/kotlin/com/example/aiapp/AnsiTest.kt new file mode 100644 index 0000000..35657aa --- /dev/null +++ b/app/androidApp/src/test/kotlin/com/example/aiapp/AnsiTest.kt @@ -0,0 +1,87 @@ +package com.example.aiapp + +import androidx.compose.ui.graphics.Color +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertNull +import kotlin.test.assertTrue + +/** + * What survives a terminal's escape sequences, and what the styling reads as. + * + * Asserted as the plain text and as the style over a named substring, rather than as span offsets, + * so a failure prints the output that was got wrong instead of a pair of numbers. + */ +class AnsiTest { + private val palette = + AnsiPalette( + colours = (0..15).map { Color(it, 0, 0) }, + foreground = Color(1f, 1f, 1f), + background = Color(0f, 0f, 0f), + ) + + private fun styled(text: String) = ansiStyled(text, palette) + + /** The style covering the first character of [word], or null where nothing styles it. */ + private fun styleOver(text: String, word: String) = + styled(text).let { annotated -> + val at = annotated.text.indexOf(word) + assertTrue(at >= 0, "no \"$word\" in ${annotated.text}") + annotated.spanStyles.firstOrNull { at >= it.start && at < it.end }?.item + } + + private val esc = '\u001B' + + @Test + fun `a colour becomes a span and the sequence itself disappears`() { + val text = "plain ${esc}[31mred${esc}[0m plain" + assertEquals("plain red plain", styled(text).text) + assertEquals(palette.colours[1], styleOver(text, "red")?.color) + assertNull(styleOver(text, "plain")) + } + + @Test + fun `bright, background and 256-colour forms all reach the same table`() { + assertEquals(palette.colours[9], styleOver("${esc}[91mx", "x")?.color) + assertEquals(palette.colours[4], styleOver("${esc}[44mx", "x")?.background) + // The first sixteen of the 256-colour table are the palette's own, so a program that + // spells a colour either way gets the same one. + assertEquals(palette.colours[1], styleOver("${esc}[38;5;1mx", "x")?.color) + // And past them, xterm's cube: 16 is its black corner, 231 its white one. + assertEquals(Color(0, 0, 0), styleOver("${esc}[38;5;16mx", "x")?.color) + assertEquals(Color(255, 255, 255), styleOver("${esc}[38;5;231mx", "x")?.color) + assertEquals(Color(10, 20, 30), styleOver("${esc}[38;2;10;20;30mx", "x")?.color) + } + + @Test + fun `everything that is not styling is dropped rather than printed`() { + // A cursor move, an erase, an OSC window title with its bell, and a bare two-character + // escape. None of them mean anything in a scrolling document, and all of them would be + // line noise if the escape alone were stripped and the body left behind. + val text = "a${esc}[2Jb${esc}[Kc${esc}]0;a titled${esc}=e" + assertEquals("abcde", styled(text).text) + } + + @Test + fun `a carriage return rewrites its line, as it does on a terminal`() { + // What a progress bar looks like: every state it passed through, ending on the last. + assertEquals("done\n", styled("10%\r50%\rdone\n").text) + // The line before it is untouched. A Windows line ending rewrites nothing and is not + // kept either: it is one line break, and passing the carriage return through would draw + // a stray control character in the middle of the output. + assertEquals("kept\nlast", styled("kept\r\nfirst\rlast").text) + } + + @Test + fun `a sequence cut off mid-stream takes no text with it`() { + // Output still arriving ends anywhere, including inside an escape. The fragment goes and + // the whole sequence arrives with the next delta. + assertEquals("text ", styled("text ${esc}[3").text) + } + + @Test + fun `unstyled text costs no spans at all`() { + assertEquals(0, styled("nothing to do here").spanStyles.size) + assertEquals(0, styled("a${esc}[2Jb").spanStyles.size) + } +} diff --git a/app/androidApp/src/test/kotlin/com/example/aiapp/DurationsTest.kt b/app/androidApp/src/test/kotlin/com/example/aiapp/DurationsTest.kt new file mode 100644 index 0000000..6ca17b8 --- /dev/null +++ b/app/androidApp/src/test/kotlin/com/example/aiapp/DurationsTest.kt @@ -0,0 +1,56 @@ +package com.example.aiapp + +import java.time.Duration +import kotlin.test.Test +import kotlin.test.assertEquals + +/** + * The two ways a span of time is written here, and the rule each of them follows. + * + * Both are read off a screen to make a decision -- how long a tool call may take, how long a quota + * has left -- so what matters is that the shortest form that answers the question is what appears. + */ +class DurationsTest { + @Test + fun `under a minute is the largest unit alone`() { + assertEquals("30ms", formatMillis(30)) + assertEquals("999ms", formatMillis(999)) + assertEquals("1s", formatMillis(1000)) + assertEquals("2.5s", formatMillis(2500)) + // One decimal, rounded rather than cut: 2.46s is nearer two and a half than two and four. + assertEquals("2.5s", formatMillis(2460)) + assertEquals("59.9s", formatMillis(59_900)) + } + + @Test + fun `a minute or more is every unit that has something in it`() { + // The figure this rule was written for: a tool timeout, which arrives as milliseconds and + // is unreadable as 480000. + assertEquals("8m", formatMillis(480_000)) + assertEquals("1m", formatMillis(60_000)) + assertEquals("1m 30s", formatMillis(90_000)) + assertEquals("5d 12h 4m", formatMillis(475_440_000)) + // Empty units are left out rather than written as zero: the labels say which is which, + // and "5d 0h 4m" is only longer. + assertEquals("5d 4m", formatMillis(432_240_000)) + } + + @Test + fun `only a whole number of milliseconds is rewritten`() { + assertEquals("8m", formatMillisText(" 480000 ")) + // A timeout a tool expressed some other way is its own words, passed through rather than + // guessed at. + assertEquals("2 minutes", formatMillisText("2 minutes")) + assertEquals("", formatMillisText("")) + } + + @Test + fun `a countdown rounds up, so it never reports a minute already spent`() { + assertEquals("3h 13m", formatSpan(Duration.ofMinutes(192).plusSeconds(50))) + // Exactly on a minute is already the answer and is not pushed past it. + assertEquals("3h 12m", formatSpan(Duration.ofMinutes(192))) + assertEquals("12m", formatSpan(Duration.ofMinutes(12))) + // Rounding up carries, so a day's worth of minutes reads as a day. + assertEquals("1d 0h", formatSpan(Duration.ofHours(23).plusMinutes(59).plusSeconds(30))) + } +}