mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-10-06 03:38:23 +00:00
fix: search field audit — typed text rollback, blank chips, recomposition
- Desktop search screen: the field<->query sync compared the field to the displayText the effect was keyed on, which arrives through collectAsState a frame late. Two keystrokes between frames saw the field ahead of a stale key and setText() rolled the second one back. Compare against the query's current value instead; the binding moves into BindSearchField so a test drives the real code (reproduced failing: expected <ab> but was <a>). - A blank display/group/scope name no longer replaces the token: an empty profile name used to erase the chip entirely. - TokenizedSearchField only recomposes when the active picker changes, not on every keystroke, now that typing no longer needs recomposition. - The output transformation is keyed on the field state too, so a new SearchFieldState is never read through a stale caret lambda. - replaceRuns compares in place instead of allocating a substring. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013ESr8Pkt6WJsVkD5dUZEw6
This commit is contained in:
+5
-4
@@ -58,7 +58,8 @@ internal fun TextFieldBuffer.replaceRuns(
|
||||
) {
|
||||
for (i in runs.indices.reversed()) {
|
||||
val run = runs[i]
|
||||
if (!run.drawn.contentEquals(typed.subSequence(run.start, run.end))) replace(run.start, run.end, run.drawn)
|
||||
val unchanged = run.drawn.length == run.end - run.start && typed.regionMatches(run.start, run.drawn, 0, run.drawn.length)
|
||||
if (!unchanged) replace(run.start, run.end, run.drawn)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -129,7 +130,7 @@ class SearchTokenTransformation(
|
||||
): String =
|
||||
when (seg) {
|
||||
is SearchSegment.Key -> {
|
||||
val name = displayName(seg.pubkey)
|
||||
val name = displayName(seg.pubkey)?.takeIf { it.isNotBlank() }
|
||||
val shown = name?.let { clip(it, 32) } ?: shortBech32(rawKeyOf(raw))
|
||||
seg.field?.let { "${it.token}:$shown" } ?: shown
|
||||
}
|
||||
@@ -137,9 +138,9 @@ class SearchTokenTransformation(
|
||||
is SearchSegment.Pointer -> "to:${shortBech32(raw.substringAfter(':'))}"
|
||||
// A group id is a stranger's opaque string; its name is the only part a reader can
|
||||
// check against the room they meant. The id stays the value, so the query is unchanged.
|
||||
is SearchSegment.Group -> "group:${groupName(seg.id)?.let { clip(it, 32) } ?: seg.id}"
|
||||
is SearchSegment.Group -> "group:${groupName(seg.id)?.takeIf { it.isNotBlank() }?.let { clip(it, 32) } ?: seg.id}"
|
||||
// Likewise a geohash: "9q8yy" says nothing, "San Francisco" says what was filtered on.
|
||||
is SearchSegment.Scope -> scopeName(seg.field, seg.value)?.let { "${seg.field}:${clip(it, 32)}" } ?: raw
|
||||
is SearchSegment.Scope -> scopeName(seg.field, seg.value)?.takeIf { it.isNotBlank() }?.let { "${seg.field}:${clip(it, 32)}" } ?: raw
|
||||
// A kind typed as a number draws under the name the registry has for it, so a screen
|
||||
// that seeds `kind:20` shows "kind:picture" — the only form a reader can check. Only
|
||||
// an exact one-token match is used: `kind:30312` must not draw as the wider `live`.
|
||||
|
||||
+6
-2
@@ -35,6 +35,7 @@ import androidx.compose.material3.MaterialTheme
|
||||
import androidx.compose.material3.Text
|
||||
import androidx.compose.runtime.Composable
|
||||
import androidx.compose.runtime.LaunchedEffect
|
||||
import androidx.compose.runtime.derivedStateOf
|
||||
import androidx.compose.runtime.getValue
|
||||
import androidx.compose.runtime.mutableIntStateOf
|
||||
import androidx.compose.runtime.remember
|
||||
@@ -117,7 +118,10 @@ fun TokenizedSearchField(
|
||||
decorationBox: (@Composable (@Composable () -> Unit) -> Unit)? = null,
|
||||
) {
|
||||
val styles = rememberSearchTokenStyles()
|
||||
val picker = state.activePicker
|
||||
// Derived, so typing that leaves the picker as it was (plain words, a caret moving through
|
||||
// them) does not recompose the field and everything under it on every keystroke.
|
||||
val pickerState = remember(state) { derivedStateOf { state.activePicker } }
|
||||
val picker = pickerState.value
|
||||
var highlighted by rememberSaveable(picker?.token?.start) { mutableIntStateOf(0) }
|
||||
|
||||
// The one picker whose rows this component fills in itself: the kind vocabulary is a
|
||||
@@ -159,7 +163,7 @@ fun TokenizedSearchField(
|
||||
cursorBrush = SolidColor(MaterialTheme.colorScheme.primary),
|
||||
// The caret is read inside the transformation, not keyed on here: the field re-runs it
|
||||
// whenever a state it read changes, so a new instance per caret move is not needed.
|
||||
outputTransformation = remember(styles, displayName, groupName, scopeName) { SearchTokenTransformation(state::settleCaret, styles, displayName, groupName, scopeName) },
|
||||
outputTransformation = remember(state, styles, displayName, groupName, scopeName) { SearchTokenTransformation(state::settleCaret, styles, displayName, groupName, scopeName) },
|
||||
decorator =
|
||||
TextFieldDecorator { inner ->
|
||||
if (decorationBox != null) {
|
||||
|
||||
+9
@@ -220,4 +220,13 @@ class SearchTokenTransformationTest {
|
||||
assertEquals("hello world", transform("\"hello world"))
|
||||
assertAppliesCleanly("\"hello world")
|
||||
}
|
||||
|
||||
@Test
|
||||
fun aBlankNameIsNotAName() {
|
||||
// A profile whose name is blank must not erase its chip: it keeps the short npub instead.
|
||||
val out = transform(NPUB, name = { " " })
|
||||
assertTrue(out.startsWith("npub1") && out.contains("…"), out)
|
||||
assertEquals("group:abc123", transform("group:abc123", groupName = { "" }))
|
||||
assertEquals("geo:9q8yy", transform("geo:9q8yy", scopeName = { _, _ -> " " }))
|
||||
}
|
||||
}
|
||||
|
||||
+22
-6
@@ -154,12 +154,7 @@ fun SearchScreen(
|
||||
val searchRelays by relayCategories.searchRelays.collectAsState()
|
||||
val displayText by state.displayText.collectAsState()
|
||||
|
||||
// The field drives the query. The reverse direction only fires for a form-driven change (the
|
||||
// advanced panel, a loaded saved search), and the guard is what keeps the two from looping.
|
||||
LaunchedEffect(fieldState.text) { state.updateFromText(fieldState.text) }
|
||||
LaunchedEffect(displayText) {
|
||||
if (fieldState.text != displayText) fieldState.setText(displayText)
|
||||
}
|
||||
BindSearchField(fieldState, state, displayText)
|
||||
|
||||
// The people a half-written `from:`/`to:` token offers. Cache first, then the search relays.
|
||||
val userSearch =
|
||||
@@ -1003,3 +998,24 @@ private fun SearchResultCard(
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Keeps the field and the query in step. The field drives the query; the reverse direction only
|
||||
* fires for a form-driven change (the advanced panel, a loaded saved search).
|
||||
*
|
||||
* The guard compares against the query's *current* text, not the [displayText] this effect was
|
||||
* keyed on: that value arrives through `collectAsState` a frame late, so two keystrokes landing
|
||||
* between frames would otherwise see the field ahead of a stale key and roll the second one back.
|
||||
*/
|
||||
@Composable
|
||||
internal fun BindSearchField(
|
||||
fieldState: SearchFieldState,
|
||||
state: AdvancedSearchBarState,
|
||||
displayText: String,
|
||||
) {
|
||||
LaunchedEffect(fieldState.text) { state.updateFromText(fieldState.text) }
|
||||
LaunchedEffect(displayText) {
|
||||
val current = state.displayText.value
|
||||
if (fieldState.text != current) fieldState.setText(current)
|
||||
}
|
||||
}
|
||||
|
||||
+77
@@ -0,0 +1,77 @@
|
||||
/*
|
||||
* Copyright (c) 2025 Vitor Pamplona
|
||||
*
|
||||
* Permission is hereby granted, free of charge, to any person obtaining a copy of
|
||||
* this software and associated documentation files (the "Software"), to deal in
|
||||
* the Software without restriction, including without limitation the rights to use,
|
||||
* copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the
|
||||
* Software, and to permit persons to whom the Software is furnished to do so,
|
||||
* subject to the following conditions:
|
||||
*
|
||||
* The above copyright notice and this permission notice shall be included in all
|
||||
* copies or substantial portions of the Software.
|
||||
*
|
||||
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
|
||||
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS
|
||||
* FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR
|
||||
* COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN
|
||||
* AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
|
||||
* WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
|
||||
*/
|
||||
package com.vitorpamplona.amethyst.desktop.ui.search
|
||||
|
||||
import androidx.compose.foundation.text.input.insert
|
||||
import androidx.compose.runtime.collectAsState
|
||||
import androidx.compose.runtime.getValue
|
||||
import androidx.compose.ui.test.junit4.v2.createComposeRule
|
||||
import com.vitorpamplona.amethyst.commons.search.AdvancedSearchBarState
|
||||
import com.vitorpamplona.amethyst.commons.ui.search.SearchFieldState
|
||||
import com.vitorpamplona.amethyst.desktop.ui.BindSearchField
|
||||
import kotlinx.coroutines.CoroutineScope
|
||||
import kotlinx.coroutines.Dispatchers
|
||||
import kotlinx.coroutines.SupervisorJob
|
||||
import org.junit.Rule
|
||||
import kotlin.test.Test
|
||||
import kotlin.test.assertEquals
|
||||
import kotlin.test.assertTrue
|
||||
|
||||
/**
|
||||
* The desktop search screen's two-way binding between the field and the query, driven frame by
|
||||
* frame so two keystrokes can land between one frame and the next.
|
||||
*/
|
||||
class SearchSyncRaceTest {
|
||||
@get:Rule
|
||||
val compose = createComposeRule()
|
||||
|
||||
@Test
|
||||
fun twoKeystrokesBetweenFramesAreNotRolledBack() {
|
||||
val field = SearchFieldState()
|
||||
val state = AdvancedSearchBarState(CoroutineScope(SupervisorJob() + Dispatchers.Unconfined))
|
||||
compose.mainClock.autoAdvance = false
|
||||
compose.setContent {
|
||||
val displayText by state.displayText.collectAsState()
|
||||
BindSearchField(field, state, displayText)
|
||||
}
|
||||
compose.mainClock.advanceTimeByFrame()
|
||||
field.textState.edit { insert(length, "a") }
|
||||
compose.mainClock.advanceTimeByFrame()
|
||||
field.textState.edit { insert(length, "b") }
|
||||
repeat(5) { compose.mainClock.advanceTimeByFrame() }
|
||||
assertEquals("ab", field.text)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun aFormDrivenChangeStillReachesTheField() {
|
||||
val field = SearchFieldState("bitcoin")
|
||||
val state = AdvancedSearchBarState(CoroutineScope(SupervisorJob() + Dispatchers.Unconfined))
|
||||
compose.setContent {
|
||||
val displayText by state.displayText.collectAsState()
|
||||
BindSearchField(field, state, displayText)
|
||||
}
|
||||
compose.waitForIdle()
|
||||
state.updateKinds(listOf(1))
|
||||
compose.waitForIdle()
|
||||
assertEquals(state.displayText.value, field.text)
|
||||
assertTrue(field.text.contains("kind:"), field.text)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user