Skip to content
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

[dynamic_color] Add tone-based colors to color scheme #599

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

hasali19
Copy link

@hasali19 hasali19 commented Jul 7, 2024

Description

Adds the new tone-based colors from Flutter 3.22 to the color scheme.

On API >= 34, the color values are retrieved directly from the Android OS.

On API >= 31, the existing core palette will continue to be used, with the mappings taken from https://github.com/material-components/material-components-android/blob/master/docs/theming/Color.md.

Issues

Fixes #574, #582

Checklist

@hasali19 hasali19 requested a review from a team as a code owner July 7, 2024 22:39
@hasali19 hasali19 requested review from guidezpl and removed request for a team July 7, 2024 22:40
@Henry-Hiles
Copy link

I'm trying to use this, but I get this error: When a TonalPalette is created with fromList, tone must be one of [0, 10, 20, 30, 40, 50, 60, 70, 80, 90, 95, 99, 100]

@hasali19
Copy link
Author

hasali19 commented Jul 9, 2024

Interesting...I don't think I've changed anything related to that. Does the latest main branch work fine for you?

@Henry-Hiles
Copy link

Interesting...I don't think I've changed anything related to that. Does the latest main branch work fine for you?

Well, from pub it doesn't have the correct colours, but no errors. I switched the pubspec to use this fork, and boom: error. I'll try main asap.

@Henry-Hiles
Copy link

Interesting...I don't think I've changed anything related to that. Does the latest main branch work fine for you?

Latest main works fine. Just this branch :)

@Henry-Hiles
Copy link

I'm using DynamicColorBuilder ofc.

@hasali19
Copy link
Author

hasali19 commented Jul 9, 2024

Which flutter version are you using?

@Henry-Hiles
Copy link

quadradical@quadraticpc ~/D/C/qlib (master)> flutter --version
Flutter 3.22.2 • channel stable • https://github.com/flutter/flutter.git
Framework • revision nixpkgs000 () • 1970-01-01 00:00:00
Engine • revision edd8546116
Tools • Dart 3.4.3 • DevTools 2.34.3

@hasali19
Copy link
Author

hasali19 commented Jul 9, 2024

Ok, I think I see the problem. Support for arbitrary colour tones were added in material_color_utilities 0.11.0, but stable flutter depends on 0.8.0. I just realised I was using the beta channel, which is why I didn't run into it.

I think the solution will be to sidestep material_color_utilities and try to get the colours from the native side for the API < 34 case as well.

@Henry-Hiles
Copy link

Oh, I see, thanks!

@hasali19
Copy link
Author

hasali19 commented Jul 9, 2024

I've taken a different approach of implementing logic to "guess" tones that are missing from the common tones list, based on the implementation in material_color_utilities. This will be needed until flutter supports the new version.

@Henry-Hiles Can you confirm if this works for you now?

@Henry-Hiles
Copy link

Sure, I'll test right now!

@Henry-Hiles
Copy link

While this no longer has an error, it still has the same issues as without this PR (very similar colors so nothing looks correct)

@Henry-Hiles
Copy link

Screenshot_20240709-185210_QJobProcessor

@Henry-Hiles
Copy link

As you can see, the navbar and background are the same, which isn't normal.

@hasali19
Copy link
Author

hasali19 commented Jul 9, 2024

I can't seem to reproduce that. Do you have an example repo/code you can share?

Screenshot_1720566342

@Henry-Hiles
Copy link

I can't seem to reproduce that. Do you have an example repo/code you can share?

Screenshot_1720566342

Hmm it happened when I upgraded to 3.22.2, I'll see if I can make a test repo.

@Henry-Hiles
Copy link

Nevermind deleting pub cache and re-getting fixed it! Thank you for this!

@resucutie
Copy link

After building my application using it, the primary color isn't accurate
image
image

@hasali19
Copy link
Author

Which android version are you using?

@resucutie
Copy link

