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

Add dynamic colors #277

Merged
merged 31 commits into from
Dec 19, 2023
Merged

Add dynamic colors #277

merged 31 commits into from
Dec 19, 2023

Conversation

BlackyHawky
Copy link
Contributor

@BlackyHawky BlackyHawky commented Nov 17, 2023

This PR adds Dynamic colors and fixes partially #29 (Openboard settings will be made later in another PR, even if already done locally).

To know:

  • This theme only appears in the list of colors for Android versions above 12+.

  • The colors are based on those of Gboard (Google is the creator of dynamic colors, so it makes sense to use theirs).

  • If you change the color in your device's color palette, the keyboard theme is not updated; you must perform a forced close.
    This is not due to the addition of this new theme, as this "bug" is currently occurring.
    I think this is due to KeyboardSwitcher.updateKeyboardThemeAndContextThemeWrapper.
    I didn't modify this part of the code because I didn't know how to change it without influencing the existing themes.
    However, if you toggle the display on your device from light to dark, the keyboard colors update correctly.
    Fixed with commit 01c3eea thanks to Helium's help.

  • A new image for keyboard_popup_panel_background_rounded_base is provided because on my phone a 1 pixel white band was appearing under the popup selector. This was noticeable when the device was on the light theme.
    Maybe it's the same on other screens with different resolutions, but unfortunately I can't really tell because I don't have enough devices in my possession.
    Maybe in another PR we'll have to think about using xml drawables instead of 9-patch files to display popups.
    Solved with commit b2e2c9e

  • Finally, the dividers in the suggestion strip are green on my Samsung tablet with Android 13. It's impossible to know where this is coming from and only this device is affected. Solved thanks to Helium - commit c2f4b61


See screenshots

The screenshots are from my tablet, as it's the only device I have that has the Color Palette option.

Day Mode Night Mode

If you don't have the Color Palette option in your device (like on my Huawei phone with Android 12), here's what it looks like:

Day Mode Night Mode

@Helium314
Copy link
Owner

What's with all the logic around themeColors? The colors are provided when creating Colors, why are they modified again?

@BlackyHawky
Copy link
Contributor Author

What's with all the logic around themeColors? The colors are provided when creating Colors, why are they modified again?

I'm not sure I understand your question...

Dynamic colors don't behave the same way as other colors light, dark, etc. when the keys are pressed.
That's why I had to add specific cases only when dynamic colors are chosen by the user without influencing the other themes...
They are therefore not modified again. I'm wrong??

Authored-by: Helium314 <helium314@mailbox.org>
@Helium314
Copy link
Owner

Having the colors doing different things depeding on the colors is highly unexpected.
E.g. if the accentColorFilter doesn't use the accent color this is not nice already, but if it sometimes does and sometimes doesn't depending on where the colors come from this is very confusing behavior.

If the user sets excactly the same colors as the ones given to Colors by the new color set, both color sets should look the same.

@BlackyHawky
Copy link
Contributor Author

Having the colors doing different things depeding on the colors is highly unexpected.
E.g. if the accentColorFilter doesn't use the accent color this is not nice already, but if it sometimes does and sometimes doesn't depending on where the colors come from this is very confusing behavior.

If the user sets excactly the same colors as the ones given to Colors by the new color set, both color sets should look the same.

I don't understand your example because accentColorFilter uses the accent color. Indeed I wrote:

if (themeColors == THEME_DYNAMIC_LIGHT || themeColors == THEME_DYNAMIC_DARK ) colorFilter(doubleDarken(accent))

In this example, if I apply a double darkening for dynamic colors, it's to better highlight the selected emoji category compared to the background of the emoji category strip. It was mainly the Dynamic Light theme that was affected, but it's not disturbing to apply it to the Dynamic dark theme.

@Helium314
Copy link
Owner

I don't understand your example because accentColorFilter uses the accent color

doubleDarken(accent) is not accent, it's a color derived from accent.
Such differences might be clear to you as author, but e.g. when someone else is adding a new theme, they might wonder why sometimes accentColorFilter uses accent color, and sometimes not. Or not notice the difference becuase they only test with user-defined colors, and then users end up with bad contrast if they select dyamic colors.

Sticking to the "Principle of Least Surprise" is quite useful in advoiding hard-to-find bugs. This app violates that principle in too many places, and it's often a pain to deal with it.
E.g. MoreKeySpec.getIntValue returns the desired value, but als sets the source string of the value to null, which is clear when checking the code, but absolutely unexpected when just looking at the name.

