Add in-memory theme picker in Settings#15
Conversation
Implement a theme picker in the settings screen that enables users to choose between "Follow system", "Light", and "Dark" themes. The selected theme is applied immediately across the entire application. Changes: - Added `ThemePreference` enum. - Added `ThemeStateHolder` global singleton state holder. - Implemented logic in `SettingsViewModel` to handle and reflect theme changes. - Added UI dialog in `SettingsScreen` to select the desired theme. - Updated `App.kt` to observe `ThemeStateHolder` and apply `colorScheme` dynamically.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a theme selection feature, allowing users to toggle between system, light, and dark modes. The changes involve updating the main application theme logic, adding a selection dialog in the settings screen, and introducing a global state holder for theme preferences. Feedback focuses on improving code quality by using Kotlin's entries property for enums, localizing hardcoded strings, and moving display-related logic from the ViewModel to the UI. Additionally, it is recommended to encapsulate the mutable state in the theme holder to ensure better control over state updates.
| title = { Text(text = stringResource(Res.string.settings_theme)) }, | ||
| text = { | ||
| Column { | ||
| ThemePreference.values().forEach { preference -> |
There was a problem hiding this comment.
Use entries instead of values() for Enum classes. In Kotlin 1.9.0+, entries is preferred as it returns a pre-allocated immutable list, avoiding the array allocation overhead of values() on every call.
| ThemePreference.values().forEach { preference -> | |
| ThemePreference.entries.forEach { preference -> |
| ThemePreference.LIGHT -> "Light" | ||
| ThemePreference.DARK -> "Dark" |
There was a problem hiding this comment.
Hardcoded strings "Light" and "Dark" should be replaced with localized string resources to support internationalization, ensuring consistency with other localized strings like settings_follow_system.
| ThemePreference.LIGHT -> "Light" | |
| ThemePreference.DARK -> "Dark" | |
| ThemePreference.LIGHT -> stringResource(Res.string.settings_theme_light) | |
| ThemePreference.DARK -> stringResource(Res.string.settings_theme_dark) |
| val displayString = when (pref) { | ||
| ThemePreference.SYSTEM -> "Follow system" | ||
| ThemePreference.LIGHT -> "Light" | ||
| ThemePreference.DARK -> "Dark" | ||
| } | ||
| _uiState.value = _uiState.value.copy(themeValue = displayString) |
There was a problem hiding this comment.
This mapping logic duplicates the string resolution found in the UI and uses hardcoded English strings. Additionally, since this is set in the init block, it might not update if the system language changes while the app is running. It is recommended to move display string resolution to the UI layer or use a more robust localization strategy for ViewModels.
| } | ||
|
|
||
| object ThemeStateHolder { | ||
| val themePreference = MutableStateFlow(ThemePreference.SYSTEM) |
There was a problem hiding this comment.
Exposing a MutableStateFlow directly in a public object breaks encapsulation. It's better to expose a read-only StateFlow and provide a dedicated method to update the value, ensuring that state changes are controlled and predictable.
…heme-picker-7433643405064918310 # Conflicts: # composeApp/src/commonMain/kotlin/dev/therealashik/github/settings/SettingsViewModel.kt
TheRealAshik
deleted the
feature/settings-theme-picker-7433643405064918310
branch
1 participant
This pull request introduces an in-memory theme picker to the application's settings, allowing users to toggle between system default, light, and dark modes. The selection is hoisted to
App.kt, ensuring immediate application of thecolorSchemethroughout the Compose Multiplatform UI. It does not introduce any persistent storage dependencies as per requirements.PR created automatically by Jules for task 7433643405064918310 started by @TheRealAshik