Use the same default style for both builders.#368
Merged
xStrom merged 1 commit intolinebender:mainfrom Jul 7, 2025
Merged
Conversation
DJMcNab
approved these changes
Jun 9, 2025
Member
DJMcNab
left a comment
There was a problem hiding this comment.
Approving if and only if we still get rendering if you don't explicitly set a font size. Having a silent non-rendering due to the default font size being 0 would be an extremely poor UX (I could also forsee it leading to other fun bugs, such as with the overlapping line heights, although maybe we already addressed that).
Member
Author
|
There is no API surface that actually uses the 0 font size. It's only being used internally for clearing memory. 👍 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The two builders (ranged/tree) use different sources of truth to achieve a default style. This is already a bad idea, as evidenced by them having a different default line height until very recently. It gets even worse, because the ranged builder uses
ResolvedStyle::default()which means it has a dummy font and doesn't respect scale.This PR makes both builders use a single source of truth - a
TextStylewhich then gets resolved.This means that we can confidently change our default style in only one place - the
TextStyle::defaultmethod. Those default values then also get properly resolved according to available fonts, scale, etc.Note that I didn't change the public API on purpose. I do think that being able to immediately provide a custom root style to the ranged builder, like it is possible with the tree builder, is probably worth doing. However, I want to do any potential public API changes as a focused follow-up PR.
The
PlainEditor::ime_cursor_areamethod needed a slight modification as well. It was depending onResolvedStyle::default()for a font size based calculation. That did not respect scale, so I fixed it by just keeping track of the font size that was provided to thePlainEditorconstructor.I replaced the custom
ResolvedStyle::default()implementation with a simple derive. The only remaining non-derivable part was setting the font size to 16. Given that it doesn't respect scale, that custom implementation just provides a false sense of utility. A zero font size is perfectly fine for a state clearing default, which is how we're still using it.I also added a test to ensure that the default styles remain in sync between the different builders.