resucutie commented Jul 14, 2024

Android 14. I could give the project to you try it out seeing what's wrong

@hasali19
Copy link
Author

On Android 14, this should be using the primary colour that is provided directly by the system.

Could you share your flutter version, debug logs and a sample project if possible.

@JunioJsv
Copy link

JunioJsv commented Jul 14, 2024

There's something wrong with the implementation. I just tested it in my project, and the colors don't match those of the system, which is predominantly green. However, in the app, the predominant color is dark blue. I'm using Android 14 and Flutter 3.22.0.

WhatsApp Image 2024-07-14 at 14 49 49
WhatsApp Image 2024-07-14 at 14 49 20

@JunioJsv
Copy link

Here's the link to the project where I ran the test: https://github.com/JunioJsv/minimum-flutter

image

@hasali19
Copy link
Author

I'm not able to reproduce the issue on my device.

Could you share the debug logs when you run the app. In particular, do you see a line similar to:

I/flutter (18079): dynamic_color: Color scheme detected.

@hasali19
Copy link
Author

It seems this may be a common issue on Samsung devices, affecting the official Android material library as well - material-components/material-components-android#3924. Samsung reports incorrect values for the new colours added in Android 14.

Do you see dynamic colours working correctly in other apps, e.g. Google apps?

Given this is a Samsung specific issue, not really sure what we can do here.

@JunioJsv
Copy link

It seems this may be a common issue on Samsung devices, affecting the official Android material library as well - material-components/material-components-android#3924. Samsung reports incorrect values for the new colours added in Android 14.

Do you see dynamic colours working correctly in other apps, e.g. Google apps?

Given this is a Samsung specific issue, not really sure what we can do here.

Indeed, the Google apps are also displaying the wrong tone, dark blue, and my device is a Samsung A54.

@resucutie
Copy link

I've decided to finally publish a "just build it" branch of my project: https://github.com/resucutie/localbooru/tree/dynamic-colors-pr-fix-test

@hasali19 feel free to build and test it

@hasali19
Copy link
Author

@resucutie Thanks, I'll take a look.

Could you also share your flutter version please?

@hasali19
Copy link
Author

From what I can see this is working as expected; Android's quick settings tiles use a slightly different shade of the primary colour.

You can see here that the shadeActive colour (used by the Android system for the quick settings tiles) uses tone 90 of the primary colour, while the material library defines primary in the colour scheme as tone 40 in light mode and 80 in dark mode.

@resucutie
Copy link

Welp, what about in comparasion to other apps? It looks odd, or I guess it is the consensus here, like if it is using the wrong color shade, but I am not sure

@hasali19
Copy link
Author

Do you mean you are seeing different colours compared to other apps using the same dynamic colour scheme?

This implementation should be using the same colours as those used by the official native material library. If this is not the case, please share a repro.

@resucutie
Copy link

I've (finally) tested it and apparently it indeed take the correct colors. It is a bit odd that it looks different on an emulator
Screenshot_20240730-170947_LocalBooru.png

Screenshot_20240730-170950_Settings.png

PR is good I think

@wildsylvan
Copy link

wildsylvan commented Aug 9, 2024

in our case, after activating dynamic colors (using this commit):
image
primary container e secondary container became the same color!


look at the cards (list a/b and the new list fab. they should be different colors but with dynamic colors they become the same)

without dynamic colors (correct)

Screenshot_2024-08-09-07-07-49-47_304e098f875053d2a3a592b681756698

with dynamic colors (wrong)

Screenshot_2024-08-09-07-07-28-30_304e098f875053d2a3a592b681756698

@hasali19
Copy link
Author

hasali19 commented Aug 9, 2024

Please can you share your flutter and android versions, device information, debug logs and a link to a code repo if possible.

@wildsylvan
Copy link

wildsylvan commented Aug 9, 2024

thanks for the quick reply! here:

