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

Font theme rework #128

Closed
wants to merge 4 commits into from
Closed

Conversation

VisenDev
Copy link
Collaborator

Very experimental changes to separate a Font's scale from a Font's base_size

I am planning on migrating all prebuilt themes to json to make them easier to edit and easier to create new themes. This is one of several changes I am going to experiment with to make Themes easier to work with.

@david-vanderson
Copy link
Owner

Can you speak to the motivation here? What does having a base_size make possible or easier?

@VisenDev
Copy link
Collaborator Author

VisenDev commented Sep 18, 2024

Can you speak to the motivation here? What does having a base_size make possible or easier?

The main thing is that it makes it easier to adjust the base font size when adding new themes. Originally, Themes had to specify specific font sizes for each instance of a font.

    .font_body = .{ .size = 13, .name = "Vera" },
    .font_heading = .{ .size = 13, .name = "VeraBd" },
    .font_caption = .{ .size = 10, .name = "Vera" },
    .font_caption_heading = .{ .size = 10, .name = "VeraBd" },

This made things difficult when trying to add new fonts for a theme, since if the default font size was too small, you would have to manually increment each .size = parameter to make them all scale equally.

This was unnecessarily difficult, so what I ended up doing on most of my themes was creating a size variable, and then initializing each font size using that size variable multiplied by a scaling factor. This allowed me to easily adjust the size of a font.

const size = 18;

pub const jungle = Theme{
    .font_body = .{ .size = size, .name = "Pixelify" },
    .font_heading = .{ .size = size, .name = "Pixelify" },
    .font_caption = .{ .size = size, .name = "Pixelify" },
    .font_caption_heading = .{ .size = size, .name = "Pixelify" },
    .font_title = .{ .size = size * 2, .name = "Pixelify" },
    .font_title_1 = .{ .size = size * 1.8, .name = "Pixelify" },
    .font_title_2 = .{ .size = size * 1.6, .name = "Pixelify" },
    .font_title_3 = .{ .size = size * 1.4, .name = "Pixelify" },
    .font_title_4 = .{ .size = size * 1.2, .name = "Pixelify" },
    //...
 };

At this point, the size variable doesn't really have anything to do with the theme. This base font size is really a property of the Font. The Theme just wants to be able to scale whatever that base size is.

Hence, its seems more straightforward to give each font a base_size parameter, and then scale that base_size up and down as necessary

This makes implementing Themes via json more straightforward because I cannot create a size variable for scaling in json as I can in zig. This means that every .size = field would have to be manually adjusted if the font was too big or too small.

Thus, it seems like swapping out the font size parameter for a font scale parameter makes fonts easier to work when implementing themes.

Hope that helps clarify. I'm not super tied to this change, I've just had this idea floating in my head for a while and wanted to test it out. Theres a couple things in Theme that have been kind of bugging me and this is one of them.

I mentioned this here as well #81

@VisenDev
Copy link
Collaborator Author

An alternative approach would be to create a SerializableTheme struct that is specifically geared to make it easier to create new themes, and then convert that data into an actual dvui.Theme automatically

@david-vanderson
Copy link
Owner

david-vanderson commented Sep 18, 2024

Thanks for the explanation! Will think about this, it seems like the base_size is a property of the theme maybe. I certainly have wanted a way to scale all the fonts in a theme (see fontSizeAdd() which I'm not happy with) , but I can't put my finger on the right way to do it.

SerializableTheme is a good idea. Maybe that's the way to go.

@VisenDev
Copy link
Collaborator Author

I'll look into sending an alternative PR that does this instead

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