@BlackyHawky
Copy link
Contributor Author

Such differences might be clear to you as author,

I totally agree with you. 😬

but e.g. when someone else is adding a new theme, they might wonder why sometimes accentColorFilter uses accent color, and sometimes not.

I hope this person will do what we do, i.e. understand what is already written... I'm a total beginner and yet I manage to understand (not often, it's true) what's coded...

Sticking to the "Principle of Least Surprise" is quite useful in advoiding hard-to-find bugs.

Thank you so much for telling me about this principle; I didn't know about it and tonight I read some interesting articles about it.


In relation to colors, when you said: " doubleDarken(accent) is not accent " , do you suggest replacing doubleDarken(accent) with something similar to adjustedBackground ?

I think I tried too hard to match Gboard's colors... ☹

@Helium314
Copy link
Owner

I hope this person will do what we do, i.e. understand what is already written... I'm a total beginner and yet I manage to understand (not often, it's true) what's coded...

It often depends on what kind of code you read... In some places the code is very much enclosed, and you only need check one or two files. Though there is InputLogic, where most of the stuff is happening in one file, but still it's quite convoluted and difficult to grasp, e.g. the different outcomes on backspace.
In other cases like the original keyboard parsing, if you want to understand what's going on you'll need to follow through many different files and function calls, a vast amount of possibilities coming from if or switch statements, the Android resources system and the keyboard texts table.

But an important point is also how simple it is to understand the code. If you stare long enough, you'll understand eventually. And simple understanding is very much coupled to clear function names (so you don't need to look up what it actually does) and straightforward code with few branches and special cases (which is usually hard due to necessary complexity).

In relation to colors, when you said: " doubleDarken(accent) is not accent " , do you suggest replacing doubleDarken(accent) with something similar to adjustedBackground ?

I don't know how exactly you want the colors to look like, but what I want is to get rid of the themeColors in Colors.
(Also the themeStyle switches bother me, but that was some necessary compromise to keep close to the old versions of holo and material.)

It would be ok for example to add some adjustedAccent, or allow setting the adjusted colors "manually", as long as they are close to the original colors.
Maybe we can go through what you want to have, I'm also ok with minor changes to other themes (e.g. using some adjustedAccent for the selected emoji tab).

@BlackyHawky
Copy link
Contributor Author

but what I want is to get rid of the themeColors in Colors.

Do you know another way of modifying all the stateLists only for dynamic colors without modifying the others?

As I said:

  • The colors are based on those of Gboard (Google is the creator of dynamic colors, so it makes sense to use theirs);
  • Dynamic colors don't behave the same way as other themes (light, dark, darker, etc.) when the keys are pressed.

First example with backgroundStateList :

THEME_DYNAMIC_LIGHT -> stateList(darken(functionalKey), background)

Second example, about actionKeyStateList :

if (themeColors == THEME_DYNAMIC_LIGHT) stateList(gesture, accent)

Third example with adjustedBackgroundStateList :

adjustedBackgroundStateList =
            } else if (themeColors == THEME_DYNAMIC_DARK) {
                if (hasKeyBorders) stateList(doubleDarken(accent), keyBackground)
See screenshots
1°/ backgroundStateList 2°/ actionKeyStateList 3°/ adjustedBackgroundStateList
When a suggestion is selected, it uses
the color of the darkened functional key.
The gesture color is used when the enter key is pressed If we use only the accent color, we'll see very little of the signs,
because the accent color is light and the signs are white.
So we apply a double darkening to the accent color.

For dynamic colors, which can use the colors of the wallpaper applied to the device, we can't simply darken or lighten key colors without being able to use the colors of other keys. Don't you agree?

@BlackyHawky
Copy link
Contributor Author

As I don't know how to modify stateLists only for dynamic colors, I've removed themeColors as you wished;
The Colors.kt file is now restored to its default values; I've only modified accentColorFilter and this will have almost no impact on existing colors.

@Helium314
Copy link
Owner

Sorry for my long non-reply. I didn't (and still don't) have enough time, and was rather occupied with working on the layout parser.

I had some thoughts on how the Colors could be re-arranged:
Use an interface with functions that set or return colors, for example

interface Colors {
    @ColorInt fun get(color: Color): Int
    fun setBackground(view: View, Color)
    fun setColor(drawable: Drawable, Color)
}

Color is some enum class, like the current BackgroundType. Could potentially have a lot of colors, like

