Add a Theme type that is more easy to edit in a configuration file

[VisenDev]

Alternative PR to #128

Before I starting converting current theme definitions to use the new format. @david-vanderson I wanted to get your feedback on this.

Example theme using new format

jungle.json

{
  "name": "Jungle",
  "font_size": 18,
  "font_name_body": "Pixelify",
  "font_name_heading": "Pixelify",
  "font_name_caption": "Pixelify",
  "font_name_title": "Pixelify",
  "color_focus": "#638465",
  "color_text": "#82a29f",
  "color_text_press": "#97af81",
  "color_fill_text": "#ffffff", 
  "color_fill_container": "#2b3a3a",
  "color_fill_control": "#2c3334",
  "color_fill_hover": "#334e57",
  "color_fill_press": "#3b6357",
  "color_border": "#60827d"
}

[david-vanderson]

Looks pretty good so far. What happens if someone wants to override the font size adjustments baked into toTheme()?

[iacore]

Maybe every field should have a load priority like this

1. read from JSON field .font_caption.size
2. .font_size * 0.7

Makes me think of CSS.

[VisenDev]

Looks pretty good so far. What happens if someone wants to override the font size adjustments baked into toTheme()?

I'm not sure how common of a use this would be, but if it would be used by most themes I can add support. Otherwise users could just use a Theme instead of a QuickTheme

The point of a QuickTheme is to contain the minimal, intuitive, and self-descriptive set of values that you actually will want to change when making a theme in order to simplify Theme creation. Whatever this minimal set of values should be is up for discussion

So if something like

  "font_name_body": "Pixelify",
  "font_size_body": 18,
  "font_name_heading": "Pixelify",
  "font_size_heading": 16,
  "font_name_caption": "Pixelify",
  "font_size_caption": 14,
  "font_name_title": "Pixelify",
  "font_size_title": 20,

or

"font_base_size": 18
"font_name_body": "Pixelify",
"font_scale_body": 1,
"font_name_heading": "Pixelify",
"font_scale_heading": 0.8,
"font_name_caption": "Pixelify",
"font_scale_caption": 0.7,
"font_name_title": "Pixelify",
"font_scale_title": 2.0,

would be more useful, then that can be changed

It could also be possible to make certain fields optional

[david-vanderson]

That sounds reasonable. I say go ahead and we can add extra fields or priority things if anyone needs them. Thanks!

[VisenDev]

This change reduces ~645 lines of theme definition zig code to ~85 lines of json

[david-vanderson]

I have some questions:

• Looks like this changes a bunch of values of the themes, did you intend that?
  • Adwaita font sizes got changed, and font_title switched from Vera to VeraBd
• The fonts we ship should always have an integer size, otherwise they'll be scaled (which usually looks bad) - does your setup always have snap_to_pixels on?
• getList() seems overly complicated - do we even need to cache themes? Seems like an StringArrayHashMap backed by the gpa would be enough?
• In Database.init(), this code is not guaranteed to work:

const alloc = self.arena.allocator();
self.themes = std.StringHashMap(Theme).init(alloc);

because alloc internally has a pointer to self.arena, which is stack memory (unless you get lucky with result location semantics). Does that make sense?

[VisenDev]

• Looks like this changes a bunch of values of the themes, did you intend that?

There are some slight discrepancies in the themes before and after. I tried to get things as close as I could but it could still use some fine tuning.

I can add back the adwaita theme zig code as a default for now.

• Adwaita font sizes got changed, and font_title switched from Vera to VeraBd

I probably passed over this, i can adjust

• The fonts we ship should always have an integer size, otherwise they'll be scaled (which usually looks bad) - does your setup always have snap_to_pixels on?

I was under the impression that font sizes were floored later elsewhere in the codebase such as line 567 in dvui.zig

        var pixel_size = @as(u32, @intFromFloat(@max(min_pixel_size, @floor(font.size))));

But perhaps I misunderstood this code. Either way I can adjust the code to make sure all font sizes are rounded to the closest integer in the QuickTheme code.

• getList() seems overly complicated - do we even need to cache themes? Seems like an StringArrayHashMap backed by the gpa would be enough?

Ah, I wasn't even aware of the StringArrayHashMap. The main reason I implemented things this way was the guarantee that iterating thru all existing themes is fast.

However, the other reason I am using a theme cache is that it allows us to change the order that the themes are iterated thru. Thus, this function can guarantee that the list of themes will be in alphabetical order

• In Database.init(), this code is not guaranteed to work:

const alloc = self.arena.allocator();
self.themes = std.StringHashMap(Theme).init(alloc);

because alloc internally has a pointer to self.arena, which is stack memory (unless you get lucky with result location semantics). Does that make sense?

Okay, I can adjust this

[david-vanderson]

I can add back the adwaita theme zig code as a default for now.

That sounds good - it will also be useful for people looking at the code to see a "raw" theme in addition to the serializable ones.

