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

Add support for fractional font sizes. #87243

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Jan 16, 2024

Changes font_size and outline_size (theme constants) from int to float for better font scaling control.

@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 16, 2024

We can't change theme constants to floats, it breaks compatibility and hinders their usability as booleans.

There is already a PR by @Calinou for that, but we haven't merged it for those reasons.


If we are going to break compatibility, we need to implement it in a more useful way and extend constants to support a variety of types (but not necessarily the full spectrum of Variant types). But that's a lot of work and the API is not clear.

@bruvzg
Copy link
Member Author

bruvzg commented Jan 16, 2024

We can't change theme constants to floats, it breaks compatibility

What exactly is it breaking? I do not see anything this can break.

hinders their usability as booleans.

bools are just 0 / 1 and it's representable by float, not sure what's the issue either.

If we are going to break compatibility, we need to implement it in a more useful way and extend constants to support a variety of types (but not necessarily the full spectrum of Variant types). But that's a lot of work and the API is not clear.

This is definitely a useful thing to consider.

@YuriSizov
Copy link
Contributor

YuriSizov commented Jan 16, 2024

I wanted to look up the aforementioned PR, and I did, #39013, but apparently there was some discussion elsewhere because it doesn't contain more examples.

So I'll reiterate what I mentioned in chat for posterity:

We don't have any control over what users may store in those properties and where they would get the values. So yes, if you control everything, you can assume it's going to be fine. But this creates a disconnect between how it works on the implementation side (either in engine or in user code) and how it's expected to work from the configuration side (mainly in user code).

To match the expectations you have to change the code. I think it is compatibility breaking when you cannot be sure that a value that looks like a 0 (and is treated as false) is in fact a 0. You would need to use is_zero_approx/is_equal_approx or other similar means to ensure that constants which need to be a boolean are indeed what they seem to be. That is if you want to be safe with possibly arbitrary values coming from untrusted or unreliable sources your way.

And that's besides compatibility being broken in the literal way, which is why you add BC methods in this PR.


That said, pardon my initially dry response. I do support the idea, and would love to have ints, floats, and proper bools as constants at the very least, maybe even more types. I would avoid going full Variant though, as we don't want to open Themes to be a storage utility for something which is not needed by them. At least not in arbitrary constants (otherwise people can start using them instead of better typed color and texture values, leading to a mess).

@bruvzg bruvzg force-pushed the float_font_size branch 2 times, most recently from 6a5be8f to c550a76 Compare January 16, 2024 18:12
@bruvzg
Copy link
Member Author

bruvzg commented Jan 16, 2024

As a POC, switched theme constants to Variant with type annotation (it the constant declarations), and validation (on set/override, warning if set to wrong type value, error if value is not convertible to correct type).

Should also use correct property editor fields:
Screenshot 2024-01-16 at 20 11 08
Screenshot 2024-01-16 at 20 11 33

@AdriaandeJongh
Copy link
Contributor

As my proposal here is strongly related to this PR, could / should glyph spacing and space glyph spacing also be included in this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make theme constant and font size use float instead of int Support for theme item booleans
3 participants