enum class Color { KEY_TEXT, KEYBOARD_BACKGROUND, SUGGESTION_BACKGROUND, MORE_KEYS_BACKGROUND, MORE_SUGGESTIONS_BACKGROUND, NAV_BAR, ... }

The Color class can be expanded whenever needed.
(name of interface and enum class don't have to be that generic if you have a better idea)

Then instead of e.g. mColors.getActionKeyIconColorFilter() the call would be mColors.setColor(drawable, Colors.ACTION_KEY_ICON).
What happens to the drawable then depends on the implementation of Colors. So you could have class DynamicDarkColors() : Colors and set the colors, state lists, color filters, ... to anything you like, without influencing the current color system (which needs to be changed into another implementation of the Colors interface).

Does that sound good for you?

@BlackyHawky
Copy link
Contributor Author

BlackyHawky commented Nov 28, 2023

Sorry for my long non-reply.
I didn't (and still don't) have enough time, and was rather occupied with working on the layout parser.

Don't apologize, there's no problem! I can see that the layout parser is very complex and requires a lot of work, so once again, no worries!
At the moment, I don't have much time either because of my work, which tires him a lot.
In any case, thank you very much for your response.

The concepts you are talking about are new to me so I will do some tests this week and I will keep you informed.

@Helium314
Copy link
Owner

The interface can be pretty simple, here an extended example:

interface Colors {
    @ColorInt fun get(color: ColorType): Int
    fun setBackground(view: View, color: ColorType)
    fun setColor(drawable: Drawable, color: ColorType)
}

@RequiresApi(Build.VERSION_CODES.S)
class DynamicColors(context: Context) : Colors {
    private val keyText = ContextCompat.getColor(context, android.R.color.system_neutral1_200)
    private val background = ContextCompat.getColor(context, android.R.color.system_neutral2_700)
    private val nav = ContextCompat.getColor(context, android.R.color.system_accent1_400)
    override fun get(color: ColorType): Int = when (color) {
        ColorType.KEY_TEXT -> keyText
        ColorType.KEYBOARD_BACKGROUND, ColorType.SUGGESTION_BACKGROUND,
        ColorType.MORE_KEYS_BACKGROUND, ColorType.MORE_SUGGESTIONS_BACKGROUND -> background
        ColorType.NAV_BAR -> nav
    } 
    override fun setColor(drawable: Drawable, color: ColorType) {
        drawable.setTint(get(color)) // could be simliar to setBackgroundColor
    }
    override fun setBackground(view: View, color: ColorType) {
        setColor(view.background, color) // could be similar to setKeyboardBackground
    }
}

enum class ColorType { KEY_TEXT, KEYBOARD_BACKGROUND, SUGGESTION_BACKGROUND, MORE_KEYS_BACKGROUND, MORE_SUGGESTIONS_BACKGROUND, NAV_BAR }

class OldColors ( // renamed because of conflict
    private val themeStyle: String,
    private val hasKeyBorders: Boolean,
    private val accent: Int,
    private val gesture: Int,
    private val background: Int,
    private val keyBackground: Int,
    private val functionalKey: Int,
    private val spaceBar: Int,
    private val keyText: Int,
    private val keyHintText: Int,
    private val spaceBarText: Int
) : Colors {
// ... the old code, but using the interface functions to set color (all other funs and all vals private)

In KeyboardTheme.getThemeColors you can then return either OldColors or DynamicColors, because both implement the Colors interface.

Co-authored-by: Helium314 <helium314@mailbox.org>
@BlackyHawky
Copy link
Contributor Author

I was finally able to work on this PR.

I was inspired by the code you gave me and, indeed, dynamic colors no longer influence other colors without the need to add themeColors. Thank you very much for your help.

I hope you'll find this commit e92d177 better than what I suggested. 😅
In any case, don't hesitate to tell me if there's anything wrong.

@Helium314
Copy link
Owner

It's not fully what I had in mind, as the idea was to have a more generic interface (allowing more flexibility), but the current state in the PR is somewhere on the way there. Definitely good enough, thanks!.

If you change the color in your device's color palette, the keyboard theme is not updated; you must perform a forced close.

Can you check whether LatinIME.onConfigurationChanged and / or LatinIME.onInitializeInterface is called after changing the colors? It might only get called when you open the keyboard on a different input field than the previous one.

@BlackyHawky
Copy link
Contributor Author

It's not fully what I had in mind, as the idea was to have a more generic interface (allowing more flexibility), but the current state in the PR is somewhere on the way there. Definitely good enough, thanks!.

Without your help, I wouldn't have achieved this result (even if it isn't perfect 😬), so it's my turn to thank you.