[√] Flutter (Channel beta, 3.24.0-0.2.pre, on Microsoft Windows [Version 10.0.22631.3880], locale en-US)
[√] Windows Version (Installed version of Windows is version 10 or higher)
[√] Android toolchain - develop for Android devices (Android SDK version 35.0.0)
[√] Chrome - develop for the web
[X] Visual Studio - develop Windows apps
    X Visual Studio not installed; this is necessary to develop Windows apps.
      Download at https://visualstudio.microsoft.com/downloads/.
      Please install the "Desktop development with C++" workload, including all of its default components
[√] Android Studio (version 2024.1)
[√] VS Code (version 1.92.1)
[√] Connected device (3 available)
[√] Network resources

testing on a real phone, oneplus 8t with android 14
image


i can provide a repro next week... it's very vanilla use of material components tho, not many customizations

@wildsylvan
Copy link

this is the oneplus color settings:
image

@wildsylvan
Copy link

ok these impact how colors are shown in the app:
image

but the difference between primary container e secondary container remains basically non-existent

@wildsylvan
Copy link

@hasali19 what would be the best way to ignore completely oneplus color presets, just get the "main accent color" from android and utilize that color to build a correct material 3 color palette?

@hasali19
Copy link
Author

hasali19 commented Aug 9, 2024

Thanks for the info. Given that you are using Android 14 where the colours are retrieved directly from the system without any conversion, it is likely OnePlus is simply reporting both colours as the same.

One thing you could try is to force use of the fallback method used for older versions of Android, e.g:

final corePalette = await DynamicColorPlugin.getCorePalette();
final lightDynamic = corePalette?.toColorScheme(brightness: Brightness.light);
final darkDynamic = corePalette?.toColorScheme(brightness: Brightness.dark);

If you just want a specific "accent" colour, you can pick one from the fields on the colour schemes provided by DynamicColorBuilder.

@wildsylvan
Copy link

wildsylvan commented Aug 10, 2024

update


final corePalette = await DynamicColorPlugin.getCorePalette();
final lightDynamic = corePalette?.toColorScheme(brightness: Brightness.light);
final darkDynamic = corePalette?.toColorScheme(brightness: Brightness.dark);

doesn't fix the issue


my colleague tested with his Pixel 7 Pro (and also inside an emulator) and he has the same issue

the issue seems to be present only in light mode tho (and our theme settings don't have any special settings for only one of the modes):

wrong in light mode

image

correct in dark mode

image

@wildsylvan
Copy link

wildsylvan commented Aug 10, 2024

this is the card (the one with the text Test):
image

this is the fab:
image

and this is the theme config:
image

@hasali19
Copy link
Author

Although it's subtle, the colours in your screenshot do appear to be slightly different. I'm also able to reproduce very similar looking colours in light mode when running in an emulator.

image

@wildsylvan
Copy link

wildsylvan commented Aug 10, 2024

yeah i was just about to write it! they are a little different... so it's more of a general issue with the way the m3 color generator works?

@wildsylvan
Copy link

with a green wallpaper the difference is less subtle too...

image

@hasali19
Copy link
Author

Right, I guess it's just an artifact of whatever algorithm Android uses to pick the system colours.

@wildsylvan
Copy link

wildsylvan commented Aug 10, 2024

well to be fair the material theme builder shows it as well with the blues (tho the difference is a little more visible):
image

while with the greens the difference is much less subtle:
image

@guidezpl is it supposed to be that different between hues?

petlyh added a commit to petlyh/JS-Dict that referenced this pull request Sep 24, 2024
While `surfaceVariant` has been deprecated in favor of `surfaceContainerHighest`, the `dynamic_color` package does not yet support it, leading to an incorrect palette.

Ignoring the deprecation warnings while waiting for material-foundation/flutter-packages#599.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using dynamic_color on Flutter 3.21.0-1.0.pre.2 yields weird/incorrect color palette
5 participants