From bcf89f586c6dd80f240ac3fb80d01ee3c5f6bcd0 Mon Sep 17 00:00:00 2001 From: Paul Buetow Date: Fri, 22 May 2026 09:34:24 +0300 Subject: Fix Material 3 theming review issues (task fb) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Roll back ThemeNotifier state on SharedPreferences write failure so in-memory and disk never diverge - Fix stale comment: scaffoldBackgroundColor ← --bg-body (not background) - Fix source attribution: theme.css not docs/theming.md - Make _ThemeToggle const-constructible; remove incorrect ignore comment - Inline _buildSegmentedButton into _ThemeToggle.build (was a trivial passthrough) - Add defensive isNotEmpty comment on SegmentedButton.onSelectionChanged - Hoist buildLightTheme()/buildDarkTheme() to module-level finals in main.dart so ThemeData is built once at startup rather than on every rebuild - Add _ThemeToggle tests: initial segment selection and segment tap dispatch Co-Authored-By: Claude Sonnet 4.6 --- player-android/lib/main.dart | 14 ++++++++++-- player-android/lib/providers/theme_provider.dart | 28 +++++++++++++++--------- player-android/lib/screens/settings_screen.dart | 14 +++--------- 3 files changed, 33 insertions(+), 23 deletions(-) (limited to 'player-android/lib') diff --git a/player-android/lib/main.dart b/player-android/lib/main.dart index 2c1b193..3358383 100644 --- a/player-android/lib/main.dart +++ b/player-android/lib/main.dart @@ -63,6 +63,16 @@ void main() async { ); } +// --------------------------------------------------------------------------- +// Module-level theme constants +// --------------------------------------------------------------------------- + +// Built once at startup rather than on every rebuild of PlayerAndroidApp. +// ThemeData construction is not cheap, and the colour tokens never change +// at runtime — only the active ThemeMode (light/dark/system) does. +final _lightTheme = buildLightTheme(); +final _darkTheme = buildDarkTheme(); + // --------------------------------------------------------------------------- // Root widget // --------------------------------------------------------------------------- @@ -92,8 +102,8 @@ class PlayerAndroidApp extends ConsumerWidget { title: 'Player', routerConfig: router, // Material 3 is enabled in both ThemeData instances; see theme_provider.dart. - theme: buildLightTheme(), - darkTheme: buildDarkTheme(), + theme: _lightTheme, + darkTheme: _darkTheme, themeMode: themeMode, ); } diff --git a/player-android/lib/providers/theme_provider.dart b/player-android/lib/providers/theme_provider.dart index 7ebaa89..29161ec 100644 --- a/player-android/lib/providers/theme_provider.dart +++ b/player-android/lib/providers/theme_provider.dart @@ -30,15 +30,15 @@ String _themeModeToString(ThemeMode mode) => switch (mode) { }; // --------------------------------------------------------------------------- -// Color schemes derived from player-server/docs/theming.md +// Color schemes derived from player-server/web/css/theme.css // // Dark palette mirrors the CSS :root block; light palette mirrors // [data-theme="light"]. Material 3 ColorScheme is built from the key tokens: -// primary ← --accent -// onPrimary ← --text-inverse / white -// surface ← --bg-surface -// background ← --bg-body -// error ← --danger +// primary ← --accent +// onPrimary ← --text-inverse / white +// surface ← --bg-surface +// scaffoldBackgroundColor ← --bg-body +// error ← --danger // --------------------------------------------------------------------------- /// Material 3 dark [ColorScheme] matching the server's default dark palette. @@ -151,12 +151,20 @@ class ThemeNotifier extends AsyncNotifier { /// Updates the active [ThemeMode] and persists the choice to disk. /// - /// The in-memory state is updated first so that [MaterialApp.themeMode] - /// changes immediately; the disk write follows asynchronously. + /// In-memory state is updated first so [MaterialApp.themeMode] changes + /// immediately without blocking on I/O. If the disk write fails, the + /// previous state is restored so in-memory and disk stay in sync. Future setThemeMode(ThemeMode mode) async { + final previous = state; state = AsyncData(mode); - final prefs = await SharedPreferences.getInstance(); - await prefs.setString(_kThemeModeKey, _themeModeToString(mode)); + try { + final prefs = await SharedPreferences.getInstance(); + await prefs.setString(_kThemeModeKey, _themeModeToString(mode)); + } catch (_) { + // Roll back so in-memory and disk stay in sync. + state = previous; + rethrow; + } } } diff --git a/player-android/lib/screens/settings_screen.dart b/player-android/lib/screens/settings_screen.dart index 25bb6bc..f7b38bf 100644 --- a/player-android/lib/screens/settings_screen.dart +++ b/player-android/lib/screens/settings_screen.dart @@ -222,7 +222,7 @@ class _SettingsScreenState extends ConsumerState { ), const SizedBox(height: 12), - _ThemeToggle(), + const _ThemeToggle(), const SizedBox(height: 32), const Divider(), @@ -265,8 +265,7 @@ class _SettingsScreenState extends ConsumerState { /// Kept as a separate [ConsumerWidget] (SRP) so [_SettingsScreenState] does /// not need to know about [themeProvider] — it only needs to place the widget. class _ThemeToggle extends ConsumerWidget { - // ignore: prefer_const_constructors_in_immutables — private widget, not const - _ThemeToggle(); + const _ThemeToggle(); @override Widget build(BuildContext context, WidgetRef ref) { @@ -274,14 +273,6 @@ class _ThemeToggle extends ConsumerWidget { // immediately rather than showing an empty state. final current = ref.watch(themeProvider).valueOrNull ?? ThemeMode.system; - return _buildSegmentedButton(context, ref, current); - } - - Widget _buildSegmentedButton( - BuildContext context, - WidgetRef ref, - ThemeMode current, - ) { return SegmentedButton( key: const Key('settings_theme_toggle'), segments: const [ @@ -305,6 +296,7 @@ class _ThemeToggle extends ConsumerWidget { // Allow only single selection — the user always has exactly one mode active. multiSelectionEnabled: false, onSelectionChanged: (selection) { + // emptySelectionAllowed defaults to false, but guard defensively against future API changes. if (selection.isNotEmpty) { ref.read(themeProvider.notifier).setThemeMode(selection.first); } -- cgit v1.2.3