Can you check whether LatinIME.onConfigurationChanged and / or LatinIME.onInitializeInterface is called after changing the colors? It might only get called when you open the keyboard on a different input field than the previous one.

Does the attached stacktrace answer your question?
I've separated it into 2 parts: the first when I change the color in the color palette, and the second when I open the keyboard.

Stacktrace.txt

@Helium314
Copy link
Owner

There is no logging in the functions I mentioned. My idea was to just add a log line, and check in Android Studio logcat viewer.

E.g. add Log.i(TAG, "onInitializeInterface");, and go to logcat viewer (near the bottom in Android Studio) and filter for onInitializeInterface. You can set whatever tag and log line you want, that's just an example.

What's interesting is that there are some log lines regarding ResourcesManager when changing the theme, but looks like ResourcesManager is only internal for the Android system, and not something than can be used from an app.

@BlackyHawky
Copy link
Contributor Author

Cool, you've taught me something again!

I can confirm that the 2 functions are called up without opening the keyboard and just after changing the color in the palette color.

Log.txt

@Helium314
Copy link
Owner

Ok, then you could use the fact that in both cases KeyboardSwitcher.updateKeyboardThemeAndContextThemeWrapper get called.
Use the changed branch of the if when the colors changed.

For checking you can add another function to the Colors interface, like haveColorsChanged(context). Using this you can read the system colors again, and return true if any of the colors is different to what is stored in the current instance of DynamicColors.

@BlackyHawky
Copy link
Contributor Author

Ok, then you could use the fact that in both cases KeyboardSwitcher.updateKeyboardThemeAndContextThemeWrapper get called.
Use the changed branch of the if when the colors changed.

I don't understand your sentence: "Use the changed branch of the if when the colors changed."

I've made dozens of attempts and now I'm still lost... 😭

@Helium314
Copy link
Owner

In the branch that triggers when anything changed:

        if (mThemeContext == null || !keyboardTheme.equals(mKeyboardTheme) || nightModeChanged
                || !mThemeContext.getResources().equals(context.getResources())) {

Add the color change to the conditions in the if, then I hope it will work.

if the colors in the color palette are changed

Co-authored-by: Helium314 <helium314@mailbox.org>
@Helium314
Copy link
Owner

So in I understood right, the plan was to make a PR based on another branch, probably Todo_Colors.kt?
Or you could just merge those changes into the Dynamic_color branch I guess.

There are some minor things, but still looks good.

@BlackyHawky
Copy link
Contributor Author

great! I'll take care of making the changes to the TodoColor branch here.

@BlackyHawky
Copy link
Contributor Author

Done 👍

ColorType.SPACEBAR_TEXT -> spaceBarText
ColorType.NAV_BAR -> navBar
ColorType.MORE_SUGGESTIONS_HINT, ColorType.SUGGESTED_WORD, ColorType.TYPED_WORD, ColorType.VALID_WORD -> adjustedKeyText
else -> Color.WHITE
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all ColorTypes should return a chosen color. This is essentially a / the main reason to use enum and not some int assigned to each color.
When explicitly assigning a color to all values, it's impossible to forget setting a color (especially considering that new values might get added).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I didn't know I had to list everything.
Done with commit a5af535.

@Helium314
Copy link
Owner

I did some adjustments so the names and the interface are closer to what I had in mind.
The colors should be unchanged. I could only test for old colors, but same should apply to dynamic colors.

@BlackyHawky
Copy link
Contributor Author

I can confirm that the colors are identical.
Thank you.

@Helium314 Helium314 merged commit 0edbf9c into Helium314:new Dec 19, 2023
@BlackyHawky
Copy link
Contributor Author

I notice that you didn't co-author... you helped me enormously on this PR so please, can you not forget this little detail again?
Especially since I don't take much credit for this PR because in the end I just provided the colors...
Thanks for everything.

@Helium314
Copy link
Owner

I don't understand... the commit says it's co-authored by me: 0edbf9c

@BlackyHawky
Copy link
Contributor Author

You have to enter the commit in the GitHub mobile application to see it. In the commits list, you don't see the avatars side by side like on the internet.

Very glad you did and thanks again for helping me on this PR.

@BlackyHawky BlackyHawky deleted the Dynamic_color branch December 19, 2023 18:55
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.

2 participants