Theme: allow selective theme definitions

miraclx commented 3 years ago

Currently (2c4bbfa), it's all or nothing when it comes to theming, except with config.themes.background.

This PR extends that selective defaulting to all the options.

This also zips the outputs, ensuring there's always a Color for every entry (useful for Debug).

In summary, it lets you change the color for that one thing without needing to set colors for all things.

tarkah commented 3 years ago

Nifty! I'm usually not a fan of macros in favor of readability, but this is pretty straightforward.

Maybe I'm missing something, but what's the purpose of the zip function? It seems redundant since the getters for each color already return the default Color if that color is None. Each color can be none if either no custom theme was specified, or if it wasn't specified in the config. Either way, it's covered by the getter?

miraclx commented 3 years ago

It's clearer when you mix-in #77 to see the active theme information.

Instead of None-s all over when you skip definitions in your config file.

I know, it's a little redundant to have method calls that return values or defaults and still have a zip method

Here's how it's laid out in my mind.

If there is no theme section in the config file, serde uses <Theme as Default>::default() which as expected, returns "defined" defaults.
If an entry is missing, serde reads this as None for not being defined.
zip() creates a fully defined instance of the Theme struct based on items from the config or the defaults.
We could end here, and there would be no issue but we also need to keep those helper methods because the colors are Option-s and we would still need to unwrap.

A resolution to this might be using a private struct for the Option<Color> entries from the config theme definitions and a public one with just the Color-s pre-zipped with the defaults.

I just thought I'd get this in to get some feedback.

tarkah commented 3 years ago

Ahh, that makes sense. I'm not really sure how necessary #77 is as it doesn't tell you anything that the theme in config isn't already defined as. We've already validated that the theme is built correctly before pushing this out.

SHOW_DEBUG and DEBUG_MOUSE are useful to reference at runtime while developing, but #77 really wont be. I'd prefer to drop #77 and remove Debug and zip as it doesn't really have any value without #77.

miraclx commented 3 years ago

Closed #77!

tarkah commented 3 years ago

Thanks! I'll get the merged and back-merged into the candlestick PR once that is all finalized.

tarkah / tickrs

Theme: allow selective theme definitions #76