feat(settings-ui): shared feature/settings-ui — exact Settings parity on iOS - #1154
Merged
Merged
Conversation
…tformSettingsHooks
Replace java.time.Instant with kotlinx.datetime.Clock so the class and its
tests can live in commonMain/commonTest and run on both Android and iOS.
The test `publishHeader writes exactly one sentinel, with the new label` was
renamed to publishHeaderWritesExactlyOneSentinelWithNewLabel — Kotlin/Native
rejects commas inside backtick-quoted test names. The behavioural claim is
identical; the assertion (`assertEquals("New Name", updated.label)`) is
unchanged.
`toSortedSet()` in InMemoryMaintenanceTarget replaced with `.sorted()` which
is available on all KMP targets (toSortedSet() returns java.util.TreeSet).
Removed-test: publishHeader writes exactly one sentinel, with the new label
…i commonMain Move all Settings Compose UI from app/src/main/.../feature/settings/ and feature/readersettings/ into the new feature/settings-ui module so both Android (:app) and iOS (:shared) render identical screens. Platform-specific files remain in androidMain (DebugLogViewModel/Screen, DictionaryPacksViewModel/Screen, DiagnosticsSection which need Android Context/DownloadManager/FileProvider; AssetFontLoader; ComicDisplaySettingsPanel uses androidMain because it references reader types not yet on iOS). Changes in this commit: - 44 Compose files copied and adapted from app/ to feature/settings-ui/commonMain - R.string.* → Res.string.* + CMP import headers applied to all migrated files - Material Icons replaced with RiffleIcons constants (new icons: Bedtime, CloudOff, Edit, KeyboardArrowRight added to RiffleIcons.kt in design-system) - java.time.* replaced with kotlinx.datetime everywhere in commonMain - org.koin.androidx.compose.koinViewModel → org.koin.compose.viewmodel.koinViewModel - WindowSizeClass → isExpandedWidth: Boolean in SettingsScreen - AppLocaleController/AppLanguage abstracted behind PlatformSettingsHooks - LocalLifecycleOwner ON_RESUME hooked via PlatformSettingsHooks.OnResumeEffect - BackHandler: androidx.activity.compose → androidx.compose.ui.backhandler (CMP) - AppLanguage enum extracted to feature/settings-ui/commonMain/i18n/AppLanguage.kt - ChangelogScreen + ChangelogViewModel + ReleaseDate helper added in commonMain - AnnotationSyncMaintenanceViewModel: java.time.format replaced with kotlinx.datetime - koin-compose-viewmodel library entry added to libs.versions.toml - feature/settings-ui build.gradle.kts: koin.compose.viewmodel dep added; androidMain receives lifecycle-runtime-ktx, core:dictionary, core:logging The test `forgetDevice also deletes the device's metadata sentinel` was moved from androidHostTest to commonTest in the previous commit alongside the production class it covers; it now runs on both Android and iOS. Removed-test: forgetDevice also deletes the device's metadata sentinel
…lete old platform copies Both :app (Android) and :shared (iOS) now import and render com.riffle.feature.settings.ui.SettingsScreen from the new shared module. Android (:app): - SettingsNavGraph now passes isExpandedWidth (derived from WindowSizeClass) and an anonymous PlatformSettingsHooks that wires AppLocaleController language change and LocalLifecycleOwner ON_RESUME for SAF-grant health refresh. - 34 files deleted from app/src/main/kotlin/.../feature/settings/ (all migrated to feature/settings-ui in the previous commit). - KoinViewModelModules updated to import from new feature/settings-ui packages. iOS (:shared): - shared/src/commonMain/.../settings/SettingsScreen.kt deleted. - HomeScreen.kt now uses com.riffle.feature.settings.ui.SettingsScreen. Tests: - CrashReportShareSubjectTest moved from app/test to feature/settings-ui/commonTest. The function name was changed from a backtick-quoted sentence to camelCase (Kotlin/Native convention); claim is identical. - DebugLogDisplayOrderTest moved from app/test to feature/settings-ui/androidHostTest. Removed-test: subject wraps the timestamp in the Riffle crash report label Removed-test: newest entry is first Removed-test: empty input yields empty output
…, and translation keys - Add functional ComicDisplaySettingsPanel for iOS (Panel View toggle, Panel Overflow radio group, On-Screen Info toggles) - Wire iOS changelog navigation via IosSettingsSubScreen.Changelog state in HomeScreen and ChangelogViewModel in iOS Koin module - Add currentLanguage() to PlatformSettingsHooks so SettingsScreen initialises appLanguage from the platform's actual current language - Add canInstallUpdate param to AppVersionSection; iOS gets false so the install button is hidden (iOS has no in-app update flow) - Add 8 missing string keys to values-bg/ and values-es/ (ComicDisplay panel labels) to fix checkTranslations - Expand PlatformSettingsHooks.LanguageRow Composable annotation
- Move SettingsAutoScrollPanel/Cadence/Display/ScreenDerivations tests from shared:commonTest to feature:settings-ui iosTest/commonTest; the old tests referenced internal symbols from the deleted shared/settings/SettingsScreen.kt - Extract shared derivation functions (listeningSummary, readaloudSubtitle, comicBackgroundChipSelection) to SettingsDerivations.kt in feature:settings-ui - Add TestTags constants for the auto-scroll/cadence WPM stepper buttons and display panel toggle rows; wire them into the production Composables - Fix iOS test assertions to match the scrollable DetailScaffold viewport: colour-chip assertions now account for the ', selected' suffix on the default chip; text checks replaced with onAllNodesWithContentDescription count checks for off-screen nodes; 'On-screen info' string fixed to match resource casing Removed-test: SettingsAutoScrollPanelTest.autoScrollPanelOffersTheToggleAndTheSpeedStepper Removed-test: SettingsAutoScrollPanelTest.theReaderToggleSwitchWritesShowAutoScroll Removed-test: SettingsAutoScrollPanelTest.theSpeedStepperMovesInAutoScrollSpeedSteps Removed-test: SettingsAutoScrollPanelTest.theSpeedStepperClampsAtTheDomainBounds Removed-test: SettingsCadencePanelTest.cadencePanelOffersTheToggleTheSpeedStepperAndTheColourChips Removed-test: SettingsCadencePanelTest.theReaderToggleSwitchWritesShowCadence Removed-test: SettingsCadencePanelTest.theSpeedStepperMovesInAutoScrollSpeedStepsAndClamps Removed-test: SettingsCadencePanelTest.aColourChipWritesTheHighlightColourEnum Removed-test: SettingsCadencePanelTest.theColourChipsAreEnumBackedSoTheyCannotOfferAnUnpaintableColour Removed-test: SettingsCadencePanelTest.anUnsupportedWebViewReplacesTheControlsWithANote Removed-test: SettingsDisplayPanelTest.displayPanelOffersEveryOnScreenInfoSwitch Removed-test: SettingsDisplayPanelTest.eachOnScreenInfoSwitchWritesItsOwnPreference Removed-test: SettingsDisplayPanelTest.coloredChapterMapIsInertWhileTheChapterMapIsOff Removed-test: SettingsDisplayPanelTest.coloredChapterMapTogglesOnceTheChapterMapIsOn Removed-test: SettingsScreenDerivationsTest.listeningSummaryPrintsTheSpeedAtTheDomainsStepGranularity Removed-test: SettingsScreenDerivationsTest.listeningSummaryTrimsAWholeNumberSpeed Removed-test: SettingsScreenDerivationsTest.readaloudSubtitleSuppressesZeroCountsAndKeepsHostAndVersion Removed-test: SettingsScreenDerivationsTest.readaloudSubtitleHasAFriendlyEmptyCase Removed-test: SettingsScreenDerivationsTest.comicBackgroundOptionsIncludeAuto Removed-test: SettingsScreenDerivationsTest.comicBackgroundChipFoldsDarkDimOntoDark Removed-test: SettingsScreenDerivationsTest.everyStoredComicBackgroundThemeSelectsAChipThatExists Removed-test: SettingsScreenDerivationsTest.panelOverflowChipsCoverEveryBehaviour
…estGuardrailLint TestGuardrailLint.TEST_SOURCE_DIR didn't include iosTest, so tests moved from shared/commonTest to feature/settings-ui/iosTest appeared as deleted and triggered a checkTestGuardrails failure. The Settings back button content description was changed from "Back" to "← Libraries" to restore the label the old shared/settings/SettingsScreen.kt had. NavDrawerTests waits for app.buttons["← Libraries"] to confirm the Settings screen is open (the string "Settings" is ambiguous while the drawer animates shut). Added ui_libraries_with_arrow string to settings-ui strings.xml (en/es/bg) matching the identical string already in feature:design-system.
"Timed out while acquiring background assertion" is emitted by the simulator process-management layer when the runner can't claim a background assertion under CPU contention — it is never produced by test code. Adding it to CRASH_RE lets the whole-suite retry fire on a clean simulator instead of exiting as a genuine failure. testAddAbsSourceEndToEnd hit this on both retry iterations in the current run; the fix ensures the next occurrence triggers the whole-suite retry path.
…played passes When platformSupported=false, the unavailable note was rendered after the hero icon and two description texts in a vertically scrollable DetailScaffold. On the API-25 phone emulator the note was off-screen, causing CadenceSettingsPanelTest.cadence_panel_platformUnsupported_hides_toggles_and_shows_note to fail with assertIsDisplayed(). Move the early-return guard to the top of the scaffold body so the note is the first (and only) visible item. Better UX too: a user whose device cannot run Cadence has no need to see the feature marketing hero.
…r harness tearDowns ContinuousChapterBoundaryHarnessTest.boundaryDetentArmedAfterBackwardPrepend was failing with SlotWriter.moveSlotGapTo ArrayIndexOutOfBoundsException — the same Readium-WebView/Compose race documented in NavigationSnapHarnessTest.tearDown(). Root cause: ContinuousChapterBoundaryHarnessTest, ContinuousAnnotationRenderHarnessTest, OrientationFlipAnnotationHarnessTest, and NoteGlyphRenderHarnessTest all called scenario.close() immediately, triggering Activity destruction before the last WebView-driven recomposition cycle finished, leaving the SlotTable gap-buffer in a partial state when the lifecycle event disposed the composition. Fix: add Thread.sleep(400) before scenario.close() in all four tearDowns, matching the pattern already documented and used in NavigationSnapHarnessTest. waitForIdle() is intentionally not used because the Readium WebView keeps triggering recompositions and would block it indefinitely.
…so viewport is always non-zero Without weight(1f), the inner scrollable Column wraps its content height. When the platform-unsupported path renders only a single text item, the scroll viewport equals just that text's height. On the API-25 emulator, statusBarsPadding/navigationBarsPadding interactions cause the item's clip bounds to be outside the visible area, making assertIsDisplayed() fail for CadenceSettingsPanelTest.cadence_panel_platformUnsupported_hides_toggles_and_shows_note. weight(1f) gives the scrollable Column a fixed height (all remaining space after the TopAppBar), making the viewport large enough for any amount of content — the standard Compose pattern for a scrollable area that fills remaining screen space.
…assertIsDisplayed failure The manual Surface(statusBarsPadding()) + TopAppBar pattern applied status-bar insets twice: once via Modifier.statusBarsPadding() on the Surface, and again via TopAppBar's default TopAppBarDefaults.windowInsets. In the ComponentActivity test environment this caused assertIsDisplayed() to fail on the first text node because the clip region was miscomputed. Replacing with Material3 Scaffold gives one unified inset pass that works correctly in both production and ComponentActivity-based instrumentation tests.
fillMaxSize() before verticalScroll() set the Column's layout bounds to the full screen, causing Compose's assertIsDisplayed() clip check to compute whether the text node was within those inflated bounds rather than within the actual scroll viewport. The text is at y=16dp (inner padding) inside a single-item scrollable — always visible — but the inflated clip region triggered a false negative on the API-25 test emulator. Without fillMaxSize(), the Column wraps its content height. The Scaffold background already covers the full screen so there is no visual regression.
Compose Multiplatform's string resource parser uses a standard XML parser which does not process Android's \' escape sequence — it renders the literal characters \' in the output string. Replace all eight occurrences with bare apostrophes, which are valid in XML text content and render correctly on both Android and iOS. This was the root cause of CadenceSettingsPanelTest cadence_panel_platformUnsupported_hides_toggles_and_shows_note failing: the test searched for "Cadence isn't available…" but the actual rendered text contained "Cadence isn\'t available…", so the semantic node was never found.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1142
Summary
feature/settings-uiKMP module with android+iOS topology (mirroringfeature:source-ui), so both:appand:sharedrender one unified Settings screenapp/src/main/kotlin/com/riffle/app/feature/settings/intofeature/settings-ui/commonMainshared/src/commonMain/kotlin/com/riffle/shared/settings/SettingsScreen.kt(250+ lines, many gaps)PlatformSettingsHooksto abstract Android-specific behaviours (language change, lifecycle resume, install-update check, current language)AnnotationSyncMaintenancefromcore:data/androidMaintocore:data/commonMain(useskotlinx.datetimeinstead ofjava.time.*)ComicDisplaySettingsPanel(Panel View, Panel Overflow, On-Screen Info toggles)IosSettingsSubScreen.Changelog+ChangelogViewModelin iOS KoincomposeResourcesfor all 267 Settings strings (en/bg/es) —checkTranslationspassesAppLanguageTest(7 tests incommonTest, runs on both platforms) andReleaseDateTestiOS call path
iosApp → shared → HomeScreen.kt → feature:settings-ui SettingsScreen(commonMain, android+iOS topology) → same Compose tree Android renders, same KMP ViewModels viakoinViewModel().Android call path
app → SettingsNavGraph.kt → feature:settings-ui SettingsScreen(same commonMain composable) with an anonymousPlatformSettingsHooksthat wiresAppLocaleController, lifecycle resume, and install-update flows.Tests
feature:settings-ui:commonTest:AppLanguageTest(7 assertions),ReleaseDateTest(3 assertions) — run on both JVM andiosSimulatorArm64core:data:commonTest:AnnotationSyncMaintenanceTest(moved fromandroidHostTest, now runs on iOS too):feature:settings-ui:iosSimulatorArm64Testadded to.github/workflows/ios.yml./gradlew test jvmTest riffleChecks :app:compileDebugAndroidTestKotlin :core:data:compileAndroidHostTest :feature:settings-ui:iosSimulatorArm64TestCommits on branch (latest last)
refactor(core:data): liftAnnotationSyncMaintenanceto commonMainfeat(settings-ui): add composeResources string skeleton — 265 ui_ keys, en/es/bgfeat(settings-ui): migrate all Settings screens to feature/settings-ui commonMainfeat(settings-ui): wire :app and :shared to shared SettingsScreen; delete old platform copiesfeat(settings-ui): add to iOS CI, add AppLanguageTest for both platformsfix(settings-ui): implement iOS ComicDisplay panel, iOS changelog nav, and translation keys🤖 Generated with Claude Code