I was under the impression that font sizes were floored later elsewhere in the codebase such as line 567 in dvui.zig

We have to floor the size when we ask freetype/stb_truetype to load a font and rasterize the glyphs, but in renderText() (and textSizeEx()) the glyph sizes are scaled back up to match the Font size if snap_to_pixels is false. The idea is that on a low dpi screen, scaling the fonts is worse than flooring the size. Does that make sense?

Ah, I wasn't even aware of the StringArrayHashMap. The main reason I implemented things this way was the guarantee that iterating thru all existing themes is fast.

Took me a while to learn about ArrayHashMap as well. It should be plenty fast. I think we can sort StringArrayHashMap?

[VisenDev]

Okay, I believe those issues have been addressed

[david-vanderson]

With the alloc issue, I see what you were thinking, but the problem isn't creating the underlying ArenaAllocator, it's doing self.arena.allocator() which gives you an Allocator struct with an internal pointer that points inside the self object which could be allocated on the stack. Then that Allocator struct is passed into HashMap.init(), so the hashmap is holding on to it. Let me know if that still doesn't make sense.

I'll take it from here - thanks!

[VisenDev]

With the alloc issue, I see what you were thinking, but the problem isn't creating the underlying ArenaAllocator, it's doing self.arena.allocator() which gives you an Allocator struct with an internal pointer that points inside the self object which could be allocated on the stack. Then that Allocator struct is passed into HashMap.init(), so the hashmap is holding on to it. Let me know if that still doesn't make sense.

I know the problem isn't creating the underlying arena. However, storing a pointer to an arena allocator stored on the heap should fix this no?

        var self: @This() = .{
            .arena = try base_allocator.create(std.heap.ArenaAllocator),
            .themes = undefined,
        };
        self.arena.* = std.heap.ArenaAllocator.init(base_allocator);

Thus, the allocator created in this line

        const alloc = self.arena.allocator();

Is getting a pointer to heap memory, not a stack pointer, no? The arena is never stored on the stack - so any pointers to the arena must be pointing to the heap memory

For example, isn't this the same way std.json handles things?
https://ziglang.org/documentation/master/std/#std.json.static.parseFromValue

[david-vanderson]

Whoops - you are totally right I missed that. Sorry!

[iacore]

Question: how much more than QuickTheme do users need? Currently, every widget gets their own style (depending on what they think is heading/body text).

A markdown rendering widget would use a lot of different fonts, but is it worth it to have the global scope be complicated?

Now I see four types of text in QuickTheme:

• body
• heading
• caption
• title

What tells me that a piece of text is title or heading? What's caption? These need to be documented, somehow.

Maybe here is where we use Zig type magic to generate theme based on widgets used. Every widget can request a set of style variables to be filled in.

Let's say we have a struct StyleDesigner.

const style: StyleDesigner = dvui.core_style_designer;
dvui.style = style;

// later, in a widget
const font = dvui.style.give_me("body text", .type_face);
const color = dvui.style.give_me("body text", .color);

The key point is that the extensible style is a Zig function, not encoded in JSON. CSS encodes rules like this. I think we should have core widgets settle on a set of constants like "body text", and then design the function. For customization, we can then have a CSS-like language in JSON, or kdl, or CSS itself. If one likes, they can write the function themselves, not relying on rules specified in JSON/kdl/CSS.

This new StyleDesigner thing should also take care of font loading, since it's convenient. I remember finding fonts to be discussed somewhere. Maybe here is where we implement it. I know how to do that on Linux (with fc-list).

[iacore]

I have created the styler branch to let you get a feel of the new design.

[VisenDev]

Question: how much more than QuickTheme do users need? Currently, every widget gets their own style (depending on what they think is heading/body text).

I'm not entirely sure how this idea would operate in practice. I did look over your branch. However, I do like the idea of using a subset of CSS directly rather than making our own bespoke styling system.

[david-vanderson]

I'm not sure I follow you. QuickTheme is just a way to serialize a Theme - it doesn't interact with widgets directly.

const font = dvui.style.give_me("body text", .type_face);

is currently written as

const font = dvui.themeGet().font_body;
// or when making a widget
dvui.myWidget(@src(), ..., .{ .font_style = .body });

I agree that some kind of documentation would help, but unsure how to do it. Maybe there's a way to enhance the debugging window to give information about the font under the mouse?

[david-vanderson]

Now that I've written that, I'm tempted to get rid of the .font_style part of options in favor of just:

dvui.myWidget(@src(), ..., .{ .font = dvui.themeGet().font_body });

The idea behind font_style was to be able to specify an Options struct in one place (especially the defaults of widgets) but still allow the Theme to change. But it seems that widgets never have a different font in their defaults - it's always font_body.

[iacore]

I'm not sure I follow you. QuickTheme is just a way to serialize a Theme - it doesn't interact with widgets directly.

It's not about QuickTheme, but my own idea of what a style API would be.