diff --git a/clients/android/app/src/main/kotlin/io/unom/punktfunk/SettingsScreen.kt b/clients/android/app/src/main/kotlin/io/unom/punktfunk/SettingsScreen.kt index 097bcbaa..8ff7167f 100644 --- a/clients/android/app/src/main/kotlin/io/unom/punktfunk/SettingsScreen.kt +++ b/clients/android/app/src/main/kotlin/io/unom/punktfunk/SettingsScreen.kt @@ -61,6 +61,7 @@ import androidx.compose.runtime.CompositionLocalProvider import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.compositionLocalOf import androidx.compose.runtime.getValue +import androidx.compose.runtime.key import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.saveable.rememberSaveable @@ -136,21 +137,38 @@ fun SettingsScreen( // row the profile doesn't override reads as the live global — and keeps following it. val s = globals.effectiveFor(active) + /** + * The scope an edit writes to, resolved AT THE EDIT rather than closed over at composition. + * + * [update] and [resetField] reach the rows as `::update` / `::resetField` — callable references, + * and two of those compare EQUAL however different the scope they captured. Compose therefore + * sees an unchanged callback and skips [CategoryDetail] on a scope switch that doesn't move any + * value on screen — which is the ordinary case, since a profile inherits the globals until it + * overrides something. The row then kept calling the reference it was first handed and wrote + * into the scope the user had just LEFT: edit a default, switch to a profile, edit the same row + * — the globals move again and the profile records nothing; switch back and the next edit lands + * on the profile. Reading the live state here is what makes the write follow the chips. + */ + fun scopeProfile(): StreamProfile? = scopeId?.let { id -> profiles.firstOrNull { it.id == id } } + fun update(next: Settings) { - val profile = active + val profile = scopeProfile() if (profile == null) { globals = next onChange(next) } else { // `absorb` against what the control was SHOWING, so picking today's global value is - // still recorded as an override (the pin) — see [SettingsOverlay.absorb]. - profileStore.save(profile.copy(overrides = profile.overrides.absorb(s, next))) + // still recorded as an override (the pin) — see [SettingsOverlay.absorb]. A skipped row + // shows what a fresh one would (that is WHY it skipped), so the effective settings + // recomputed here are the same ones it rendered. + val shown = globals.effectiveFor(profile) + profileStore.save(profile.copy(overrides = profile.overrides.absorb(shown, next))) profiles = profileStore.all() } } fun resetField(field: String) { - val profile = active ?: return + val profile = scopeProfile() ?: return profileStore.save(profile.copy(overrides = profile.overrides.clear(field))) profiles = profileStore.all() } @@ -231,16 +249,22 @@ fun SettingsScreen( } val detail: @Composable (SettingsCategory, (() -> Unit)?) -> Unit = { cat, back -> - CategoryDetail( - category = cat, - settings = s, - onChange = ::update, - context = context, - onMicChange = onMicChange, - onOpenControllers = { showControllers = true }, - onOpenLicenses = { showLicenses = true }, - onBack = back, - ) + // Keyed on the scope: switching chips rebuilds the page rather than recomposing + // it in place. Correctness no longer depends on it (see [scopeProfile]), but a + // row's own `remember` is per-scope state too — "Custom…" picked while editing + // a profile has no business still being picked over on the defaults. + key(active?.id) { + CategoryDetail( + category = cat, + settings = s, + onChange = ::update, + context = context, + onMicChange = onMicChange, + onOpenControllers = { showControllers = true }, + onOpenLicenses = { showLicenses = true }, + onBack = back, + ) + } } if (twoPane) { diff --git a/clients/android/app/src/test/kotlin/io/unom/punktfunk/SettingsScopeTest.kt b/clients/android/app/src/test/kotlin/io/unom/punktfunk/SettingsScopeTest.kt new file mode 100644 index 00000000..1c7e360a --- /dev/null +++ b/clients/android/app/src/test/kotlin/io/unom/punktfunk/SettingsScopeTest.kt @@ -0,0 +1,127 @@ +package io.unom.punktfunk + +import android.content.Context +import androidx.activity.ComponentActivity +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue +import androidx.compose.ui.test.isToggleable +import androidx.compose.ui.test.junit4.createAndroidComposeRule +import androidx.compose.ui.test.onNodeWithText +import androidx.compose.ui.test.performClick +import androidx.test.core.app.ApplicationProvider +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Rule +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.GraphicsMode +import org.robolectric.annotation.Config + +/** + * An edit must land in the scope the chips say is selected — the one thing the two-layer settings + * surface can get wrong without looking wrong. + * + * The regression this pins: `update` reached the rows as `::update`, and two callable references + * compare EQUAL however different the scope they captured, so Compose skipped the whole detail page + * on a scope switch that moved nothing on screen (the ordinary case — a profile inherits the globals + * until it overrides something). Each edit then wrote to the scope the user had just left: change a + * default, switch to a profile, change the same row — the globals moved again and the profile + * recorded nothing — and back on the defaults the next edit went into the profile, which reads as + * "the default settings can't be changed any more". It needs the real Compose runtime to catch, so + * this drives the actual screen rather than the model underneath it. + * + * `sdk = [36]` for the reason every Robolectric test here pins it: android-all jars stop at 36 while + * the app compiles against 37. + */ +@RunWith(RobolectricTestRunner::class) +@GraphicsMode(GraphicsMode.Mode.NATIVE) +@Config(sdk = [36], qualifiers = "w360dp-h800dp-xxhdpi") +class SettingsScopeTest { + @get:Rule + val compose = createAndroidComposeRule() + + private val context: Context get() = ApplicationProvider.getApplicationContext() + + /** + * "Invert scroll direction" is the row under test: Input is profileable end to end, and that + * toggle is the only toggleable node on the page, so a click needs no fragile lookup. + */ + private fun toggleTheRow() { + compose.onNode(isToggleable()).performClick() + compose.waitForIdle() + } + + private fun selectScope(chip: String) { + compose.onNodeWithText(chip).performClick() + compose.waitForIdle() + } + + @Test + fun editsFollowTheSelectedScope() { + val profiles = ProfileStore(context) + profiles.save(newProfile("Work", PROFILE_ACCENTS.first())) + + // Mirrors App.kt: the screen is fed from state the host recomposes it with. + var saved = Settings() + compose.setContent { + var settings by remember { mutableStateOf(saved) } + SettingsScreen( + initial = settings, + onChange = { settings = it; saved = it }, + onBack = {}, + initialCategory = SettingsCategory.Input, + ) + } + + // 1. On the defaults, the globals move and no profile records anything. + toggleTheRow() + assertEquals(true, saved.invertScroll) + assertNull(profiles.all().single().overrides.invertScroll) + + // 2. In profile scope the SAME row — untouched by the profile, so it still shows the global + // value and nothing on the page changed — must record an override and leave the globals + // alone. This is the step that used to write straight through to the globals. + selectScope("Work") + toggleTheRow() + assertEquals("the globals must not move while a profile is selected", true, saved.invertScroll) + assertEquals(false, profiles.all().single().overrides.invertScroll) + + // 3. Back on the defaults the row is editable again, and the profile keeps its override. + selectScope("Default settings") + toggleTheRow() + assertEquals(false, saved.invertScroll) + assertEquals( + "the profile's override must survive an edit made on the defaults", + false, + profiles.all().single().overrides.invertScroll, + ) + } + + /** A reset puts the row back to inheriting — and, like an edit, it must obey the live scope. */ + @Test + fun resetClearsTheSelectedProfilesOverride() { + val profiles = ProfileStore(context) + profiles.save(newProfile("Work", PROFILE_ACCENTS.first())) + + compose.setContent { + var settings by remember { mutableStateOf(Settings()) } + SettingsScreen( + initial = settings, + onChange = { settings = it }, + onBack = {}, + initialCategory = SettingsCategory.Input, + initialProfileId = profiles.all().single().id, + ) + } + + toggleTheRow() + assertEquals(true, profiles.all().single().overrides.invertScroll) + + compose.onNodeWithText("Reset").performClick() + compose.waitForIdle() + assertNull(profiles.all().single().overrides.invertScroll) + } +}