material-foundation / material-theme-builder

Visualize dynamic color and create a custom Material Theme.
https://material-foundation.github.io/material-theme-builder/
Apache License 2.0
397 stars 27 forks source link

Incorrect custom colors #44

Closed tonyhallett closed 2 years ago

tonyhallett commented 2 years ago

Describe the bug In addition to Material Theme Builder Web - Export Android Views - custom color values same for unharmonized and harmonized there are also additional issues.

Custom color harmonized color roles generated with the Web Material Theme Builder are incorrect. image

In addition to this there are multiple issues with the generated code - Export / Jetpack Compose (Theme.kt).

  1. Missing imports
  2. undefined variable - errorHarmonize. Given that the functionality to harmonize errors is not currently available then I would expect that you create this variable and set to false in Color.kt
val seed = Color(0xFF6750a4)
val error = Color(0xFFba1b1b)
val color99 = Color(0xFFB77330)
val color99Harmonize = true
var errorHarmonize = false // add here
  1. In multiple places you are using the wrong types. Using Color when it should be an Int
  2. Calling methods that do not exist
    ColorRoles.initialize()
  3. Using properties that do not exist https://github.com/material-components/material-components-android/blob/f3d7ca40148d13ec6c9ad5a16f2f2b2a33aeba17/lib/java/com/google/android/material/color/ColorRoles.java#L28

fun setupErrorColors(colorScheme: ColorScheme, isLight: Boolean): ColorScheme { val harmonizedError = MaterialColors.harmonize(error, colorScheme.primary) val roles = MaterialColors.getColorRoles(harmonizedError, isLight) //returns a colorScheme with newly harmonized error colors return colorScheme.copy( error = roles.color, // do you mean accent onError = roles.onColor, // do you mean onAccent errorContainer = roles.colorContainer, // do you mean accentContainer onErrorContainer = roles.onColorContainer // do you mean onAccentContainer ) }

6. Using MaterialColors.getColorRoles - which [provides incorrect tones for dark theme](https://github.com/material-components/material-components-android/issues/2584)

7. Restricting the harmonized theme

@RequiresApi(Build.VERSION_CODES.S) @Composable fun HarmonizedTheme(

I think that this is only necessary for dynamicDarkColorScheme and dynamicLightColorScheme.
Alternative, that I have used in the provided repo below

@Composable fun HarmonizedTheme( useDarkTheme: Boolean = isSystemInDarkTheme(), isDynamic: Boolean = true, content: @Composable() () -> Unit ) { val canDynamicColor = Build.VERSION.SDK_INT >= Build.VERSION_CODES.S

val colors = if (isDynamic and canDynamicColor) {
    val context = LocalContext.current
    if (useDarkTheme) dynamicDarkColorScheme(context) else dynamicLightColorScheme(context)
} else {
    if (useDarkTheme) DarkThemeColors else LightThemeColors
}


Please see my [repo](https://github.com/tonyhallett/UseGeneratedTheme) that uses your generated code, with modifications, to show what the harmonized color values should be.
Run on an android device - displays
![image](https://user-images.githubusercontent.com/11292998/156027683-c9de36f4-2536-4d7e-a516-59eeeea29c0e.png)
![image](https://user-images.githubusercontent.com/11292998/156027763-e99e7d1a-8a54-47d2-97c0-bd115d2afa1e.png)

**To Reproduce**
Steps to reproduce the behavior:
1. Go to [Material Theme Builder Web ](https://material-foundation.github.io/material-theme-builder/#/custom)
2. Primary - 103, 80, 164, Custom - 183, 115, 48 Harmonize
3. Compare values to screenshot above

**Expected behavior**
Light and Dark Harmonized roles agree with screenshots above.
That the generated code can compile ( feasable that I have missed something as new to Jetpack Compose and Kotlin )
That the generated code will create custom color roles with the correct tones for dark theme.  

**Desktop (please complete the following information):**
 - OS: Windos 10
 - Browser: Chrome 
 - Version 98.0.4758.102 (Official Build) (64-bit)

**Additional context**
It is getting tiresome finding issue after issue.
jwill commented 2 years ago

The harmonized code for Compose was intended for an internal build that unfortunately made it into the external build. It depended on having a small port of a couple of functions from MaterialColors from @ColorInt to Color. Without it, the colors would be wrong or error.

It has been removed. Helper theme code for Compose color harmony will not be exported at this time. Sorry for the misunderstanding.