garretyoder / Colorful

Android runtime theme library
Apache License 2.0
2.13k stars 194 forks source link

small improvements #28

Closed MFlisar closed 6 years ago

MFlisar commented 6 years ago

1) I added the base interface CBaseActivity, so this can be extended from any activity and one must only call two functions to get the same functionality as with CActivity and CAppCompatActivity. See the simple example here: https://github.com/MFlisar/Colorful/blob/master/library/src/main/java/io/multimoon/colorful/CAppCompatActivity.kt => I did not change anything in there, it's just the common code of the 2 default activities 2) I added some logging infos (theme string e.g.) 3) auto formatted some of the files with default code styles 4) custom theme is removed from preferences if you pass 0 as custom theme

MFlisar commented 6 years ago

PS: I would like to add following things as well:

1) a string array with all colors + a string array with int mappings for colors like following:

    <string-array name="theme_color_names">
        <item>Green</item>
        <item>Red</item>
        <item>Pink</item>
        <item>Blue</item>
        ...
    </string-array>

    <string-array name="theme_color_names_values">
        <item>0</item>
        <item>1</item>
        <item>2</item>
        <item>3</item>
        ...
    </string-array>

This arrays can be used in preferences then and will allow to select any existing theme color. Not sure if you think this should be part of your lightweight library or not though, let me know what you think

garretyoder commented 6 years ago

I appreciate the code clean, it's something I haven't had time to do yet. Was your PR a fix for this issue? Also, check your log and see if you get the logmessage of Colorful trying to apply a custom user style. I can't reproduce this issue.

MFlisar commented 6 years ago

Was your PR a fix for this issue?

Which one? This one: https://github.com/garretyoder/Colorful/issues/27 ? No, no fix for this. And to your logging questions, yes I always see all relevant log messages but UI does not change... Which is weitrd, because order of functions is the same after a restart and it should lead to the same result... My theme has default colors in it, those are probably overwriting the custom values as the theme is merged as the end, but still, it works if I change the setting and the activity recreates itself. Imho, it makes more sense to apply the custom theme instead of the colorful default theme and merge the colors into it afterwards, this is what I do now

PR at least solves https://github.com/garretyoder/Colorful/issues/25 as a side effect and therefore I would suggest to remove the info about to use Colorful().apply(this, override = true, appcompat = false) and use the interface instead if you merge my pull request... Or put the functions from the interface into the utility class if you prefer this

MFlisar commented 6 years ago

One question:

ColorPack.light() is somehow not consistant, as it is the normal / default color. Would you mind renaming it? To normal e.g.? I would like to write an extension for this class that gives me a light version of the default color, because I use a light version of the theme color to tint background images in my app

garretyoder commented 6 years ago

I appreciate the work it's much needed, however I will probably rename the CBaseActivity to something more descriptive like ThemeInterface when I push 2.1 because it's a interface, not a activity.