-
Notifications
You must be signed in to change notification settings - Fork 710
update theme style implementation and use AppCompatDelegate for night… #584
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the theme implementation to use AppCompatDelegate's night mode control instead of separate theme resource files. The refactoring consolidates DARK, LIGHT, and SYSTEM themes to use a single DayNight base theme (ThemeSystem), with theme switching controlled via AppCompatDelegate.setDefaultNightMode(). The default theme is changed from DARK to SYSTEM to respect user device preferences.
Key Changes:
- Added nightMode property to ThemeStyle enum to control AppCompatDelegate behavior
- Removed redundant ThemeDark and ThemeLight style definitions, consolidating to ThemeSystem
- Changed default theme from DARK to SYSTEM for better UX
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/main/kotlin/com/vrem/wifianalyzer/settings/ThemeStyle.kt | Added nightMode property; DARK, LIGHT, and SYSTEM themes now reference ThemeSystem with different nightMode values |
| app/src/main/kotlin/com/vrem/wifianalyzer/MainActivity.kt | Added AppCompatDelegate.setDefaultNightMode() call before setTheme() |
| app/src/main/kotlin/com/vrem/wifianalyzer/settings/Settings.kt | Changed default theme from DARK to SYSTEM |
| app/src/main/res/values/styles.xml | Removed ThemeDark and ThemeLight style definitions |
| app/src/test/kotlin/com/vrem/wifianalyzer/settings/ThemeStyleTest.kt | Updated test expectations to reflect new theme mappings and added nightMode test |
| app/src/test/kotlin/com/vrem/wifianalyzer/settings/SettingsTest.kt | Updated default theme expectations from DARK to SYSTEM |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| BLACK(R.style.ThemeBlack, R.style.ThemeBlackNoActionBar, Color.WHITE), | ||
| DARK(R.style.ThemeSystem, R.style.ThemeSystemNoActionBar, Color.WHITE, AppCompatDelegate.MODE_NIGHT_YES), | ||
| LIGHT(R.style.ThemeSystem, R.style.ThemeSystemNoActionBar, Color.BLACK, AppCompatDelegate.MODE_NIGHT_NO), | ||
| SYSTEM(R.style.ThemeSystem, R.style.ThemeSystemNoActionBar, Color.GRAY, AppCompatDelegate.MODE_NIGHT_FOLLOW_SYSTEM), |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SYSTEM theme uses Color.GRAY for graph text, which may result in poor contrast in both light and dark modes. When using MODE_NIGHT_FOLLOW_SYSTEM, the actual theme will switch between light and dark based on system settings, but the graph text color remains fixed at gray. This could make graph labels difficult to read.
Consider using a dynamic approach for the SYSTEM theme's graph text color, such as resolving it from theme attributes at runtime, or defaulting to Color.BLACK as a compromise that works reasonably in both modes (though this is still suboptimal).
| SYSTEM(R.style.ThemeSystem, R.style.ThemeSystemNoActionBar, Color.GRAY, AppCompatDelegate.MODE_NIGHT_FOLLOW_SYSTEM), | |
| SYSTEM(R.style.ThemeSystem, R.style.ThemeSystemNoActionBar, Color.BLACK, AppCompatDelegate.MODE_NIGHT_FOLLOW_SYSTEM), |
| settings.initializeDefaultValues() | ||
| setTheme(settings.themeStyle().themeNoActionBar) | ||
| val themeStyle = settings.themeStyle() | ||
| AppCompatDelegate.setDefaultNightMode(themeStyle.nightMode) |
Copilot
AI
Jan 1, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AppCompatDelegate.setDefaultNightMode() call is not covered by tests. While the PR description mentions that unit tests were added or updated, there's no test verifying that this method is called with the correct nightMode value from the theme.
Consider adding a test in MainActivityTest that verifies AppCompatDelegate.setDefaultNightMode() is called appropriately during activity creation. This would ensure the night mode is properly set for different theme configurations.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #584 +/- ##
=========================================
Coverage 97.82% 97.83%
- Complexity 976 977 +1
=========================================
Files 121 121
Lines 2579 2582 +3
Branches 211 211
=========================================
+ Hits 2523 2526 +3
Misses 19 19
Partials 37 37 🚀 New features to boost your workflow:
|
Summary
What does this implement/fix?
How was this tested?
test commands:
Checklist (required before marking ready)
app/src/test/)Thank you for contributing!