Theme manager strawman #272
Conversation
| @type pallette :: %{ | ||
| atom() => Scenic.Color.t() | ||
| } |
There was a problem hiding this comment.
We assume that a pallette is something like:
%{
primary: Scenic.Color.named().red(),
secondary: Scenic.Color.named().yellow()
}I wonder if we want to scope-creep this to be any sort of fill or pattern?
There was a problem hiding this comment.
Given the types here we can also just use simple atom names for the colors (via the implicit color mechanism) in Scenic.Color
%{
primary: :red,
secondary: :yellow
}But of course the benefit to the long approach is that you'll get an error earlier if you try to use an invalid color name.
There was a problem hiding this comment.
I would like the type of a color to be Scenic.Color.t(), which allows for either format of color.
%{
primary: :red,
secondary: :yellow
}
is a very convenient shorthand.
But sometimes you want to be explicit, or add an alpha channel, so that needs to be supported too.
| def put_setting(%__MODULE__{} = theme, :font, font_name), do: %{theme | font: font_name} | ||
|
|
||
| def put_setting(%__MODULE__{} = theme, :pallette, pallette) when is_map(pallette), | ||
| do: %{theme | pallette: pallette} |
There was a problem hiding this comment.
I'm not 100% sold on this chaining pattern, but I've seen it other places and it doesn't bother me too much.
There was a problem hiding this comment.
+1 from me to the chaining, I generally like chaining on my structs.
| table_handle = | ||
| :ets.new(@ets_name, [:set, :protected, :named_table, {:read_concurrency, true}]) |
There was a problem hiding this comment.
So, we setup a named table here so that anybody can read from it without the full genserver cost.
I'm curious if this function should also setup a default style pulled from either the settings or some normal Scenic default.
There was a problem hiding this comment.
I agree that there should be a default scenic theme. That would avoid user code from having to treat the "no theme" case separately.
There was a problem hiding this comment.
Absolutely yes on the default scene. I vote for :dark, but that might be just me...
|
One of the big questions before moving forward is...do we want this thing to have knowledge of an "active" theme? Or should viewports/scenes track that themselves and ask for it before doing drawing? |
| @@ -0,0 +1,57 @@ | |||
| defmodule Scenic.Theme do | |||
There was a problem hiding this comment.
I'm 100% in favor of having separate %Theme{} and theme manager modules. We have a few options for the naming scheme:
ThemeandThemeManagerThemeandTheme.ManagerThemes.ThemeandThemes.Manager(i.e. Phoenix context style)
I have an inclination for the first or third but I don't have a super strong feeling.
There was a problem hiding this comment.
I thought about this a bit more, and kind of doubling-down on the Phoenix convention, Theme and Manager might be the way to go.
| * `pallette` , which takes a `pallette()`. | ||
| """ | ||
| @spec put_setting(t(), atom, String.t() | pallette()) :: t() | ||
| def put_setting(%__MODULE__{} = theme, :font, font_name), do: %{theme | font: font_name} |
There was a problem hiding this comment.
We probably want to replace this with a plain put function. And possibly a put_pallette or put_color that would update a color within a pallette, alternatively we could tell users that they need to do:
theme = %Theme{} = get_theme_from_somewhere()
pallette = Map.put(theme.pallete, :secondary, :blue)
Theme.put(theme, :pallete)
| * `font` , which takes a `String.t`. | ||
| * `pallette` , which takes a `pallette()`. | ||
| """ | ||
| @spec put_setting(t(), atom, String.t() | pallette()) :: t() |
There was a problem hiding this comment.
Since we're not allowing arbitrary keys we can restrict the type further (although you likely elided this since this is just a strawman). It would look something like:
| @spec put_setting(t(), atom, String.t() | pallette()) :: t() | |
| @type setting :: :font | :font_size | :pallette | |
| @spec put_setting(t(), setting(), String.t() | pallette()) :: t() |
| @spec create(any) :: {:ok, t()} | {:error, String.t()} | ||
| def create(name) when is_atom(name), do: {:ok, %__MODULE__{name: name}} | ||
| def create(_), do: {:error, "Themes must be named with an atom."} |
There was a problem hiding this comment.
Personally since this is just creating a struct instance and not saving it to ETS or a file I would call this function new/1 and new!/ instead of "create". That's just my 2c though.
There was a problem hiding this comment.
I think I've probably been a bit inconsistent here and have used build/n as well.
new/1 and new!/1 feel better
| Creates a theme object with a given name. | ||
| """ | ||
| @spec create(any) :: {:ok, t()} | {:error, String.t()} | ||
| def create(name) when is_atom(name), do: {:ok, %__MODULE__{name: name}} |
There was a problem hiding this comment.
We also discussed having a version of this that would take a bulk update of settings (similar to assigns) so the user doesn't have to repeatedly call put
| * `font` is a String of the font that a theme should use, or nil if no preference. | ||
| * `pallette` is a map mapping atoms like `:primary` or `:secondary` to a `Scenic.Color`. | ||
| """ | ||
| @type t :: %__MODULE__{ |
There was a problem hiding this comment.
I'm not sure what Scenic's library policy is, but I'm a big fan of https://hex.pm/packages/typed_struct. If we pulled it in, it would make this type much more convenient to write.
There was a problem hiding this comment.
I think it'd be cool to add that all in a big typespec pass later!
There was a problem hiding this comment.
Yeah OK. I hadn't seen typed_struct before. Looks very cool. I'm open to bringing it in.
Description
Strawman proposal for #267 , as discussed with @boydm .
I'm mostly testing the waters to see if this is heading in the direction we want.
Motivation and Context
See #267 .
Types of changes
This adds a theme manager, which can be used to store various themes for use.
not work as expected)
but make things better)
Checklist