From 9ecba63de42d923f5c6aed481a00cde3a4a1a407 Mon Sep 17 00:00:00 2001 From: Adron Hall Date: Wed, 16 Sep 2026 14:04:59 -0700 Subject: [PATCH] feat(settings): sync the theme preference to the account MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dark mode was device-local: `ThemeSettingsStore` held the choice and `MainActivity` rendered from it, but nothing ever reached the account, so a user who picked dark on the web still landed in light on a fresh install. The account's `theme` field now participates two ways. `ThemeSettingsStore` becomes an interface (SharedPreferences impl, mirroring `LastSeenNotificationStore`) that additionally remembers whether the stored mode has reached the account. `SettingsRepository.setThemeMode` writes the device first and unconditionally, then PATCHes `theme` alone; `SettingsRepository.refresh` is the sync point that reconciles the two. The reconciliation rule is explicit rather than emergent: an unsynced local choice wins, otherwise the account wins. Neither side carries a timestamp, but an unsynced flag is happened-after evidence — the account's value is by construction whatever was there before a change that never got out. So a change made offline is pushed rather than silently undone, while a device with nothing pending adopts the account's choice, which is what lands a fresh install in the theme picked on the web. An absent `theme`, or one this app cannot render, changes nothing on either side. Wire values come from `/help/settings` ("Light, dark, or system (follows your device preference)"), because the API will not tell us: unlike `viewingPreference`, `theme` has no server-side validation at all — a PATCH of "sepia", or of "", is answered 200 and stored verbatim. Since the account has a real "follows the device" option, a local SYSTEM choice maps to "system" and is never coerced into light or dark. The control sits in the Settings **Profile** group, between where Avatar would be and the message character limit, matching the web's own ordering. It reads the device store rather than `settings.theme`, so it shows what the app is actually rendering. A failed save is the one on this screen that does not roll back — the app has already re-themed — and says so. `AccountThemeSyncEffect` runs the sync when the signed-in shell is entered, which covers both a cold start with a session and a fresh sign-in; without it the theme would only reconcile for users who opened Settings. Tests: `ThemeSyncTest` pins the rule and the wire mapping in both directions; `SettingsThemeSyncTest` drives the repository through MockWebServer (local change PATCHes `theme` alone, account value adopted at sign-in, an offline change survives — including across a process death — and syncs on reconnect, a pending change beats the account value, a failed push stays owed); `SettingsThemeTest` covers the screen state and the no-rollback behaviour. Closes #36 --- .../navigation/InterlinedListNavHost.kt | 5 + .../core/datastore/ThemeSettingsStore.kt | 68 +++- .../core/datastore/di/DataStoreModule.kt | 11 + .../feature/profile/ui/SettingsScreenTest.kt | 56 ++++ .../profile/data/DefaultSettingsRepository.kt | 95 +++++- .../profile/data/SettingsRepository.kt | 31 +- .../feature/profile/domain/ThemeSync.kt | 103 ++++++ .../profile/ui/settings/AccountThemeSync.kt | 48 +++ .../profile/ui/settings/SettingsScreen.kt | 58 +++- .../profile/ui/settings/SettingsViewModel.kt | 52 +++ .../data/DefaultSettingsRepositoryTest.kt | 10 +- .../profile/data/FakeThemeSettingsStore.kt | 37 +++ .../profile/data/SettingsThemeSyncTest.kt | 309 ++++++++++++++++++ .../feature/profile/domain/ThemeSyncTest.kt | 175 ++++++++++ .../profile/ui/FakeSettingsRepository.kt | 22 ++ .../feature/profile/ui/SettingsThemeTest.kt | 147 +++++++++ 16 files changed, 1197 insertions(+), 30 deletions(-) create mode 100644 feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/domain/ThemeSync.kt create mode 100644 feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/AccountThemeSync.kt create mode 100644 feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/data/FakeThemeSettingsStore.kt create mode 100644 feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/data/SettingsThemeSyncTest.kt create mode 100644 feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/domain/ThemeSyncTest.kt create mode 100644 feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/ui/SettingsThemeTest.kt diff --git a/app/src/main/java/com/interlinedlist/android/navigation/InterlinedListNavHost.kt b/app/src/main/java/com/interlinedlist/android/navigation/InterlinedListNavHost.kt index 8ae99df..754231f 100644 --- a/app/src/main/java/com/interlinedlist/android/navigation/InterlinedListNavHost.kt +++ b/app/src/main/java/com/interlinedlist/android/navigation/InterlinedListNavHost.kt @@ -79,6 +79,7 @@ import com.interlinedlist.android.feature.profile.ui.profile.PublicDocumentRoute import com.interlinedlist.android.feature.profile.ui.profile.PublicListRoute import com.interlinedlist.android.feature.profile.ui.profile.UserProfileRoute import com.interlinedlist.android.feature.profile.ui.search.UserSearchRoute +import com.interlinedlist.android.feature.profile.ui.settings.AccountThemeSyncEffect import com.interlinedlist.android.feature.profile.ui.settings.SettingsRoute import com.interlinedlist.android.ui.home.HomeViewModel @@ -212,6 +213,10 @@ fun InterlinedListNavHost( } composable(Routes.MAIN) { val context = LocalContext.current + // Entered both by a cold start with a session and by a fresh sign-in, so + // this is where the account's theme is adopted (and any change made + // offline is pushed) without waiting for the user to open Settings. + AccountThemeSyncEffect() MainShell( notificationRoute = notificationRoute, onLoggedOut = { diff --git a/core/datastore/src/main/kotlin/com/interlinedlist/android/core/datastore/ThemeSettingsStore.kt b/core/datastore/src/main/kotlin/com/interlinedlist/android/core/datastore/ThemeSettingsStore.kt index 32f4e9a..f284fd5 100644 --- a/core/datastore/src/main/kotlin/com/interlinedlist/android/core/datastore/ThemeSettingsStore.kt +++ b/core/datastore/src/main/kotlin/com/interlinedlist/android/core/datastore/ThemeSettingsStore.kt @@ -17,24 +17,71 @@ enum class ThemeMode { } /** - * Persists the user's appearance preference (System / Light / Dark) in plain - * SharedPreferences — it carries no secrets, so it does not need the encrypted - * session store. Exposes the current value as a [StateFlow] so the theme - * recomposes the moment the setting changes. + * Holds the device's appearance preference (System / Light / Dark) and remembers + * whether that preference has reached the account yet. + * + * The preference is *also* an account field (`theme` on `GET /api/user` / + * `PATCH /api/user/update`), so this store is deliberately the **device's** side of a + * two-way sync rather than a plain local setting: + * + * - [setThemeMode] records a choice the user made *here*. It takes effect + * immediately — online or not — and leaves [hasUnsyncedChange] true until the + * account confirms it, so a change made offline survives a process death and is + * still pushed on the next sync. + * - [setSyncedThemeMode] records the value the *account* holds, clearing the flag. + * It is used both when adopting the account's choice (fresh install, or changed on + * the web) and when a push of a local choice succeeds. + * + * Modelled behind an interface — like `LastSeenNotificationStore` — so the sync logic + * that drives it can be unit-tested against an in-memory fake with no Android + * dependency. The value carries no secrets, so the implementation uses plain + * SharedPreferences rather than the encrypted session store, and exposes the current + * mode as a [StateFlow] so the theme recomposes the moment it changes. */ +interface ThemeSettingsStore { + + /** The appearance currently in force on this device. */ + val themeMode: StateFlow + + /** + * True when [themeMode] holds a choice made on this device that the account has + * not confirmed yet — the flag that makes an offline change win the next + * reconciliation instead of being overwritten by the stale account value. + */ + val hasUnsyncedChange: Boolean + + /** Records a choice made on this device, pending a push to the account. */ + fun setThemeMode(mode: ThemeMode) + + /** Records [mode] as the value the account holds, clearing [hasUnsyncedChange]. */ + fun setSyncedThemeMode(mode: ThemeMode) +} + +/** SharedPreferences-backed [ThemeSettingsStore]. */ @Singleton -class ThemeSettingsStore @Inject constructor( +class SharedPrefsThemeSettingsStore @Inject constructor( @ApplicationContext context: Context, -) { +) : ThemeSettingsStore { + private val prefs: SharedPreferences = context.getSharedPreferences(PREFS_FILE, Context.MODE_PRIVATE) private val _themeMode = MutableStateFlow(readThemeMode()) - val themeMode: StateFlow = _themeMode.asStateFlow() + override val themeMode: StateFlow = _themeMode.asStateFlow() + + override val hasUnsyncedChange: Boolean + get() = prefs.getBoolean(KEY_UNSYNCED, false) + + override fun setThemeMode(mode: ThemeMode) = store(mode, unsynced = true) + + override fun setSyncedThemeMode(mode: ThemeMode) = store(mode, unsynced = false) - /** Persists [mode] and pushes it to observers immediately. */ - fun setThemeMode(mode: ThemeMode) { - prefs.edit().putString(KEY_THEME_MODE, mode.name).apply() + /** Persists [mode] plus its sync state and pushes it to observers immediately. */ + private fun store(mode: ThemeMode, unsynced: Boolean) { + prefs.edit() + .putString(KEY_THEME_MODE, mode.name) + .putBoolean(KEY_UNSYNCED, unsynced) + .apply() _themeMode.value = mode } @@ -46,5 +93,6 @@ class ThemeSettingsStore @Inject constructor( companion object { private const val PREFS_FILE = "il_settings.prefs" private const val KEY_THEME_MODE = "theme_mode" + private const val KEY_UNSYNCED = "theme_mode_unsynced" } } diff --git a/core/datastore/src/main/kotlin/com/interlinedlist/android/core/datastore/di/DataStoreModule.kt b/core/datastore/src/main/kotlin/com/interlinedlist/android/core/datastore/di/DataStoreModule.kt index 832dfca..1be0709 100644 --- a/core/datastore/src/main/kotlin/com/interlinedlist/android/core/datastore/di/DataStoreModule.kt +++ b/core/datastore/src/main/kotlin/com/interlinedlist/android/core/datastore/di/DataStoreModule.kt @@ -4,6 +4,8 @@ import android.content.Context import android.content.SharedPreferences import com.interlinedlist.android.core.common.session.SessionTokenProvider import com.interlinedlist.android.core.datastore.SessionStore +import com.interlinedlist.android.core.datastore.SharedPrefsThemeSettingsStore +import com.interlinedlist.android.core.datastore.ThemeSettingsStore import dagger.Binds import dagger.Module import dagger.Provides @@ -30,4 +32,13 @@ abstract class SessionBindsModule { @Binds abstract fun bindSessionTokenProvider(impl: SessionStore): SessionTokenProvider + + /** + * Singleton so the activity's theme observer and the settings sync that writes to + * it are the same store — otherwise an adopted account theme would not reach the + * running UI until the process restarted. + */ + @Binds + @Singleton + abstract fun bindThemeSettingsStore(impl: SharedPrefsThemeSettingsStore): ThemeSettingsStore } diff --git a/feature/profile/src/androidTest/kotlin/com/interlinedlist/android/feature/profile/ui/SettingsScreenTest.kt b/feature/profile/src/androidTest/kotlin/com/interlinedlist/android/feature/profile/ui/SettingsScreenTest.kt index 12361e1..8d84e3e 100644 --- a/feature/profile/src/androidTest/kotlin/com/interlinedlist/android/feature/profile/ui/SettingsScreenTest.kt +++ b/feature/profile/src/androidTest/kotlin/com/interlinedlist/android/feature/profile/ui/SettingsScreenTest.kt @@ -17,6 +17,8 @@ import androidx.compose.ui.test.performImeAction import androidx.compose.ui.test.performTextClearance import androidx.compose.ui.test.performTextInput import androidx.test.ext.junit.runners.AndroidJUnit4 +import com.google.common.truth.Truth.assertThat +import com.interlinedlist.android.core.datastore.ThemeMode import com.interlinedlist.android.core.designsystem.theme.InterlinedListTheme import com.interlinedlist.android.feature.profile.domain.UserSettings import com.interlinedlist.android.feature.profile.domain.ViewingPreference @@ -38,6 +40,7 @@ class SettingsScreenTest { private fun setContent( state: SettingsUiState, + onSelectThemeMode: (ThemeMode) -> Unit = {}, onSelectViewingPreference: (ViewingPreference) -> Unit = {}, onToggleShowPreviews: (Boolean) -> Unit = {}, onSetMessagesPerPage: (Int) -> Unit = {}, @@ -55,6 +58,7 @@ class SettingsScreenTest { state = state, onBack = {}, onRetry = onRetry, + onSelectThemeMode = onSelectThemeMode, onSelectViewingPreference = onSelectViewingPreference, onToggleShowPreviews = onToggleShowPreviews, onSetMessagesPerPage = onSetMessagesPerPage, @@ -367,6 +371,7 @@ class SettingsScreenTest { state = SettingsUiState(settings = settings, errorMessage = null), onBack = {}, onRetry = {}, + onSelectThemeMode = {}, onSelectViewingPreference = {}, onToggleShowPreviews = {}, onSetMessagesPerPage = {}, @@ -453,6 +458,7 @@ class SettingsScreenTest { state = SettingsUiState(settings = settings), onBack = {}, onRetry = {}, + onSelectThemeMode = {}, onSelectViewingPreference = {}, onToggleShowPreviews = {}, onSetMessagesPerPage = {}, @@ -475,4 +481,54 @@ class SettingsScreenTest { composeRule.onNodeWithTag(SettingsTestTags.PRIVATE_ACCOUNT).assertIsOff() } + + // --- Theme (issue #36) --------------------------------------------------- + // Filed under Profile because that is where the web files it: `/help/settings` + // lists Theme under "Profile settings", between Avatar and Max message length. + + @Test + fun theme_showsTheDeviceSelectionInsideTheProfileGroup() { + setContent(SettingsUiState(settings = UserSettings(), themeMode = ThemeMode.DARK)) + + composeRule.onNodeWithTag(SettingsTestTags.PROFILE).assertIsDisplayed() + composeRule.onNodeWithTag(SettingsTestTags.themeMode(ThemeMode.DARK)).assertIsSelected() + } + + /** + * The account field and the device store can disagree while an offline change is + * still owed; the control must show what the app is actually rendering. + */ + @Test + fun theme_prefersTheDeviceStoreOverTheAccountField() { + setContent( + SettingsUiState( + settings = UserSettings(theme = "light"), + themeMode = ThemeMode.DARK, + ), + ) + + composeRule.onNodeWithTag(SettingsTestTags.themeMode(ThemeMode.DARK)).assertIsSelected() + } + + @Test + fun choosingATheme_reportsTheSelection() { + var chosen: ThemeMode? = null + setContent( + SettingsUiState(settings = UserSettings(), themeMode = ThemeMode.SYSTEM), + onSelectThemeMode = { chosen = it }, + ) + + composeRule.onNodeWithTag(SettingsTestTags.themeMode(ThemeMode.DARK)).performClick() + + assertThat(chosen).isEqualTo(ThemeMode.DARK) + } + + /** "Follow the device" is an option the account carries too, so it is offered. */ + @Test + fun theme_offersFollowTheSystem() { + setContent(SettingsUiState(settings = UserSettings(), themeMode = ThemeMode.LIGHT)) + + composeRule.onNodeWithTag(SettingsTestTags.themeMode(ThemeMode.SYSTEM)).assertIsDisplayed() + composeRule.onNodeWithTag(SettingsTestTags.themeMode(ThemeMode.SYSTEM)).performClick() + } } diff --git a/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/data/DefaultSettingsRepository.kt b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/data/DefaultSettingsRepository.kt index c039cde..9c29119 100644 --- a/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/data/DefaultSettingsRepository.kt +++ b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/data/DefaultSettingsRepository.kt @@ -3,13 +3,18 @@ package com.interlinedlist.android.feature.profile.data import com.interlinedlist.android.core.common.dispatcher.DispatcherProvider import com.interlinedlist.android.core.common.result.ApiResult import com.interlinedlist.android.core.common.result.AppError +import com.interlinedlist.android.core.datastore.ThemeMode +import com.interlinedlist.android.core.datastore.ThemeSettingsStore import com.interlinedlist.android.core.network.error.safeApiCall import com.interlinedlist.android.core.network.preferences.NotificationTrayLimitStore import com.interlinedlist.android.feature.profile.data.mapper.toRequest import com.interlinedlist.android.feature.profile.data.mapper.toUserSettings import com.interlinedlist.android.feature.profile.data.remote.ProfileApi +import com.interlinedlist.android.feature.profile.domain.ThemeReconciliation import com.interlinedlist.android.feature.profile.domain.UserSettings import com.interlinedlist.android.feature.profile.domain.UserSettingsUpdate +import com.interlinedlist.android.feature.profile.domain.reconcileTheme +import com.interlinedlist.android.feature.profile.domain.wire import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.asStateFlow @@ -28,11 +33,18 @@ import javax.inject.Singleton * group by that preference and no feature module here may depend on another. Without * the forward, a limit changed in Settings would not take effect until the process * restarted. + * + * `theme` is the one preference that is **not** read straight off the account: the + * device's [ThemeSettingsStore] is what the app renders from, so the appearance holds + * with no connection, and [refresh] reconciles the two. See + * [com.interlinedlist.android.feature.profile.domain.reconcileTheme] for the rule that + * decides which side wins. */ @Singleton class DefaultSettingsRepository @Inject constructor( private val api: ProfileApi, private val trayLimitStore: NotificationTrayLimitStore, + private val themeStore: ThemeSettingsStore, private val json: Json, private val dispatchers: DispatcherProvider, ) : SettingsRepository { @@ -41,28 +53,85 @@ class DefaultSettingsRepository @Inject constructor( override fun observeSettings(): Flow = cached.asStateFlow() + override fun observeThemeMode(): Flow = themeStore.themeMode + override suspend fun refresh(): ApiResult = withContext(dispatchers.io) { + when (val result = fetch()) { + is ApiResult.Success -> ApiResult.Success(reconcileTheme(result.data)) + is ApiResult.Failure -> result + } + } + + override suspend fun setThemeMode(mode: ThemeMode): ApiResult = + withContext(dispatchers.io) { + // Local first, and regardless of what the network does next: the app must + // re-theme immediately and keep the choice while offline. The store marks + // it unsynced, which is what makes the next refresh push it rather than + // overwrite it with the account's stale value. + themeStore.setThemeMode(mode) + patch(UserSettingsUpdate(theme = mode.wire)).also { result -> + if (result is ApiResult.Success) themeStore.setSyncedThemeMode(mode) + } + } + + override suspend fun update(update: UserSettingsUpdate): ApiResult = + withContext(dispatchers.io) { patch(update) } + + /** `GET /api/user` into the cache, with no theme reconciliation. */ + private suspend fun fetch(): ApiResult = when (val result = safeApiCall(json) { api.getCurrentUser().userOrSelf }) { + is ApiResult.Success -> result.data + ?.let { ApiResult.Success(publish(it.toUserSettings())) } + ?: ApiResult.Failure(AppError.Unknown("No user in response")) + is ApiResult.Failure -> result + } + + /** + * `PATCH /api/user/update` into the cache. Falls back to [fetch] — not [refresh] — + * when the endpoint echoes a thin body, so a push issued *by* reconciliation can + * never loop back into reconciliation. + */ + private suspend fun patch(update: UserSettingsUpdate): ApiResult = + when (val result = safeApiCall(json) { api.updateProfile(update.toRequest()).userOrSelf }) { is ApiResult.Success -> { + // The endpoint echoes the full updated user; if a thin body comes + // back instead, re-read so the cache still holds server truth. val dto = result.data - ?: return@withContext ApiResult.Failure(AppError.Unknown("No user in response")) - ApiResult.Success(publish(dto.toUserSettings())) + if (dto != null) ApiResult.Success(publish(dto.toUserSettings())) else fetch() } is ApiResult.Failure -> result } - } - override suspend fun update(update: UserSettingsUpdate): ApiResult = - withContext(dispatchers.io) { - when (val result = safeApiCall(json) { api.updateProfile(update.toRequest()).userOrSelf }) { - is ApiResult.Success -> { - // The endpoint echoes the full updated user; if a thin body comes - // back instead, re-read so the cache still holds server truth. - val dto = result.data - if (dto != null) ApiResult.Success(publish(dto.toUserSettings())) else refresh() - } - is ApiResult.Failure -> result + /** + * Brings the device's appearance and the account's `theme` back into agreement and + * answers with the settings that hold afterwards (a successful push re-reads them). + * + * A failed push is swallowed on purpose: the refresh that triggered it still + * succeeded, and the choice stays marked unsynced so the next refresh tries again. + */ + private suspend fun reconcileTheme(settings: UserSettings): UserSettings = + when ( + val outcome = reconcileTheme( + local = themeStore.themeMode.value, + hasUnsyncedLocalChange = themeStore.hasUnsyncedChange, + accountTheme = settings.theme, + ) + ) { + ThemeReconciliation.InSync -> settings + + is ThemeReconciliation.AdoptAccount -> { + themeStore.setSyncedThemeMode(outcome.mode) + settings } + + is ThemeReconciliation.PushLocal -> + when (val pushed = patch(UserSettingsUpdate(theme = outcome.mode.wire))) { + is ApiResult.Success -> { + themeStore.setSyncedThemeMode(outcome.mode) + pushed.data + } + is ApiResult.Failure -> settings + } } private fun publish(settings: UserSettings): UserSettings { diff --git a/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/data/SettingsRepository.kt b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/data/SettingsRepository.kt index cd89518..759b6b6 100644 --- a/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/data/SettingsRepository.kt +++ b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/data/SettingsRepository.kt @@ -1,6 +1,7 @@ package com.interlinedlist.android.feature.profile.data import com.interlinedlist.android.core.common.result.ApiResult +import com.interlinedlist.android.core.datastore.ThemeMode import com.interlinedlist.android.feature.profile.domain.UserSettings import com.interlinedlist.android.feature.profile.domain.UserSettingsUpdate import kotlinx.coroutines.flow.Flow @@ -20,9 +21,37 @@ interface SettingsRepository { /** The last known settings, or null until the first successful [refresh]. */ fun observeSettings(): Flow - /** Re-reads the settings from `GET /api/user` and publishes them to [observeSettings]. */ + /** + * Re-reads the settings from `GET /api/user` and publishes them to [observeSettings]. + * + * This is also the app's **theme sync point**: after the account's values land, the + * device's appearance is reconciled against the account's `theme` by + * [com.interlinedlist.android.feature.profile.domain.reconcileTheme] — adopting the + * account's choice when this device has nothing unsynced (so a fresh install lands + * in the theme picked on the web), and pushing a choice made offline when it does. + * A failed push leaves the choice pending for the next refresh; it is never lost. + */ suspend fun refresh(): ApiResult + /** + * The appearance in force on this device — the local store, which is the source of + * truth for what the app renders whether or not the account is reachable. + * + * Read from here rather than from [UserSettings.theme]: the account field is the + * last value the *server* knew, which an offline change deliberately outruns. + */ + fun observeThemeMode(): Flow + + /** + * Applies a theme chosen on this device. + * + * The local store is written **first and unconditionally**, so the app re-themes at + * once and keeps the choice with no connection; the account is then PATCHed with + * `theme` alone. A failure is reported so the UI can say the change has not synced + * yet, but it does **not** roll the choice back — the next [refresh] pushes it. + */ + suspend fun setThemeMode(mode: ThemeMode): ApiResult + /** * Applies a partial [update] via `PATCH /api/user/update`. Fields left null in * [update] are omitted from the request, so other preferences are untouched. diff --git a/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/domain/ThemeSync.kt b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/domain/ThemeSync.kt new file mode 100644 index 0000000..1b356f6 --- /dev/null +++ b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/domain/ThemeSync.kt @@ -0,0 +1,103 @@ +package com.interlinedlist.android.feature.profile.domain + +import com.interlinedlist.android.core.datastore.ThemeMode + +/** + * The account's wire value for this appearance — the `theme` field of `GET /api/user` + * and `PATCH /api/user/update`. + * + * The values come from the help centre, because the API will not tell us: `theme` is a + * plain Prisma string column with **no server-side validation at all** (a PATCH of + * `"sepia"`, or of `""`, is answered 200 and stored verbatim), so unlike + * `viewingPreference` there is no allow-list to coax out of a 400. `/help/settings` + * publishes the web's own vocabulary instead: "Theme: Light, dark, or system (follows + * your device preference)". Live `GET /api/user` returns it lower-case (`"light"`). + * + * Two consequences worth stating: + * - The account **does** have a "follows the device" option, so a local [ThemeMode.SYSTEM] + * maps straight onto `"system"`. It is never coerced into light or dark, and a user + * who follows their phone's theme keeps doing so after a sync. + * - Because the server validates nothing, this app must be the conservative side: it + * sends only these three values and refuses to adopt anything else. + */ +val ThemeMode.wire: String + get() = when (this) { + ThemeMode.SYSTEM -> "system" + ThemeMode.LIGHT -> "light" + ThemeMode.DARK -> "dark" + } + +/** + * Parses an account `theme` value, returning null when it is missing or is something + * this app cannot render. + * + * Deliberately tolerant about casing and whitespace, and about `"auto"` as the obvious + * synonym for "follow the device" — but deliberately *intolerant* about everything + * else. Since the server stores any string, a null here is a real possibility, and + * "leave the device's theme alone" is a far better answer to an unknown value than + * guessing at light. + */ +fun themeModeFromWire(value: String?): ThemeMode? = + when (value?.trim()?.lowercase()) { + "system", "auto" -> ThemeMode.SYSTEM + "light" -> ThemeMode.LIGHT + "dark" -> ThemeMode.DARK + else -> null + } + +/** What reconciling the device's theme against the account's `theme` should do. */ +sealed interface ThemeReconciliation { + + /** The two already agree, or neither side has anything to offer. Do nothing. */ + data object InSync : ThemeReconciliation + + /** Store [mode] as the device's theme and record the account as holding it. */ + data class AdoptAccount(val mode: ThemeMode) : ThemeReconciliation + + /** PATCH [mode] to the account; the device keeps it either way. */ + data class PushLocal(val mode: ThemeMode) : ThemeReconciliation +} + +/** + * Decides which side wins when the device's theme and the account's disagree. + * + * **The rule: an unsynced local choice wins; otherwise the account wins.** + * + * Neither side carries a modification timestamp, so "newest wins" is not available. + * What *is* available is whether this device is holding a choice that never reached + * the account ([hasUnsyncedLocalChange], set by `ThemeSettingsStore.setThemeMode` and + * cleared only when the account confirms the value). That flag is exactly the + * happened-after evidence the rule needs: + * + * - **Unsynced local change → push it.** The account's value cannot be newer: it is + * whatever was there *before* the user made this change, because the change never + * got out. Adopting the account here would silently undo an edit the user made + * offline — the failure mode the issue calls out. The one exception is when the + * account already holds the same mode, where pushing would be a pointless write, so + * the flag is simply cleared instead. + * - **No unsynced local change → adopt the account.** Everything this device knows + * has already been pushed, so any difference came from somewhere else (the web, or + * another device) and is newer. This is also what makes a fresh install land in the + * theme chosen on the web: the local default is untouched, so the account wins. + * - **Nothing usable on the account → leave both alone.** `theme` may be absent, and + * because the server validates nothing it may hold a value this app cannot render. + * Neither is worth acting on: we do not adopt what we cannot show, and we do not + * overwrite it with a default the user never chose. + * + * @param local the appearance currently in force on this device. + * @param hasUnsyncedLocalChange whether [local] is a choice the account has not confirmed. + * @param accountTheme the raw `theme` value from `GET /api/user`. + */ +fun reconcileTheme( + local: ThemeMode, + hasUnsyncedLocalChange: Boolean, + accountTheme: String?, +): ThemeReconciliation { + val account = themeModeFromWire(accountTheme) + return when { + hasUnsyncedLocalChange && account == local -> ThemeReconciliation.AdoptAccount(local) + hasUnsyncedLocalChange -> ThemeReconciliation.PushLocal(local) + account == null || account == local -> ThemeReconciliation.InSync + else -> ThemeReconciliation.AdoptAccount(account) + } +} diff --git a/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/AccountThemeSync.kt b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/AccountThemeSync.kt new file mode 100644 index 0000000..0a0dd6a --- /dev/null +++ b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/AccountThemeSync.kt @@ -0,0 +1,48 @@ +package com.interlinedlist.android.feature.profile.ui.settings + +import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect +import androidx.hilt.navigation.compose.hiltViewModel +import androidx.lifecycle.ViewModel +import androidx.lifecycle.viewModelScope +import com.interlinedlist.android.feature.profile.data.SettingsRepository +import dagger.hilt.android.lifecycle.HiltViewModel +import kotlinx.coroutines.launch +import javax.inject.Inject + +/** + * Runs the account-preference sync for the signed-in shell. + * + * This is what makes "adopt the account value on first sign-in" actually happen: + * without it the theme would only reconcile when the user happened to open Settings, + * which is precisely the screen they would not open if the app already looked right + * on the web. + */ +@HiltViewModel +class AccountThemeSyncViewModel @Inject constructor( + private val repository: SettingsRepository, +) : ViewModel() { + + /** + * Reads the account's preferences and reconciles the device's theme with them. + * + * The result is deliberately ignored: this runs behind whatever screen the user + * landed on, so a failure must stay silent — the device keeps rendering its own + * stored theme, and anything it still owes the account is pushed by the next sync. + */ + fun sync() { + viewModelScope.launch { repository.refresh() } + } +} + +/** + * Syncs the account's theme into the app once, when the signed-in shell is entered. + * + * Placed at the shell rather than in the activity because the shell is entered in both + * cases that matter — a cold start that already had a session, and the navigation that + * follows a fresh sign-in — whereas the activity is only created in the first. + */ +@Composable +fun AccountThemeSyncEffect(viewModel: AccountThemeSyncViewModel = hiltViewModel()) { + LaunchedEffect(Unit) { viewModel.sync() } +} diff --git a/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/SettingsScreen.kt b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/SettingsScreen.kt index e89efd1..3df7ead 100644 --- a/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/SettingsScreen.kt +++ b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/SettingsScreen.kt @@ -33,6 +33,7 @@ import androidx.compose.ui.tooling.preview.Preview import androidx.compose.ui.unit.dp import androidx.hilt.navigation.compose.hiltViewModel import androidx.lifecycle.compose.collectAsStateWithLifecycle +import com.interlinedlist.android.core.datastore.ThemeMode import com.interlinedlist.android.core.designsystem.theme.InterlinedListTheme import com.interlinedlist.android.feature.profile.domain.SettingsBounds import com.interlinedlist.android.feature.profile.domain.UserSettings @@ -43,6 +44,7 @@ import com.interlinedlist.android.feature.profile.domain.maxMessageLengthOrDefau import com.interlinedlist.android.feature.profile.domain.messagesPerPageOrDefault import com.interlinedlist.android.feature.profile.domain.notificationTrayLimitOrDefault import com.interlinedlist.android.feature.profile.domain.showAdvancedPostSettingsOrDefault +import com.interlinedlist.android.feature.profile.domain.wire /** Stable test tags for the Settings screen. */ object SettingsTestTags { @@ -67,6 +69,23 @@ object SettingsTestTags { /** Tag for one feed-filter option, keyed on its wire value. */ fun viewingPreference(option: ViewingPreference): String = "settingsViewingPreference_${option.wire}" + + /** Tag for one appearance option, keyed on its account wire value. */ + fun themeMode(option: ThemeMode): String = "settingsTheme_${option.wire}" +} + +/** The label shown for each appearance (wording follows the web help centre). */ +private fun ThemeMode.label(): String = when (this) { + ThemeMode.SYSTEM -> "System" + ThemeMode.LIGHT -> "Light" + ThemeMode.DARK -> "Dark" +} + +/** The one-line explanation shown under each appearance. */ +private fun ThemeMode.description(): String = when (this) { + ThemeMode.SYSTEM -> "Follow your device's light or dark setting" + ThemeMode.LIGHT -> "Always use the light appearance" + ThemeMode.DARK -> "Always use the dark appearance" } /** The label shown for each feed filter (wording follows the web help centre). */ @@ -103,6 +122,7 @@ fun SettingsRoute( state = state, onBack = onBack, onRetry = viewModel::refresh, + onSelectThemeMode = viewModel::setThemeMode, onSelectViewingPreference = viewModel::setViewingPreference, onToggleShowPreviews = viewModel::setShowPreviews, onSetMessagesPerPage = viewModel::setMessagesPerPage, @@ -123,6 +143,7 @@ fun SettingsScreen( state: SettingsUiState, onBack: () -> Unit, onRetry: () -> Unit, + onSelectThemeMode: (ThemeMode) -> Unit, onSelectViewingPreference: (ViewingPreference) -> Unit, onToggleShowPreviews: (Boolean) -> Unit, onSetMessagesPerPage: (Int) -> Unit, @@ -160,6 +181,8 @@ fun SettingsScreen( } ProfileGroup( settings = settings, + themeMode = state.themeMode, + onSelectThemeMode = onSelectThemeMode, onSetMaxMessageLength = onSetMaxMessageLength, ) ViewPreferencesGroup( @@ -274,16 +297,23 @@ private fun ViewPreferencesGroup( } /** - * "Profile": the account-wide message character limit. + * "Profile": the app's appearance and the account-wide message character limit. + * + * Both are filed here because the web files them here: `/help/settings` lists + * **Profile settings** as Display Name, Bio, Avatar, "Theme: Light, dark, or system + * (follows your device preference)", then "Max message length", and the help centre + * tells users to "adjust it in Settings, then Profile (not Message Settings)". Theme + * sits above the limit in the same order the web uses. * - * The web deliberately files the limit here and not under Message settings — its own - * help centre tells users to "adjust it in Settings, then Profile (not Message - * Settings)" — so this mirrors that. Message settings points at it for anyone who - * looks there first. + * The theme is read from [themeMode] — the device's own store — rather than from + * `settings.theme`, so the selection shows what the app is actually rendering even + * when the account has not caught up yet. */ @Composable private fun ProfileGroup( settings: UserSettings, + themeMode: ThemeMode, + onSelectThemeMode: (ThemeMode) -> Unit, onSetMaxMessageLength: (Int) -> Unit, ) { SettingsGroup( @@ -291,6 +321,22 @@ private fun ProfileGroup( description = "Your display name, bio and avatar are edited from Edit profile.", modifier = Modifier.testTag(SettingsTestTags.PROFILE), ) { + Text( + text = "Theme", + style = MaterialTheme.typography.bodyLarge, + color = MaterialTheme.colorScheme.onSurface, + modifier = Modifier.padding(horizontal = 24.dp, vertical = 4.dp), + ) + ThemeMode.entries.forEach { option -> + SettingsRadioRow( + label = option.label(), + description = option.description(), + selected = themeMode == option, + onSelect = { onSelectThemeMode(option) }, + tag = SettingsTestTags.themeMode(option), + ) + } + HorizontalDivider(color = MaterialTheme.colorScheme.outlineVariant) SettingsNumberRow( label = "Message character limit", description = "The longest message you can post (default 666 characters).", @@ -431,9 +477,11 @@ private fun SettingsScreenPreview() { showAdvancedPostSettings = false, isPrivateAccount = true, ), + themeMode = ThemeMode.DARK, ), onBack = {}, onRetry = {}, + onSelectThemeMode = {}, onSelectViewingPreference = {}, onToggleShowPreviews = {}, onSetMessagesPerPage = {}, diff --git a/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/SettingsViewModel.kt b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/SettingsViewModel.kt index 4fa483c..e67bf39 100644 --- a/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/SettingsViewModel.kt +++ b/feature/profile/src/main/kotlin/com/interlinedlist/android/feature/profile/ui/settings/SettingsViewModel.kt @@ -3,6 +3,7 @@ package com.interlinedlist.android.feature.profile.ui.settings import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import com.interlinedlist.android.core.common.result.ApiResult +import com.interlinedlist.android.core.datastore.ThemeMode import com.interlinedlist.android.feature.profile.data.SettingsRepository import com.interlinedlist.android.feature.profile.domain.SettingsBounds import com.interlinedlist.android.feature.profile.domain.UserSettings @@ -27,9 +28,16 @@ import javax.inject.Inject * Settings screen state. [settings] is null until the first load succeeds (the screen * shows the loading/error state then); [isSaving] covers an in-flight preference write, * which the screen uses to keep the controls responsive but visibly pending. + * + * [themeMode] is carried separately from [settings] on purpose: the appearance the app + * renders comes from the **device's** store, which an offline change deliberately + * outruns, while `settings.theme` is only the last value the server knew. Showing the + * account field here would make the control flicker back to the stale value every time + * the settings refreshed. */ data class SettingsUiState( val settings: UserSettings? = null, + val themeMode: ThemeMode = ThemeMode.SYSTEM, val isLoading: Boolean = false, val isSaving: Boolean = false, val errorMessage: String? = null, @@ -57,6 +65,13 @@ class SettingsViewModel @Inject constructor( if (settings != null) _uiState.update { it.copy(settings = settings) } } } + // The device's theme is its own source of truth, so follow it directly rather + // than deriving it from the account field the settings carry. + viewModelScope.launch { + repository.observeThemeMode().collect { mode -> + _uiState.update { it.copy(themeMode = mode) } + } + } refresh() } @@ -75,6 +90,34 @@ class SettingsViewModel @Inject constructor( } } + /** + * Switches the app's appearance. + * + * Unlike every other preference here this is **not** rolled back when the save + * fails: the device's store already holds the choice, the app has already + * re-themed, and undoing that because the network was unavailable is exactly the + * behaviour the offline requirement rules out. + * + * Because the outcome differs from the other rows, so does the message — the + * server's reason plus [THEME_NOT_SYNCED_NOTE], so the user is told that the + * appearance they are looking at is real and that only the account copy is behind. + */ + fun setThemeMode(mode: ThemeMode) { + if (_uiState.value.themeMode == mode) return + _uiState.update { it.copy(isSaving = true, errorMessage = null) } + viewModelScope.launch { + when (val result = repository.setThemeMode(mode)) { + is ApiResult.Success -> _uiState.update { it.copy(isSaving = false) } + is ApiResult.Failure -> _uiState.update { + it.copy( + isSaving = false, + errorMessage = "${result.error.toUserMessage()} $THEME_NOT_SYNCED_NOTE", + ) + } + } + } + } + /** Switches which messages the Home feed shows. */ fun setViewingPreference(preference: ViewingPreference) { val current = _uiState.value.settings ?: return @@ -201,6 +244,15 @@ class SettingsViewModel @Inject constructor( fun dismissError() = _uiState.update { it.copy(errorMessage = null) } + companion object { + /** + * Appended to a failed theme save. The choice is kept and the app has already + * re-themed, so the message must not read like the change was lost. + */ + const val THEME_NOT_SYNCED_NOTE: String = + "Your theme is applied on this device and will sync to your account later." + } + /** * True when [value] is inside [range]; otherwise reports it as an error naming * the bounds and returns false, leaving the stored value alone. diff --git a/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/data/DefaultSettingsRepositoryTest.kt b/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/data/DefaultSettingsRepositoryTest.kt index 41b237d..1d02dc2 100644 --- a/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/data/DefaultSettingsRepositoryTest.kt +++ b/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/data/DefaultSettingsRepositoryTest.kt @@ -57,7 +57,15 @@ class DefaultSettingsRepositoryTest { api = retrofit.create(ProfileApi::class.java) trayLimitStore = NotificationTrayLimitStore(retrofit.create(InterlinedListApi::class.java), json) - repository = DefaultSettingsRepository(api, trayLimitStore, json, dispatchers) + // The theme half of the settings surface has its own suite + // (SettingsThemeSyncTest); here it only has to exist and not interfere. + repository = DefaultSettingsRepository( + api, + trayLimitStore, + FakeThemeSettingsStore(), + json, + dispatchers, + ) } @After diff --git a/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/data/FakeThemeSettingsStore.kt b/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/data/FakeThemeSettingsStore.kt new file mode 100644 index 0000000..3dc748e --- /dev/null +++ b/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/data/FakeThemeSettingsStore.kt @@ -0,0 +1,37 @@ +package com.interlinedlist.android.feature.profile.data + +import com.interlinedlist.android.core.datastore.ThemeMode +import com.interlinedlist.android.core.datastore.ThemeSettingsStore +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow + +/** + * In-memory [ThemeSettingsStore] with the same semantics as the SharedPreferences + * one: [setThemeMode] marks the value unsynced, [setSyncedThemeMode] clears the flag. + * + * @param initial the appearance already stored on the "device". + * @param unsynced whether [initial] is a choice the account has not confirmed — + * i.e. what survives a process death after a change made offline. + */ +class FakeThemeSettingsStore( + initial: ThemeMode = ThemeMode.SYSTEM, + unsynced: Boolean = false, +) : ThemeSettingsStore { + + private val mode = MutableStateFlow(initial) + + override val themeMode: StateFlow = mode + + override var hasUnsyncedChange: Boolean = unsynced + private set + + override fun setThemeMode(mode: ThemeMode) { + this.mode.value = mode + hasUnsyncedChange = true + } + + override fun setSyncedThemeMode(mode: ThemeMode) { + this.mode.value = mode + hasUnsyncedChange = false + } +} diff --git a/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/data/SettingsThemeSyncTest.kt b/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/data/SettingsThemeSyncTest.kt new file mode 100644 index 0000000..2a0d8ce --- /dev/null +++ b/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/data/SettingsThemeSyncTest.kt @@ -0,0 +1,309 @@ +package com.interlinedlist.android.feature.profile.data + +import com.google.common.truth.Truth.assertThat +import com.interlinedlist.android.core.common.dispatcher.DispatcherProvider +import com.interlinedlist.android.core.common.result.ApiResult +import com.interlinedlist.android.core.datastore.ThemeMode +import com.interlinedlist.android.core.network.api.InterlinedListApi +import com.interlinedlist.android.core.network.preferences.NotificationTrayLimitStore +import com.interlinedlist.android.feature.profile.data.remote.ProfileApi +import com.jakewharton.retrofit2.converter.kotlinx.serialization.asConverterFactory +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.runTest +import kotlinx.serialization.json.Json +import kotlinx.serialization.json.JsonObject +import kotlinx.serialization.json.jsonPrimitive +import okhttp3.MediaType.Companion.toMediaType +import okhttp3.OkHttpClient +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import okhttp3.mockwebserver.RecordedRequest +import org.junit.After +import org.junit.Before +import org.junit.Test +import retrofit2.Retrofit + +/** + * Issue #36: the theme preference synced to the account, driven end to end through the + * real Retrofit stack against a MockWebServer. + * + * What is pinned here is the *contract between the two stores*: the device's + * `ThemeSettingsStore` is what the app renders from and survives with no connection, + * the account's `theme` is what a second device sees, and `refresh()` is the sync point + * that reconciles them. The rule itself is tested in isolation by `ThemeSyncTest`; this + * covers that the repository actually applies it, and that a push carries `theme` alone. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class SettingsThemeSyncTest { + + private lateinit var server: MockWebServer + private lateinit var repository: DefaultSettingsRepository + private lateinit var themeStore: FakeThemeSettingsStore + + private val json = Json { ignoreUnknownKeys = true; explicitNulls = false } + private val testDispatcher = StandardTestDispatcher() + private val dispatchers = object : DispatcherProvider { + override val io: CoroutineDispatcher = testDispatcher + override val default: CoroutineDispatcher = testDispatcher + override val main: CoroutineDispatcher = testDispatcher + } + + @Before + fun setUp() { + server = MockWebServer() + server.start() + } + + @After + fun tearDown() = server.shutdown() + + /** Builds the repository over a "device" that already holds [local]. */ + private fun repositoryWithLocalTheme(local: ThemeMode, unsynced: Boolean = false) { + themeStore = FakeThemeSettingsStore(initial = local, unsynced = unsynced) + val retrofit = Retrofit.Builder() + .baseUrl(server.url("/")) + .client(OkHttpClient.Builder().build()) + .addConverterFactory(json.asConverterFactory("application/json".toMediaType())) + .build() + repository = DefaultSettingsRepository( + api = retrofit.create(ProfileApi::class.java), + trayLimitStore = NotificationTrayLimitStore( + retrofit.create(InterlinedListApi::class.java), + json, + ), + themeStore = themeStore, + json = json, + dispatchers = dispatchers, + ) + } + + /** A `GET /api/user` (or PATCH echo) whose account theme is [theme]. */ + private fun enqueueUser(theme: String?) = server.enqueue( + MockResponse().setResponseCode(200).setBody( + """ + { + "user": { + "id": "u1", + "username": "adron", + "theme": ${theme?.let { "\"$it\"" } ?: "null"}, + "viewingPreference": "all_messages", + "showPreviews": true + } + } + """.trimIndent(), + ), + ) + + private fun enqueueOffline() = + server.enqueue(MockResponse().setSocketPolicy(okhttp3.mockwebserver.SocketPolicy.DISCONNECT_AT_START)) + + private fun RecordedRequest.jsonBody() = + json.parseToJsonElement(body.readUtf8()) as JsonObject + + // --- Local change -> PATCH ------------------------------------------------ + + @Test + fun `choosing a theme stores it locally and PATCHes theme alone`() = runTest(testDispatcher) { + repositoryWithLocalTheme(ThemeMode.SYSTEM) + enqueueUser(theme = "dark") + + val result = repository.setThemeMode(ThemeMode.DARK) + + assertThat(result).isInstanceOf(ApiResult.Success::class.java) + assertThat(repository.observeThemeMode().first()).isEqualTo(ThemeMode.DARK) + assertThat(themeStore.hasUnsyncedChange).isFalse() + + val recorded = server.takeRequest() + assertThat(recorded.method).isEqualTo("PATCH") + assertThat(recorded.path).isEqualTo("/api/user/update") + val body = recorded.jsonBody() + assertThat(body.keys).containsExactly("theme") + assertThat(body.getValue("theme").jsonPrimitive.content).isEqualTo("dark") + } + + /** + * "Follow the device" is a real account value (`/help/settings`: "Light, dark, or + * system"), so it is pushed as `system` rather than coerced into light or dark. + */ + @Test + fun `choosing system pushes system rather than coercing it`() = runTest(testDispatcher) { + repositoryWithLocalTheme(ThemeMode.DARK) + enqueueUser(theme = "system") + + repository.setThemeMode(ThemeMode.SYSTEM) + + val body = server.takeRequest().jsonBody() + assertThat(body.keys).containsExactly("theme") + assertThat(body.getValue("theme").jsonPrimitive.content).isEqualTo("system") + assertThat(repository.observeThemeMode().first()).isEqualTo(ThemeMode.SYSTEM) + } + + // --- Account value applied at sign-in ------------------------------------- + + /** + * The issue's headline case: dark was chosen on the web, this install has never + * had a theme picked on it, so the first refresh after signing in adopts dark — + * without PATCHing anything back. + */ + @Test + fun `a fresh install adopts the account theme on the first refresh`() = runTest(testDispatcher) { + repositoryWithLocalTheme(ThemeMode.SYSTEM) + enqueueUser(theme = "dark") + + val result = repository.refresh() + + assertThat(result).isInstanceOf(ApiResult.Success::class.java) + assertThat(repository.observeThemeMode().first()).isEqualTo(ThemeMode.DARK) + assertThat(themeStore.hasUnsyncedChange).isFalse() + assertThat(server.requestCount).isEqualTo(1) + assertThat(server.takeRequest().method).isEqualTo("GET") + } + + @Test + fun `a theme changed elsewhere is adopted on the next refresh`() = runTest(testDispatcher) { + repositoryWithLocalTheme(ThemeMode.DARK) + enqueueUser(theme = "light") + + repository.refresh() + + assertThat(repository.observeThemeMode().first()).isEqualTo(ThemeMode.LIGHT) + assertThat(server.requestCount).isEqualTo(1) + } + + /** + * The server stores any string for `theme` (a PATCH of "sepia" is answered 200), so + * a value this app cannot render really can come back. It must neither be adopted + * nor overwritten. + */ + @Test + fun `an unrenderable account theme leaves the device alone`() = runTest(testDispatcher) { + repositoryWithLocalTheme(ThemeMode.DARK) + enqueueUser(theme = "sepia") + + repository.refresh() + + assertThat(repository.observeThemeMode().first()).isEqualTo(ThemeMode.DARK) + assertThat(server.requestCount).isEqualTo(1) + } + + @Test + fun `an account with no theme leaves the device alone`() = runTest(testDispatcher) { + repositoryWithLocalTheme(ThemeMode.LIGHT) + enqueueUser(theme = null) + + repository.refresh() + + assertThat(repository.observeThemeMode().first()).isEqualTo(ThemeMode.LIGHT) + assertThat(server.requestCount).isEqualTo(1) + } + + // --- Offline change survives and syncs on reconnect ----------------------- + + @Test + fun `a change made offline keeps the theme and stays pending`() = runTest(testDispatcher) { + repositoryWithLocalTheme(ThemeMode.LIGHT) + enqueueOffline() + + val result = repository.setThemeMode(ThemeMode.DARK) + + // The save is reported as failed... + assertThat(result).isInstanceOf(ApiResult.Failure::class.java) + // ...but the choice is not rolled back, and it is still owed to the account. + assertThat(repository.observeThemeMode().first()).isEqualTo(ThemeMode.DARK) + assertThat(themeStore.hasUnsyncedChange).isTrue() + } + + @Test + fun `the change made offline syncs on the next refresh`() = runTest(testDispatcher) { + repositoryWithLocalTheme(ThemeMode.LIGHT) + enqueueOffline() + repository.setThemeMode(ThemeMode.DARK) + server.takeRequest() + + // Back online. The account still holds the pre-offline value. + enqueueUser(theme = "light") + enqueueUser(theme = "dark") + val result = repository.refresh() + + assertThat(result).isInstanceOf(ApiResult.Success::class.java) + assertThat((result as ApiResult.Success).data.theme).isEqualTo("dark") + assertThat(repository.observeThemeMode().first()).isEqualTo(ThemeMode.DARK) + assertThat(themeStore.hasUnsyncedChange).isFalse() + + assertThat(server.takeRequest().method).isEqualTo("GET") + val push = server.takeRequest() + assertThat(push.method).isEqualTo("PATCH") + // The body is a one-shot buffer, so read it once and assert on that. + val body = push.jsonBody() + assertThat(body.keys).containsExactly("theme") + assertThat(body.getValue("theme").jsonPrimitive.content).isEqualTo("dark") + } + + /** + * The offline change survives a process death: the device's store is re-read with + * the pending flag still set, so the very first sync of the next launch pushes it. + */ + @Test + fun `an offline change that outlived the process still pushes`() = runTest(testDispatcher) { + repositoryWithLocalTheme(ThemeMode.DARK, unsynced = true) + enqueueUser(theme = "light") + enqueueUser(theme = "dark") + + repository.refresh() + + assertThat(repository.observeThemeMode().first()).isEqualTo(ThemeMode.DARK) + assertThat(themeStore.hasUnsyncedChange).isFalse() + assertThat(server.takeRequest().method).isEqualTo("GET") + assertThat(server.takeRequest().method).isEqualTo("PATCH") + } + + /** + * The reconciliation rule in the direction that matters most: a pending local + * change is never overwritten by the account's stale value, even though the very + * same call adopts the account's value when nothing is pending. + */ + @Test + fun `a pending local change wins over the account value`() = runTest(testDispatcher) { + repositoryWithLocalTheme(ThemeMode.DARK, unsynced = true) + enqueueUser(theme = "light") + enqueueUser(theme = "dark") + + repository.refresh() + + assertThat(repository.observeThemeMode().first()).isEqualTo(ThemeMode.DARK) + } + + @Test + fun `a pending push that fails again keeps the change owed`() = runTest(testDispatcher) { + repositoryWithLocalTheme(ThemeMode.DARK, unsynced = true) + enqueueUser(theme = "light") + enqueueOffline() + + val result = repository.refresh() + + // The refresh itself still succeeded — only the push did not. + assertThat(result).isInstanceOf(ApiResult.Success::class.java) + assertThat(repository.observeThemeMode().first()).isEqualTo(ThemeMode.DARK) + assertThat(themeStore.hasUnsyncedChange).isTrue() + } + + /** + * When the account already agrees with the pending change (the confirmation was + * lost, or the same choice was made on the web), reconciling clears the debt + * without spending a request. + */ + @Test + fun `a pending change the account already holds is not re-sent`() = runTest(testDispatcher) { + repositoryWithLocalTheme(ThemeMode.DARK, unsynced = true) + enqueueUser(theme = "dark") + + repository.refresh() + + assertThat(themeStore.hasUnsyncedChange).isFalse() + assertThat(repository.observeThemeMode().first()).isEqualTo(ThemeMode.DARK) + assertThat(server.requestCount).isEqualTo(1) + } +} diff --git a/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/domain/ThemeSyncTest.kt b/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/domain/ThemeSyncTest.kt new file mode 100644 index 0000000..9cf1ba2 --- /dev/null +++ b/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/domain/ThemeSyncTest.kt @@ -0,0 +1,175 @@ +package com.interlinedlist.android.feature.profile.domain + +import com.google.common.truth.Truth.assertThat +import com.interlinedlist.android.core.datastore.ThemeMode +import org.junit.Test + +/** + * Issue #36: the rule that decides which side wins when the device's theme and the + * account's `theme` disagree. + * + * The rule is deliberately **not** "newest timestamp wins" (neither side carries one) + * and **not** "the account always wins" (that would silently undo a change made with + * no connection). It is: *an unsynced local choice wins; otherwise the account wins*. + * Both halves are asserted here so the behaviour cannot drift into whatever the + * repository happens to do. + */ +class ThemeSyncTest { + + // --- Wire mapping -------------------------------------------------------- + + /** + * `/help/settings` publishes the account's own vocabulary: "Theme: Light, dark, or + * system (follows your device preference)" — so "follows the system" is a real + * account value and a local SYSTEM choice does **not** have to be coerced into + * light or dark. Live `GET /api/user` returns these lower-case. + */ + @Test + fun `each mode maps to the theme value the web publishes`() { + assertThat(ThemeMode.LIGHT.wire).isEqualTo("light") + assertThat(ThemeMode.DARK.wire).isEqualTo("dark") + assertThat(ThemeMode.SYSTEM.wire).isEqualTo("system") + } + + @Test + fun `every mode round-trips through the wire value`() { + ThemeMode.entries.forEach { mode -> + assertThat(themeModeFromWire(mode.wire)).isEqualTo(mode) + } + } + + @Test + fun `parsing tolerates casing and the obvious synonym for system`() { + assertThat(themeModeFromWire("DARK")).isEqualTo(ThemeMode.DARK) + assertThat(themeModeFromWire(" Light ")).isEqualTo(ThemeMode.LIGHT) + assertThat(themeModeFromWire("auto")).isEqualTo(ThemeMode.SYSTEM) + } + + /** + * The live server accepts *any* string for `theme` — a PATCH of "sepia" (or of "") + * is answered 200 and stored verbatim — so an unrenderable value really can come + * back. It must parse to null rather than to a mode we would then adopt. + */ + @Test + fun `a value the app cannot render does not parse`() { + assertThat(themeModeFromWire("sepia")).isNull() + assertThat(themeModeFromWire("")).isNull() + assertThat(themeModeFromWire(null)).isNull() + } + + // --- The account wins when nothing changed here --------------------------- + + /** + * The headline case from the issue: dark was chosen on the web, this is a fresh + * install whose local default is SYSTEM and was never touched, so the account's + * choice is adopted. + */ + @Test + fun `a fresh install adopts the account theme`() { + val outcome = reconcileTheme( + local = ThemeMode.SYSTEM, + hasUnsyncedLocalChange = false, + accountTheme = "dark", + ) + + assertThat(outcome).isEqualTo(ThemeReconciliation.AdoptAccount(ThemeMode.DARK)) + } + + @Test + fun `a synced device adopts a theme changed elsewhere`() { + val outcome = reconcileTheme( + local = ThemeMode.DARK, + hasUnsyncedLocalChange = false, + accountTheme = "light", + ) + + assertThat(outcome).isEqualTo(ThemeReconciliation.AdoptAccount(ThemeMode.LIGHT)) + } + + @Test + fun `agreement is left alone`() { + val outcome = reconcileTheme( + local = ThemeMode.DARK, + hasUnsyncedLocalChange = false, + accountTheme = "dark", + ) + + assertThat(outcome).isEqualTo(ThemeReconciliation.InSync) + } + + /** + * Nothing to adopt and nothing the user asked to push: an account with no stored + * theme (or one we cannot render) leaves the device's own setting alone. In + * particular we never seed the account from a default the user never chose, and + * never clobber a value the web understands and we do not. + */ + @Test + fun `an absent or unrenderable account theme changes nothing`() { + assertThat( + reconcileTheme(ThemeMode.DARK, hasUnsyncedLocalChange = false, accountTheme = null), + ).isEqualTo(ThemeReconciliation.InSync) + assertThat( + reconcileTheme(ThemeMode.DARK, hasUnsyncedLocalChange = false, accountTheme = "sepia"), + ).isEqualTo(ThemeReconciliation.InSync) + } + + // --- An unsynced local choice wins ---------------------------------------- + + /** + * The offline case. The user picked dark with no connection, so the account still + * says light; the account value is stale by construction and the local choice is + * pushed instead of being overwritten. + */ + @Test + fun `an unsynced local choice is pushed rather than overwritten`() { + val outcome = reconcileTheme( + local = ThemeMode.DARK, + hasUnsyncedLocalChange = true, + accountTheme = "light", + ) + + assertThat(outcome).isEqualTo(ThemeReconciliation.PushLocal(ThemeMode.DARK)) + } + + @Test + fun `an unsynced local choice is pushed to an account with no theme`() { + val outcome = reconcileTheme( + local = ThemeMode.SYSTEM, + hasUnsyncedLocalChange = true, + accountTheme = null, + ) + + assertThat(outcome).isEqualTo(ThemeReconciliation.PushLocal(ThemeMode.SYSTEM)) + } + + /** + * The account already agrees (the push landed but its confirmation was lost, or + * the same choice was made on the web). Re-sending it would be pointless; the + * device just records the account as holding it, which clears the pending flag. + */ + @Test + fun `an unsynced choice the account already holds is only marked synced`() { + val outcome = reconcileTheme( + local = ThemeMode.DARK, + hasUnsyncedLocalChange = true, + accountTheme = "dark", + ) + + assertThat(outcome).isEqualTo(ThemeReconciliation.AdoptAccount(ThemeMode.DARK)) + } + + /** + * An unrenderable account value does not block a pending push: the user's own + * choice is what they asked for, so it wins over a value neither side can show. + */ + @Test + fun `an unsynced local choice beats an unrenderable account theme`() { + val outcome = reconcileTheme( + local = ThemeMode.LIGHT, + hasUnsyncedLocalChange = true, + accountTheme = "sepia", + ) + + assertThat(outcome).isEqualTo(ThemeReconciliation.PushLocal(ThemeMode.LIGHT)) + } +} diff --git a/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/ui/FakeSettingsRepository.kt b/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/ui/FakeSettingsRepository.kt index edd4003..45720f1 100644 --- a/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/ui/FakeSettingsRepository.kt +++ b/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/ui/FakeSettingsRepository.kt @@ -1,6 +1,7 @@ package com.interlinedlist.android.feature.profile.ui import com.interlinedlist.android.core.common.result.ApiResult +import com.interlinedlist.android.core.datastore.ThemeMode import com.interlinedlist.android.feature.profile.data.SettingsRepository import com.interlinedlist.android.feature.profile.domain.UserSettings import com.interlinedlist.android.feature.profile.domain.UserSettingsUpdate @@ -12,19 +13,40 @@ import kotlinx.coroutines.flow.MutableStateFlow * [updateResult] drive the success/failure paths; on success the fake behaves like * the real one — it publishes the new settings to [observeSettings] — and every * update is recorded so tests can assert exactly which fields were sent. + * + * The theme half mirrors the real repository's contract too: [setThemeMode] stores the + * mode locally **before** the result is consulted, so a failed save still leaves the + * device holding the user's choice. */ class FakeSettingsRepository : SettingsRepository { private val settings = MutableStateFlow(null) + private val themeMode = MutableStateFlow(ThemeMode.SYSTEM) var refreshResult: ApiResult = ApiResult.Success(UserSettings()) var updateResult: ((UserSettingsUpdate) -> ApiResult)? = null + var themeResult: ((ThemeMode) -> ApiResult)? = null var refreshCount = 0 val updates = mutableListOf() + val themeModes = mutableListOf() override fun observeSettings(): Flow = settings + override fun observeThemeMode(): Flow = themeMode + + /** Seeds the device's stored appearance, as a previous session would have left it. */ + fun seedThemeMode(mode: ThemeMode) { + themeMode.value = mode + } + + override suspend fun setThemeMode(mode: ThemeMode): ApiResult { + themeModes += mode + // Local first, exactly like the real one: the choice survives a failed push. + themeMode.value = mode + return themeResult?.invoke(mode) ?: ApiResult.Success(settings.value ?: UserSettings()) + } + override suspend fun refresh(): ApiResult { refreshCount++ return refreshResult.also { if (it is ApiResult.Success) settings.value = it.data } diff --git a/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/ui/SettingsThemeTest.kt b/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/ui/SettingsThemeTest.kt new file mode 100644 index 0000000..261f056 --- /dev/null +++ b/feature/profile/src/test/kotlin/com/interlinedlist/android/feature/profile/ui/SettingsThemeTest.kt @@ -0,0 +1,147 @@ +package com.interlinedlist.android.feature.profile.ui + +import app.cash.turbine.test +import com.google.common.truth.Truth.assertThat +import com.interlinedlist.android.core.common.result.ApiResult +import com.interlinedlist.android.core.common.result.AppError +import com.interlinedlist.android.core.datastore.ThemeMode +import com.interlinedlist.android.feature.profile.domain.UserSettings +import com.interlinedlist.android.feature.profile.ui.settings.SettingsViewModel +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.StandardTestDispatcher +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.test.setMain +import org.junit.After +import org.junit.Before +import org.junit.Test + +/** + * Issue #36: the theme control on the Settings screen. + * + * Two things set it apart from the other preferences on this screen and are pinned + * here: it reads from the **device's** store rather than from the account field the + * settings carry, and a failed save does **not** roll it back — the app has already + * re-themed and the choice is owed to the account, not lost. + */ +@OptIn(ExperimentalCoroutinesApi::class) +class SettingsThemeTest { + + private val dispatcher = StandardTestDispatcher() + private lateinit var repo: FakeSettingsRepository + + @Before + fun setUp() { + Dispatchers.setMain(dispatcher) + repo = FakeSettingsRepository() + } + + @After + fun tearDown() = Dispatchers.resetMain() + + private fun loadedViewModel( + settings: UserSettings = UserSettings(), + storedTheme: ThemeMode = ThemeMode.SYSTEM, + ): SettingsViewModel { + repo.refreshResult = ApiResult.Success(settings) + repo.seedThemeMode(storedTheme) + return SettingsViewModel(repo) + } + + private val serverErrorMessage = "InterlinedList is having trouble right now. Try again shortly." + + @Test + fun `the control shows the theme stored on the device`() = runTest(dispatcher) { + val vm = loadedViewModel(storedTheme = ThemeMode.DARK) + advanceUntilIdle() + + assertThat(vm.uiState.value.themeMode).isEqualTo(ThemeMode.DARK) + } + + /** + * The device's store wins over the account field even when the two disagree: the + * account may simply not have caught up with a change made offline. + */ + @Test + fun `the control follows the device store rather than the account field`() = + runTest(dispatcher) { + val vm = loadedViewModel( + settings = UserSettings(theme = "light"), + storedTheme = ThemeMode.DARK, + ) + advanceUntilIdle() + + assertThat(vm.uiState.value.themeMode).isEqualTo(ThemeMode.DARK) + } + + @Test + fun `choosing a theme saves it and shows it at once`() = runTest(dispatcher) { + val vm = loadedViewModel(storedTheme = ThemeMode.SYSTEM) + + vm.uiState.test { + advanceUntilIdle() + vm.setThemeMode(ThemeMode.DARK) + + // The save is marked pending without waiting for the dispatcher. + assertThat(vm.uiState.value.isSaving).isTrue() + advanceUntilIdle() + + assertThat(repo.themeModes).containsExactly(ThemeMode.DARK) + // The theme goes out on its own path, not mixed into a settings PATCH. + assertThat(repo.updates).isEmpty() + val state = expectMostRecentItem() + assertThat(state.themeMode).isEqualTo(ThemeMode.DARK) + assertThat(state.isSaving).isFalse() + assertThat(state.errorMessage).isNull() + } + } + + @Test + fun `re-choosing the current theme does not call the repository`() = runTest(dispatcher) { + val vm = loadedViewModel(storedTheme = ThemeMode.LIGHT) + advanceUntilIdle() + + vm.setThemeMode(ThemeMode.LIGHT) + advanceUntilIdle() + + assertThat(repo.themeModes).isEmpty() + } + + /** + * The offline case as the user sees it: the app has re-themed, the failure is + * reported, and the selection stays where they put it. + */ + @Test + fun `a failed save reports the error but keeps the chosen theme`() = runTest(dispatcher) { + val vm = loadedViewModel(storedTheme = ThemeMode.LIGHT) + repo.themeResult = { ApiResult.Failure(AppError.Server("boom")) } + + vm.uiState.test { + advanceUntilIdle() + vm.setThemeMode(ThemeMode.DARK) + advanceUntilIdle() + + val state = expectMostRecentItem() + assertThat(state.themeMode).isEqualTo(ThemeMode.DARK) + assertThat(state.isSaving).isFalse() + assertThat(state.errorMessage) + .isEqualTo("$serverErrorMessage ${SettingsViewModel.THEME_NOT_SYNCED_NOTE}") + } + } + + @Test + fun `a theme adopted from the account while the screen is open moves the selection`() = + runTest(dispatcher) { + val vm = loadedViewModel(storedTheme = ThemeMode.SYSTEM) + advanceUntilIdle() + + // The reconciliation that follows a refresh writes the account's choice to + // the device store; the screen follows it. + repo.seedThemeMode(ThemeMode.DARK) + advanceUntilIdle() + + assertThat(vm.uiState.value.themeMode).isEqualTo(ThemeMode.DARK) + } +}