From 47165ce93fda77297b2ede4dfaf5894821ee163a Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Thu, 22 Oct 2026 11:22:05 +0200 Subject: [PATCH] fix(worldclock): keep a ticking row's accessibility actions stable With seconds on every row recomposed each second and rebuilt its custom actions from new lambdas, so TalkBack saw changed semantics every second. The callbacks are remembered per id and the action list per label and callback. The prefs isolation test now uses a barrier on the same flow, a weak settings assertion is gone, and a clock set is tested to show at once and restart the minute tick. --- .../clockula/ui/worldclock/WorldClockRow.kt | 35 +++++++++++-------- .../ui/worldclock/WorldClockScreen.kt | 9 +++-- .../clockula/data/prefs/ClockPrefsTest.kt | 7 ++-- .../ui/settings/SettingsViewModelTest.kt | 7 ++-- .../ui/worldclock/WorldClockViewModelTest.kt | 20 +++++++++++ 5 files changed, 53 insertions(+), 25 deletions(-) diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/ui/worldclock/WorldClockRow.kt b/app/src/main/java/de/jeanlucmakiola/clockula/ui/worldclock/WorldClockRow.kt index 315c91c..b6c6219 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/ui/worldclock/WorldClockRow.kt +++ b/app/src/main/java/de/jeanlucmakiola/clockula/ui/worldclock/WorldClockRow.kt @@ -83,20 +83,27 @@ internal fun WorldClockRow( } else { stringResource(R.string.world_clock_row_description_unknown, row.name, summary) } - val actions = listOf( - CustomAccessibilityAction(stringResource(R.string.world_clock_row_move_up)) { - onMoveUp() - true - }, - CustomAccessibilityAction(stringResource(R.string.world_clock_row_move_down)) { - onMoveDown() - true - }, - CustomAccessibilityAction(stringResource(R.string.world_clock_row_remove)) { - onRemove() - true - }, - ) + val moveUpLabel = stringResource(R.string.world_clock_row_move_up) + val moveDownLabel = stringResource(R.string.world_clock_row_move_down) + val removeLabel = stringResource(R.string.world_clock_row_remove) + // Remembered, so a row that ticks every second keeps equal actions and its + // semantics only change when what TalkBack would say changes. + val actions = remember(moveUpLabel, moveDownLabel, removeLabel, onMoveUp, onMoveDown, onRemove) { + listOf( + CustomAccessibilityAction(moveUpLabel) { + onMoveUp() + true + }, + CustomAccessibilityAction(moveDownLabel) { + onMoveDown() + true + }, + CustomAccessibilityAction(removeLabel) { + onRemove() + true + }, + ) + } GroupedSurface( position = position, diff --git a/app/src/main/java/de/jeanlucmakiola/clockula/ui/worldclock/WorldClockScreen.kt b/app/src/main/java/de/jeanlucmakiola/clockula/ui/worldclock/WorldClockScreen.kt index b4efa20..791959c 100644 --- a/app/src/main/java/de/jeanlucmakiola/clockula/ui/worldclock/WorldClockScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/clockula/ui/worldclock/WorldClockScreen.kt @@ -21,6 +21,7 @@ import androidx.compose.material3.TextButton import androidx.compose.runtime.Composable import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember import androidx.compose.runtime.saveable.rememberSaveable import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment @@ -158,9 +159,11 @@ fun WorldClockScreen( timeStyle = rowTimeStyle, dragHandle = dragHandle, isDragging = isDragging, - onMoveUp = { viewModel.onMoveUp(row.id) }, - onMoveDown = { viewModel.onMoveDown(row.id) }, - onRemove = { pendingRemoval = row.id }, + // Remembered per id: a row that ticks every second must + // not hand TalkBack new custom actions every second. + onMoveUp = remember(id) { { viewModel.onMoveUp(id) } }, + onMoveDown = remember(id) { { viewModel.onMoveDown(id) } }, + onRemove = remember(id) { { pendingRemoval = id } }, ) } } diff --git a/app/src/test/java/de/jeanlucmakiola/clockula/data/prefs/ClockPrefsTest.kt b/app/src/test/java/de/jeanlucmakiola/clockula/data/prefs/ClockPrefsTest.kt index c11a2c6..a23c986 100644 --- a/app/src/test/java/de/jeanlucmakiola/clockula/data/prefs/ClockPrefsTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/clockula/data/prefs/ClockPrefsTest.kt @@ -90,10 +90,11 @@ class ClockPrefsTest { assertThat(awaitItem()).isEqualTo(ClockDefaults()) prefs.setWorldClockSeconds(true) - // Wait until the write is visible to a reader, so a re-emission of - // the defaults would have had its chance to arrive. - prefs.worldClockSeconds.first { it } + // A barrier on this same flow: the next item must be this write, + // so a re-emission caused by the seconds write would come first. + prefs.setSnoozeMinutes(7) + assertThat(awaitItem()).isEqualTo(ClockDefaults(snoozeMinutes = 7)) expectNoEvents() cancel() } diff --git a/app/src/test/java/de/jeanlucmakiola/clockula/ui/settings/SettingsViewModelTest.kt b/app/src/test/java/de/jeanlucmakiola/clockula/ui/settings/SettingsViewModelTest.kt index 4391896..af80c34 100644 --- a/app/src/test/java/de/jeanlucmakiola/clockula/ui/settings/SettingsViewModelTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/clockula/ui/settings/SettingsViewModelTest.kt @@ -130,18 +130,15 @@ class SettingsViewModelTest { } @Test - fun `the world clock's seconds switch writes through, apart from the defaults`(@TempDir tempDir: Path) = + fun `the world clock's seconds switch writes through`(@TempDir tempDir: Path) = runTest(main.dispatcher) { val viewModel = collecting(newViewModel(tempDir)) advanceUntilIdle() - val defaultsBefore = (viewModel.state.value as SettingsUiState.Loaded).defaults viewModel.onWorldClockSecondsChanged(true) advanceUntilIdle() - val loaded = viewModel.state.value as SettingsUiState.Loaded - assertThat(loaded.worldClockSeconds).isTrue() - assertThat(loaded.defaults).isEqualTo(defaultsBefore) + assertThat((viewModel.state.value as SettingsUiState.Loaded).worldClockSeconds).isTrue() } @Test diff --git a/app/src/test/java/de/jeanlucmakiola/clockula/ui/worldclock/WorldClockViewModelTest.kt b/app/src/test/java/de/jeanlucmakiola/clockula/ui/worldclock/WorldClockViewModelTest.kt index 0666b93..236858e 100644 --- a/app/src/test/java/de/jeanlucmakiola/clockula/ui/worldclock/WorldClockViewModelTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/clockula/ui/worldclock/WorldClockViewModelTest.kt @@ -20,6 +20,7 @@ import org.junit.jupiter.api.io.TempDir import java.nio.file.Files import java.nio.file.Path import java.time.ZoneId +import kotlin.time.Duration.Companion.minutes import kotlin.time.Duration.Companion.seconds /** @@ -414,6 +415,25 @@ class WorldClockViewModelTest { assertThat(harness.loaded().home.zoneId).isEqualTo("Asia/Tokyo") } + /** #23: a set clock shows at once, and the tick restarts against the new time. */ + @Test + fun `a clock set shows at once and restarts the minute tick`(@TempDir tempDir: Path) = + runTest(main.dispatcher) { + val harness = collecting(worldClockHarness(tempDir)) + advanceUntilIdle() + val before = harness.loaded().home.time + + harness.wallClock.advance(5.minutes) + harness.clockChanges.fire() + advanceUntilIdle() + + assertThat(harness.loaded().home.time).isEqualTo(before.plusMinutes(5)) + assertThat(harness.ticker.requestedUnits).containsExactly( + WorldClockDefaults.tickFor(showSeconds = false), + WorldClockDefaults.tickFor(showSeconds = false), + ) + } + /** ยง5.9 #20, with #23's seconds on. */ @Test fun `a second of wall clock moves the face and the rows when seconds are on`(@TempDir tempDir: Path) =