feat: event-theme fonts — capture + apply app-wide (Google flavor) with sheet opt-out#6163
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds event font metadata, platform-specific font resolution, app-wide enablement state, and a UI toggle for applying event fonts, with repository coverage updated for the new theme fields. ChangesEvent fonts
Sequence Diagram(s)sequenceDiagram
participant FlavorModule
participant EventFontResolver
participant GoogleFontsEventFontResolver
participant GoogleFontProvider
participant AppCompositionLocals
participant AppTheme
FlavorModule->>EventFontResolver: provide resolver
AppCompositionLocals->>EventFontResolver: resolve event fonts
EventFontResolver->>GoogleFontsEventFontResolver: map firmware font names
GoogleFontsEventFontResolver->>GoogleFontProvider: build FontFamily values
AppCompositionLocals->>AppTheme: provide LocalEventTypographyFonts
AppTheme->>AppTheme: apply event fonts to typography
sequenceDiagram
participant UiPrefs
participant UIViewModel
participant MainActivity
participant EventInfoSheet
UiPrefs->>UIViewModel: eventFontsEnabled state
MainActivity->>UIViewModel: read / update toggle state
EventInfoSheet->>UIViewModel: user toggles switch
UIViewModel->>UiPrefs: persist eventFontsEnabled
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
The live /resource/eventFirmware v2 populates `theme.fonts` for at least the
DEFCON edition (`{"heading":"Lato","body":"Atkinson Hyperlegible"}`), which the
model was silently dropping via @JsonIgnoreUnknownKeys.
- core:model — add `fonts: EventFirmwareFonts?` (new data class with
`heading`/`body`) to EventFirmwareTheme. These are Google Font *family names*,
not URLs. Persists for free: theme is cached as a JSON column, so fonts rides
the existing round-trip (no entity/DAO/schema change).
- Extended v2FieldsRoundTripThroughCache to assert theme.fonts survives the
seed → DB → getEdition path.
Captures the data only. Actually rendering with these fonts is deliberately out
of scope — a downloadable-font provider is Google-flavor only (F-Droid has none)
and would need a KMP expect/actual for cosmetic sheet typography; consumers fall
back to the app typeface until/unless that's deemed worth it.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fe3107f to
3d6dbf9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImpl.kt (1)
93-104: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: cache the seeded flag to avoid a DB
count()on every call.
ensureSeeded()re-querieslocalDataSource.count()on everygetEdition()call even after the table has been seeded. A cheap in-memory@Volatileseeded flag would avoid the extra round trip on the hot path once seeding has completed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImpl.kt` around lines 93 - 104, `ensureSeeded()` is still doing a database `count()` check on every `getEdition()` path even after seeding has already completed. Add an in-memory `@Volatile` seeded flag to `EventFirmwareRepositoryImpl`, use it as the fast path in `ensureSeeded()`, and set it to true only after `localDataSource.upsertAll(...)` succeeds inside the `writeMutex.withLock` block so the existing double-check behavior remains safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@core/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImpl.kt`:
- Around line 93-104: `ensureSeeded()` is still doing a database `count()` check
on every `getEdition()` path even after seeding has already completed. Add an
in-memory `@Volatile` seeded flag to `EventFirmwareRepositoryImpl`, use it as
the fast path in `ensureSeeded()`, and set it to true only after
`localDataSource.upsertAll(...)` succeeds inside the `writeMutex.withLock` block
so the existing double-check behavior remains safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: efa42723-7aee-4fcc-9d96-6cbd2cd50631
📒 Files selected for processing (23)
.github/workflows/scheduled-updates.yml.skills/compose-ui/strings-index.txtandroidApp/src/fdroid/kotlin/org/meshtastic/app/di/FDroidNetworkModule.ktandroidApp/src/main/assets/event_firmware.jsoncore/data/src/commonMain/kotlin/org/meshtastic/core/data/datasource/EventFirmwareEditionLocalDataSource.ktcore/data/src/commonMain/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImpl.ktcore/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/DeviceLinkRepositoryImplTest.ktcore/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/EventFirmwareRepositoryImplTest.ktcore/data/src/jvmTest/kotlin/org/meshtastic/core/data/repository/FirmwareReleaseRepositoryImplTest.ktcore/database/schemas/org.meshtastic.core.database.MeshtasticDatabase/47.jsoncore/database/src/commonMain/kotlin/org/meshtastic/core/database/MeshtasticDatabase.ktcore/database/src/commonMain/kotlin/org/meshtastic/core/database/dao/EventFirmwareEditionDao.ktcore/database/src/commonMain/kotlin/org/meshtastic/core/database/entity/EventFirmwareEditionEntity.ktcore/model/src/commonMain/kotlin/org/meshtastic/core/model/EventFirmware.ktcore/network/src/commonMain/kotlin/org/meshtastic/core/network/EventFirmwareRemoteDataSource.ktcore/network/src/commonMain/kotlin/org/meshtastic/core/network/service/ApiService.ktcore/resources/src/commonMain/composeResources/values/strings.xmlcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/EventInfoSheet.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/MainAppBar.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/util/LocalEventBranding.ktcore/ui/src/commonTest/kotlin/org/meshtastic/core/ui/util/EventBrandingTest.ktfeature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/ConnectionsScreen.ktschemas/event_firmware.schema.json
💤 Files with no reviewable changes (1)
- schemas/event_firmware.schema.json
…pt-out
Renders an event edition's theme.fonts (Google Font family names, e.g. DEFCON's
Lato/Atkinson Hyperlegible) as the app-wide typeface while connected to that
event firmware, via downloadable Google Fonts. Google flavor only — F-Droid has
no Play font provider and keeps the default typeface.
🌟 New Features
- App-wide typography swap: AppTheme reads LocalEventTypographyFonts and remaps
the full M3 typescale (heading → display/headline/title, body → body/label)
when an event edition is active, fonts resolve, and the user hasn't opted out.
- Opt-out toggle ("Use event fonts") in the event info sheet, shown only when the
fonts are actually available (Google flavor + edition ships fonts). Backed by a
new eventFontsEnabled UiPrefs boolean (default on).
🛠️ Architecture (flavor seam, mirrors ApiService)
- core:ui EventFonts.kt: EventFontResolver fun interface + EventFonts +
Typography.withEventFonts + LocalEventTypographyFonts / LocalEventFontsToggle.
- Google flavor binds GoogleFontsEventFontResolver (ui-text-google-fonts +
GoogleFont.Provider); F-Droid binds a no-op resolver. Resolved via Koin,
provided into the composition at MainActivity next to LocalEventBranding.
- Ships androidApp/src/google/res/values/font_certs.xml (canonical AOSP cert
array) for the font provider — google source set only, F-Droid stays clean.
Caveats: downloadable fonts load async (brief fallback→swap on first show) and
fetch from Google's font provider (google flavor only, consistent with Maps).
Runtime rendering not verifiable here (needs a device); compiles + DI-verifies on
both flavors.
Testing: spotlessCheck, detekt, :core:ui:build, :core:prefs:allTests,
:androidApp:{compile,test}{Google,Fdroid}DebugKotlin/UnitTest, :core:data:jvmTest
all green — incl. KoinVerificationTest on both flavors.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/EventInfoSheet.kt (1)
103-114: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the whole row toggle the switch, not just the Switch itself.
Currently only tapping the small
Switchtoggles the preference; the labelTextoccupies most of the row's width but isn't clickable. Consider applyingModifier.toggleable(value = fontsToggle.enabled, onValueChange = fontsToggle.onChange, role = Role.Switch)on theRowand passingonCheckedChange = nullto theSwitchto avoid double-toggling, matching common Material list-row patterns and improving touch-target size/accessibility.♻️ Suggested refactor
Row( - modifier = Modifier.fillMaxWidth(), + modifier = Modifier + .fillMaxWidth() + .toggleable( + value = fontsToggle.enabled, + onValueChange = fontsToggle.onChange, + role = Role.Switch, + ), horizontalArrangement = Arrangement.spacedBy(12.dp), verticalAlignment = Alignment.CenterVertically, ) { Text( text = stringResource(Res.string.event_use_event_fonts), style = MaterialTheme.typography.bodyMedium, modifier = Modifier.weight(1f), ) - Switch(checked = fontsToggle.enabled, onCheckedChange = fontsToggle.onChange) + Switch(checked = fontsToggle.enabled, onCheckedChange = null) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/EventInfoSheet.kt` around lines 103 - 114, The `EventInfoSheet` row currently only toggles via the `Switch`, so make the entire `Row` act as the toggle target. Update the `Row` that contains the `Text` label and `Switch` to use `Modifier.toggleable` with `fontsToggle.enabled`, `fontsToggle.onChange`, and `Role.Switch`, and make the `Switch` non-interactive by passing `onCheckedChange = null` so it doesn’t double-fire. Keep the existing `fontsToggle` wiring intact and ensure the label and switch still reflect the same state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/EventInfoSheet.kt`:
- Around line 103-114: The `EventInfoSheet` row currently only toggles via the
`Switch`, so make the entire `Row` act as the toggle target. Update the `Row`
that contains the `Text` label and `Switch` to use `Modifier.toggleable` with
`fontsToggle.enabled`, `fontsToggle.onChange`, and `Role.Switch`, and make the
`Switch` non-interactive by passing `onCheckedChange = null` so it doesn’t
double-fire. Keep the existing `fontsToggle` wiring intact and ensure the label
and switch still reflect the same state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dfa9c20f-fe31-495b-9f91-88d341c07ae8
📒 Files selected for processing (16)
.skills/compose-ui/strings-index.txtandroidApp/build.gradle.ktsandroidApp/src/fdroid/kotlin/org/meshtastic/app/di/FlavorModule.ktandroidApp/src/google/kotlin/org/meshtastic/app/di/FlavorModule.ktandroidApp/src/google/kotlin/org/meshtastic/app/theme/GoogleFontsEventFontResolver.ktandroidApp/src/google/res/values/font_certs.xmlandroidApp/src/main/kotlin/org/meshtastic/app/MainActivity.ktcore/prefs/src/commonMain/kotlin/org/meshtastic/core/prefs/ui/UiPrefsImpl.ktcore/repository/src/commonMain/kotlin/org/meshtastic/core/repository/AppPreferences.ktcore/resources/src/commonMain/composeResources/values/strings.xmlcore/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeAppPreferences.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/EventInfoSheet.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/theme/EventFonts.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/theme/Theme.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/viewmodel/UIViewModel.ktgradle/libs.versions.toml
✅ Files skipped from review due to trivial changes (3)
- core/resources/src/commonMain/composeResources/values/strings.xml
- androidApp/build.gradle.kts
- .skills/compose-ui/strings-index.txt
CodeRabbit nitpick: the whole row (label + switch) now toggles the pref via Modifier.toggleable(role = Role.Switch); the Switch is visual-only (onCheckedChange = null) so it doesn't double-fire. Bigger touch target, one semantics node. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Runtime verification on a Pixel 6a showed the app-wide event typography swap was
wired correctly (the themed Text read fontFamily=GoogleFont("Atkinson
Hyperlegible")/Lato) but rendered the Roboto fallback with zero font-provider
requests. Root cause: Compose does NOT auto-fetch a downloadable (Google) font
that is referenced only through the theme's Typography — it silently uses the
fallback. The documented fix is to preload the font into the resolver cache.
- AppTheme now preloads the active event heading/body FontFamily via
LocalFontFamilyResolver.preload() in a LaunchedEffect keyed on the fonts.
No-op when there are no event fonts (desktop / F-Droid / non-event / opted
out). Once cached, the themed text re-resolves to the real typeface.
Verified end-to-end on device (DEFCON replay): before the fix ON vs OFF was
pixel-identical (Roboto); after, "DEF CON 34" renders in Lato and body text in
Atkinson Hyperlegible, and toggling "Use event fonts" visibly switches the
typeface app-wide. GMS FontLog confirms the fonts are fetched.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/theme/Theme.kt`:
- Around line 143-147: The font preloading logic in LaunchedEffect for Theme.kt
currently uses runCatching around fontResolver.preload, which can swallow
CancellationException and hide preload failures. Update the
eventFonts/fontResolver block to catch exceptions explicitly in the preload
loop, re-throw CancellationException immediately, and handle other failures with
logging or error reporting so the preload issue is visible. Keep the fix
localized to the LaunchedEffect that iterates over fonts.heading and fonts.body.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e1d24926-bef4-4a6a-b8c3-97a6d0bc13c4
📒 Files selected for processing (1)
core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/theme/Theme.kt
CodeRabbit: runCatching around the suspending fontResolver.preload() also catches CancellationException, so a cancelled LaunchedEffect would continue to the next font instead of stopping (structured-concurrency violation), and preload failures were dropped silently. Replaced with an explicit try/catch that re-throws CancellationException and logs other exceptions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The opt-out toggle previously governed only the app-wide fonts, while the app-bar
accent wash was always-on. Consolidate them: one composition local carries the
whole ambient event theme and one toggle governs both.
- core:ui EventFonts.kt: new EventTheme(accent, fonts); LocalEventTypographyFonts
→ LocalEventTheme; EventFontsToggle → EventThemeToggle (dropped the unused
`available` field — the sheet only opens for an active event, which always has
an accent, so the toggle is always meaningful).
- MainAppBar reads the accent wash from LocalEventTheme (now gated by the toggle)
instead of unconditionally from the edition. The branding icon + info sheet
still read LocalEventBranding, so they stay available even when opted out.
- AppTheme reads LocalEventTheme.fonts.
- Pref eventFontsEnabled → eventThemeEnabled (key event-theme-enabled); string
event_use_event_fonts → event_use_event_theme ("Use event theme").
- MainActivity provides EventTheme{accent, fonts} only while on event firmware
and not opted out.
Decided against app-wide color-scheme recolor: there is no built-in Compose M3
arbitrary-seed→ColorScheme API (dynamicColorScheme seeds only from the system
wallpaper), and slotting raw brand hex into primary/secondary breaks on-color
contrast. Accent-wash-only keeps it safe with no new dependency.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Why
The live
/resource/eventFirmware(v2) shipstheme.fonts(Google Font family names — e.g. DEFCON's{heading: Lato, body: Atkinson Hyperlegible}), which the model was dropping. This captures the field and renders it as the app-wide typeface while a device is on that event firmware. Follow-up to #6162; surfaced while auditing the payload for design#120.🌟 What it does
EventFirmwareTheme.fonts({heading, body}) added to the model; persists for free via the existing JSON theme column (no schema change).AppThemereadingLocalEventTypographyFonts.eventFontsEnabledUiPrefs boolean (default on).🛠️ Flavor seam (mirrors the
ApiServicesplit)core:uiEventFonts.kt:EventFontResolverfun interface +EventFonts+Typography.withEventFonts+ the two CompositionLocals.GoogleFontsEventFontResolver(androidx.compose.ui:ui-text-google-fonts+GoogleFont.Provider); F-Droid binds a no-op resolver → default typeface. Resolved via Koin, provided atMainActivitynext toLocalEventBranding.androidApp/src/google/res/values/font_certs.xml(canonical AOSP cert array) in the google source set only — F-Droid stays Google-free.Testing
spotlessCheck,detekt,:core:ui:build,:core:prefs:allTests,:androidApp:{compile,test}{Google,Fdroid}DebugKotlin/UnitTest,:core:data:jvmTestall green — includingKoinVerificationTeston both flavors andv2FieldsRoundTripThroughCache(theme.fonts survives the DB round-trip).Summary by CodeRabbit