Thin the app's comments
The same pass the server had, on the Kotlin side: comments restating what the code says are gone, and the ones recording a measurement, a constraint or an incident are kept but cut to a few lines each. 6540 comment lines to 5674, and 920 lines off the app. Two doc comments had drifted onto the item above the one they describe -- `contextAfter`'s onto `sessionWorking` in Events.kt, and `UsageMonitor`'s equivalent on the server was fixed in the previous commit. Each is back on its own item, which is the only non-comment line this diff moves. The comments are reflowed to the column limit at their own indentation: several were written wide, and ktfmt re-wrapped them into lines holding a single orphan word. `/tmp` script, not kept -- ktfmt is idempotent over the result, which is the check. Left alone deliberately: this codebase's remaining comment density is high because the comments carry things the code cannot say -- what a null means, what a number was measured against, which bug a guard exists for. Of the 238 one-line doc comments in the app, five were pure restatement of the name and were removed; the rest each say something the signature does not. ktfmtFormat, compileDebugKotlin, lintDebug and testDebugUnitTest pass; cargo test (127), clippy --all-targets and fmt still clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
79682f03a7
commit
edc39c7371
68 files changed
+2077
-2997
No files matched your search
@@ -42,14 +42,11 @@ import androidx.compose.ui.unit.dp
|
||||
* the transcript's own order is what paging and the event stream depend on, and one screen's idea
|
||||
* of "these belong together" must not reach back into it.
|
||||
*
|
||||
* Immutable, and said so, because Compose cannot tell.
|
||||
*
|
||||
* A row is a value: it is rebuilt from the transcript rather than edited, and two rows describing
|
||||
* the same events are equal. Compose infers stability from a class's fields, and a `List` field --
|
||||
* which several of these carry -- makes it assume the worst, so every composable taking one
|
||||
* recomposed whenever anything above it did. A page of history landing recomposed all 148 loaded
|
||||
* rows including the markdown inside them, measured as 701 compositions for 148 rows in one scroll,
|
||||
* and that is what a page landing costs on top of the fetch itself.
|
||||
* Immutable, and said so, because Compose cannot tell: a row is rebuilt from the transcript rather
|
||||
* than edited, and two rows describing the same events are equal. Compose infers stability from a
|
||||
* class's fields, and a `List` field -- which several of these carry -- makes it assume the worst,
|
||||
* so a page of history landing recomposed all 148 loaded rows including the markdown inside them,
|
||||
* measured as 701 compositions for 148 rows in one scroll.
|
||||
*
|
||||
* The promise this makes is real and has to stay true: nothing here is mutated after it is built.
|
||||
*/
|
||||
@@ -59,16 +56,12 @@ sealed class TranscriptRow {
|
||||
* This row's identity in the list, which must survive everything that can happen to the row.
|
||||
*
|
||||
* The list is keyed by this so that inserting a new message at one end, or a page of history at
|
||||
* the other, moves the rows and not the reader. That makes it the load-bearing value on this
|
||||
* screen: when a key changes, the list loses its anchor and the transcript steps under whoever
|
||||
* is reading it.
|
||||
* the other, moves the rows and not the reader. When a key changes, the list loses its anchor
|
||||
* and the transcript steps under whoever is reading it.
|
||||
*
|
||||
* A tool row therefore keys on [TranscriptItem.ToolRun.runId] rather than on a sequence number,
|
||||
* and it is the *same* value whether the run is drawn as one card or as a group. A lone call
|
||||
* that gains a neighbour becomes a group without changing identity, which is the case a
|
||||
* seq-based key got wrong: the row the reader was looking at was replaced rather than updated.
|
||||
* Which value that is belongs to the item ([TranscriptItem.key]), not to a `when` here: a row
|
||||
* is one item and the item is what knows what it is called.
|
||||
* and it is the *same* value whether the run is drawn as one card or as a group. Which value
|
||||
* that is belongs to the item ([TranscriptItem.key]), not to a `when` here.
|
||||
*/
|
||||
abstract val key: Any
|
||||
|
||||
@@ -76,12 +69,9 @@ sealed class TranscriptRow {
|
||||
* Where this row starts in the transcript: the sequence number of the oldest event behind it.
|
||||
*
|
||||
* Separate from [key], and deliberately so. [key] is the list's identity and is a display
|
||||
* decision -- a tool row is named after its run, and a run takes its name from whichever call
|
||||
* was first when it was folded, which changes as pages arrive. A seq is the server's own
|
||||
* numbering: it is assigned once, never moves, and means the same thing to every device. So
|
||||
* anything that has to point at a place in the conversation and still find it later -- a saved
|
||||
* scroll position is the one -- points with this, and anything that has to identify a row
|
||||
* within one composition uses [key].
|
||||
* decision; a seq is the server's own numbering, assigned once and meaning the same thing to
|
||||
* every device. So anything that has to point at a place in the conversation and still find it
|
||||
* later -- a saved scroll position -- points with this.
|
||||
*/
|
||||
abstract val startSeq: Long
|
||||
|
||||
@@ -133,8 +123,7 @@ private fun groupRuns(items: List<TranscriptItem>): List<TranscriptRow> {
|
||||
// Grouped by the run each call says it belongs to, not by adjacency worked out here.
|
||||
// Adjacency is the same answer most of the time and a worse one at the edges: a call
|
||||
// arriving next to an existing run, or a page of history arriving in front of one, both
|
||||
// change which call is *first*, and a group named after its first member is a different
|
||||
// group every time that happens.
|
||||
// change which call is *first*.
|
||||
if (item is TranscriptItem.ToolRun && (run.isEmpty() || run.first().runId == item.runId)) {
|
||||
run += item
|
||||
} else {
|
||||
@@ -152,18 +141,14 @@ private fun groupRuns(items: List<TranscriptItem>): List<TranscriptRow> {
|
||||
* What says the calls belong together is the surface behind them, which is the one cue rather than
|
||||
* two half-cues -- rounded to the same corner every other card in the app has, so a group reads as
|
||||
* one object rather than as a square patch behind round things. The calls sit on it inset by
|
||||
* [GROUP_INSET], which is the container's own padding rather than an indent: they are the same rows
|
||||
* they would be on their own, and a rounded corner drawn hard against a rounded corner reads as a
|
||||
* notch.
|
||||
* [GROUP_INSET], which is the container's own padding rather than an indent.
|
||||
*
|
||||
* Inside, the calls are a connected stack. Facing corners are square and the outer ones are not, so
|
||||
* the run reads as one thing broken into its parts; [GROUP_GAP] keeps the parts legible without
|
||||
* separating them. See [connectedShape].
|
||||
* the run reads as one thing broken into its parts; see [connectedShape].
|
||||
*
|
||||
* It closes from either end. A long group's header scrolls off while its last call is still on
|
||||
* screen, and the reader who wants it shut is looking at the bottom, not hunting for the top. The
|
||||
* bar at the foot is the same height as the heading at the top, so the surface the calls sit on is
|
||||
* as thick below them as above.
|
||||
* screen, and the reader who wants it shut is looking at the bottom. The bar at the foot is the
|
||||
* same height as the heading at the top.
|
||||
*/
|
||||
@Composable
|
||||
fun ToolGroup(
|
||||
@@ -171,8 +156,7 @@ fun ToolGroup(
|
||||
expanded: Boolean,
|
||||
/**
|
||||
* Where it was pressed is the row's business rather than the control's -- a group has a control
|
||||
* at each end, and only the row knows where its own ends are, so the row records the touch
|
||||
* itself and this just says that one happened.
|
||||
* at each end, and only the row knows where its own ends are.
|
||||
*/
|
||||
onToggle: () -> Unit,
|
||||
isToolExpanded: (String) -> Boolean,
|
||||
@@ -222,8 +206,8 @@ fun ToolGroup(
|
||||
)
|
||||
}
|
||||
}
|
||||
// Shutting it from here anchors the other end: the reader is at the bottom of a long
|
||||
// group, and what they are looking at is what follows it.
|
||||
// Shutting it from here anchors the other end: the reader is at the bottom of a long group,
|
||||
// and what they are looking at is what follows it.
|
||||
CollapseBar(barHeight, onToggle)
|
||||
}
|
||||
}
|
||||
@@ -232,8 +216,8 @@ fun ToolGroup(
|
||||
* The height of a group's heading, and so of the bar at its foot.
|
||||
*
|
||||
* Derived from the type the heading is set in rather than written down, because the two have to
|
||||
* match and a pair of numbers chosen to look equal stops being equal the moment either the style or
|
||||
* the density changes. Taking the line height also means the heading cannot be clipped by it.
|
||||
* match and a pair of numbers chosen to look equal stops being equal the moment the density
|
||||
* changes.
|
||||
*/
|
||||
@Composable
|
||||
private fun groupBarHeight(): Dp {
|
||||
@@ -242,10 +226,9 @@ private fun groupBarHeight(): Dp {
|
||||
}
|
||||
|
||||
/**
|
||||
* The bottom half of a group's toggle: an arrow back up to its heading.
|
||||
*
|
||||
* Given the heading's height rather than padded to something that looks close, so the surface the
|
||||
* calls sit on is the same thickness at both ends. See [groupBarHeight].
|
||||
* The bottom half of a group's toggle: an arrow back up to its heading. Given the heading's height
|
||||
* rather than padded to something that looks close, so the surface the calls sit on is the same
|
||||
* thickness at both ends.
|
||||
*/
|
||||
@Composable
|
||||
private fun CollapseBar(height: Dp, onToggle: () -> Unit) {
|
||||
@@ -266,8 +249,7 @@ private fun CollapseBar(height: Dp, onToggle: () -> Unit) {
|
||||
* does not.
|
||||
*
|
||||
* Written once and given an index rather than branched at each end, because a stack has three cases
|
||||
* that are one rule -- and the middle one is the case a hand-written first/last pair gets wrong
|
||||
* when a run turns out to have three calls in it.
|
||||
* that are one rule -- and the middle one is what a hand-written first/last pair gets wrong.
|
||||
*/
|
||||
@Composable
|
||||
private fun connectedShape(index: Int, count: Int): CornerBasedShape {
|
||||
@@ -294,12 +276,10 @@ private val GROUP_GAP = 2.dp
|
||||
* One tool call.
|
||||
*
|
||||
* Closed, it is a single line: the tool's name and what the call is for. The command itself is not
|
||||
* on it, because a wrapped command turns one row into four and a run of them into a wall -- and the
|
||||
* name plus the intent is what somebody scanning the transcript is reading for.
|
||||
* on it, because a wrapped command turns one row into four and a run of them into a wall.
|
||||
*
|
||||
* Open, it shows the command, whatever else the input carried, and the output. The timeout sits at
|
||||
* the top right: it is a limit on the call rather than part of what the call does, and it is worth
|
||||
* seeing beside the command it constrains rather than buried in the fields below it.
|
||||
* the top right: it is a limit on the call rather than part of what the call does.
|
||||
*
|
||||
* A call waiting on permission is shown open whatever the reader last chose, since the command is
|
||||
* the thing being decided and a row saying only "Bash" cannot be decided on.
|
||||
@@ -342,10 +322,9 @@ fun ToolCard(
|
||||
)
|
||||
} ?: Spacer(Modifier.weight(1f))
|
||||
}
|
||||
// A spinner says the machine is working. While this call is waiting on an
|
||||
// answer the machine is doing nothing at all -- the turn is stopped on the
|
||||
// person reading it -- so it says whose move it is instead, in the colour this
|
||||
// app uses everywhere for that.
|
||||
// A spinner says the machine is working. While this call is waiting on an answer
|
||||
// the machine is doing nothing at all -- the turn is stopped on the person reading
|
||||
// it -- so it says whose move it is instead.
|
||||
if (deciding) {
|
||||
Spacer(Modifier.width(8.dp))
|
||||
Text(
|
||||
@@ -370,9 +349,9 @@ fun ToolCard(
|
||||
modifier = Modifier.padding(top = 4.dp),
|
||||
)
|
||||
}
|
||||
// Everything AskUserQuestion carries is the questions, and those are drawn
|
||||
// below as something answerable; dumping the same JSON above them would be the
|
||||
// decision stated twice, once unreadably.
|
||||
// Everything AskUserQuestion carries is the questions, and those are drawn below as
|
||||
// something answerable; dumping the same JSON above them would be the decision
|
||||
// stated twice, once unreadably.
|
||||
if (tool.tool != ASK_USER_QUESTION) {
|
||||
ToolInputView(tool.tool, tool.input, Modifier.padding(top = 4.dp))
|
||||
}
|
||||
@@ -381,14 +360,12 @@ fun ToolCard(
|
||||
Text("Output", style = MaterialTheme.typography.labelSmall)
|
||||
// What the tool printed, on the surface everything verbatim gets and in the
|
||||
// face it was written for: this is column-aligned far more often than it is
|
||||
// prose -- a directory listing, a diff, a table of numbers -- and a
|
||||
// proportional font silently destroys the alignment that carried the meaning.
|
||||
// prose, and a proportional font silently destroys the alignment that carried
|
||||
// the meaning.
|
||||
//
|
||||
// Its terminal styling applied and the rest of the escapes taken out, since
|
||||
// what a shell prints is written for a terminal: colour is often the whole of
|
||||
// what a diff or a test run is saying, and the sequences that carry it are
|
||||
// unreadable drawn verbatim. Remembered against the text, so a card that is
|
||||
// open through a scroll parses once. See [ansiStyled].
|
||||
// Its terminal styling applied and the rest of the escapes taken out: colour is
|
||||
// often the whole of what a diff or a test run is saying. Remembered against
|
||||
// the text, so a card that is open through a scroll parses once.
|
||||
val palette = remember { ansiPalette() }
|
||||
val styled = remember(tool.output, palette) { ansiStyled(tool.output, palette) }
|
||||
RawBlock(Modifier.padding(top = 2.dp)) {
|
||||
@@ -400,10 +377,8 @@ fun ToolCard(
|
||||
}
|
||||
}
|
||||
}
|
||||
// Shown open or closed. A call that produced a picture is one
|
||||
// whose result *is* the picture, and a row that hides it says
|
||||
// less than the one line it replaced -- unlike a command, which
|
||||
// is what the closed line already summarises.
|
||||
// Shown open or closed. A call that produced a picture is one whose result *is* the
|
||||
// picture, and a row that hides it says less than the one line it replaced.
|
||||
tool.images.forEach { ref -> image(ref) }
|
||||
if (tool.asks.isNotEmpty()) {
|
||||
if (tool.tool == ASK_USER_QUESTION) {
|
||||
@@ -428,11 +403,10 @@ private fun PermissionAsk(
|
||||
onAnswer: (List<QuestionAnswer>, onSettled: () -> Unit) -> Unit,
|
||||
) {
|
||||
// What was pressed, before the answer has been round-tripped. Two bare words with no submit
|
||||
// step -- unlike a question card, where the answer is several choices and worth reviewing --
|
||||
// so the press has to be its own acknowledgement or the row sits unchanged for a round trip
|
||||
// and reads as having missed the tap. Cleared when the request settles: by then either the
|
||||
// answer is in `ask.answers` and the mark stands on a measurement, or it failed and the
|
||||
// buttons come back rather than leaving a decision marked that nothing recorded.
|
||||
// step -- unlike a question card, where the answer is worth reviewing -- so the press has to be
|
||||
// its own acknowledgement or the row sits unchanged for a round trip. Cleared when the request
|
||||
// settles: by then either the answer is in `ask.answers`, or it failed and the buttons come
|
||||
// back.
|
||||
var pressed by remember(ask.id) { mutableStateOf<String?>(null) }
|
||||
Spacer(Modifier.height(8.dp))
|
||||
Text(
|
||||
@@ -442,8 +416,7 @@ private fun PermissionAsk(
|
||||
)
|
||||
// Answered or not, the options stay and the one that was taken is marked -- see
|
||||
// [AskedQuestion], which is the same rule on the question card. A permission is where it
|
||||
// matters most: "Answered: Deny" alone does not say that Allow was the alternative, and
|
||||
// whether a tool was allowed or refused is the thing a reader comes back to this row for.
|
||||
// matters most: "Answered: Deny" alone does not say that Allow was the alternative.
|
||||
val settled = ask.answers.isNotEmpty()
|
||||
AnswerOptions(
|
||||
ask.options,
|
||||
|
||||
Reference in new issue
Block a user