[Color] Fix dynamic color support check #4192
Conversation
| @@ -401,7 +401,7 @@ public static boolean isDynamicColorAvailable() { | |||
| if (VERSION.SDK_INT < VERSION_CODES.S) { | |||
| return false; | |||
| } | |||
| if (BuildCompat.isAtLeastT()) { | |||
| if (VERSION.SDK_INT >= VERSION_CODES.S_V2) { | |||
There was a problem hiding this comment.
Hi, shouldn't this be >= VERSION_CODES.TIRAMISU in order to be equivalent to BuildCompat.isAtLeastT()? Or > VERSION_CODES.S_V2.
Android T is the first version where all devices should support dynamic color by default, so we don't want to return true for S_V2.
There was a problem hiding this comment.
Hi, shouldn't this be
>= VERSION_CODES.TIRAMISUin order to be equivalent toBuildCompat.isAtLeastT()? Or> VERSION_CODES.S_V2.
No, that's not a mistake. When the BuildCompat.isAtLeastT() check was written, the library used androidx.core:core:1.7.0 (transitive dependency from androidx.appcompat:appcompat:1.4.0), and in it, the code name "T" meant the version that comes after "S" (see implementation).
There was a problem hiding this comment.
Android T is the first version where all devices should support dynamic color by default, so we don't want to return true for S_V2.
- Dynamic color has been available since Android 12.
- Dynamic color with monet, wallpaper color extraction, and stuff has been available since Android 12L.
?
There was a problem hiding this comment.
That's an interesting find re: the initial BuildCompat.isAtLeastT() implementation, but tbh I don't think that was intentional on our part.
Dynamic Color was initially introduced in Android 12, but OEMs had to opt-in to generate the system_ colors at runtime based on the wallpaper / user choice, otherwise those system_ colors would just remain at their hardcoded defaults. That's why we have the device / manufacturer allow list. I'm fairly certain that T is the first version where that behavior of updating system_ colors was baked into the Android Framework / OS by default, but I'll have to do some more digging to make sure.
There was a problem hiding this comment.
Regardless, I would say the current behavior of the library (with up-to-date dependencies), both internally and externally is that BuildCompat.isAtLeastT() means VERSION.SDK_INT >= VERSION_CODES.T. So I think we should follow that as opposed to reverting to the potentially initial / accidental behavior from years ago.
There was a problem hiding this comment.
The behavior you require is implemented in the Android 12 patches. They're included in Android 12L and above.
pubiqq
force-pushed
the
color/fix-dynamic-color-support-check
branch
from
f5e9f6b
to
f428ee3
Compare
pubiqq
force-pushed
the
color/fix-dynamic-color-support-check
branch
from
f428ee3
to
2145fe0
Compare
|
Ping |
No description provided.