WIP: feat: Dark theme for AMOLED screens #1178
Conversation
- Also changed previews to include theme = 2 instead of isDarkMode = true
Yeah this isn't written the best and can be confusing. For now a better way to handle it would be to let the This breaks the ideal that core libraries shouldn't reference above them, but |
There was a problem hiding this comment.
Nice work, left a few comments, and hopefully #1178 (comment) clears up how we could use the theme?
| @@ -40,13 +40,15 @@ class MainActivity : ComponentActivity() { | |||
| /** | |||
| * Returns a [Pair] of (dark mode enabled) to (dynamic color enabled). | |||
| */ | |||
| private fun generateTheme(theme: Theme, isSystemDarkMode: Boolean): Pair<Boolean, Boolean> { | |||
| private fun generateTheme(theme: Theme, isSystemDarkMode: Boolean): Pair<Theme, Boolean> { | |||
There was a problem hiding this comment.
Based on the PR comment previously, this could probably be removed and the whole itself passed through to the NekomeTheme object
There was a problem hiding this comment.
All updated in latest commit deaa97d. No longer use generateTheme() at all, we just call
NekomeTheme( Theme.yourTheme) and the composable figures out the rest (Dynamic compatibility, system theming etc.)
| @@ -10,19 +10,28 @@ import androidx.compose.runtime.Composable | |||
| import androidx.compose.ui.platform.LocalContext | |||
| import com.google.accompanist.systemuicontroller.rememberSystemUiController | |||
|
|
|||
| /** | |||
| * Used to set the theme for a given composable, defaulting to the system theme | |||
There was a problem hiding this comment.
This is correct, but the ideal is that the theme is set for the view hierarchy once at the start, not for a single composable
There was a problem hiding this comment.
Addressed in commit deaa97d, now a bit less... or more? vague.
| @@ -137,6 +137,7 @@ | |||
| <string name="settings_theme_light">Light</string> | |||
| <string name="settings_theme_dynamic_dark">Dynamic Dark</string> | |||
| <string name="settings_theme_dynamic_light">Dynamic Light</string> | |||
| <string name="settings_theme_black">Black</string> | |||
There was a problem hiding this comment.
Maybe "AMOLED Black" would be clearer for the difference with dark mode?
There was a problem hiding this comment.
Addressed in commit deaa97d, changed both string resources. Wasn't sure whether to have the japanese one be
アモールド ブラックテーマ
or
AMOLED ブラックテーマ
currently using the former
| val BlackColorPalette = DarkColorPalette.copy( | ||
| background = Color(0xFF000000), | ||
| surface = Color(0xFF000000) | ||
| ) |
There was a problem hiding this comment.
It would be good to get the cards to also be 000000, but still retain usability, but that might be difficult. I think this is a great start to providing amoled though 👍
There was a problem hiding this comment.
Will look further into this to figure out what the general consensus is for AMOLED theming
* Removed generateTheme from mainActivity * Improved NekomeTheme to be more readable and concise * Changed values() to entries on enums as per android studios recommendation since Kotlin 1.9.0 * Updated strings to include AMOLED. (Unsure if we wanted katakana for the word AMOLED or just the way its written in english) ** Hidden previews in credentials screen and syncingScreen - These things likely just need their dependencies updated as well in order to use the new theming. Will get around to it on the next commit. Might want to look at removing it though * Made a default constructor for system default on theme for ease of preview * Updated core:compose dependency
|
Changed card colour to complete black, seems to still have functionality. Maybe could add some sort of faint light shadow to the bottom of the cards or something so you can tell where they end, but that might defeat the point of an AMOLED mode. |
Aiming to close issue #734
What it does
Adds a Black theme to the themes menu. Also changes a parameter in the NekomeTheme composable.
Potential oversights
Im unsure if there's a way to tell if the screen is AMOLED when setting a system theme, or if a black theme is something that android is supporting as a system theme right now. So i have not done anything in regards to setting Dynamic black or getting it set when its a system default
Issues
Readability
Currently, the way we get themes is a bit strange, relying on a pair containing 2 bools. While verbose it made it hard to adapt in its current state to include more themes.
I was looking at maybe using a Triple instead of a Pair, but i believe that may end up making things more confusing, especially when it comes to the logic of checking what theme is currently being used.
I wanted to opt for an Enum, but the enum class ...flags.Theme isn't accessible from ...Compose, so i ended up using the Enums value which isn't ideal but solves the problem immediately.
So currently when themes are checked & set within NekomeTheme it doesn't use an enum, and instead just goes through the when statement checking against ints.
Colour choices
I assume that all that really needs to be changed for this theme is setting the background and surface colours to black? Not entirely sure what else would need to be changed.
Visibility
In some weird case where you decide to log out whilst in Black mode, the Nekome logo will become completely unseeable.
I added some kDoc to NekomeTheme to hopefully explain my reasoning a bit to anyone that would stumble across this in the future lol.