-
Notifications
You must be signed in to change notification settings - Fork 24
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
[fontique] use windows-rs for dwrite backend #85
base: main
Are you sure you want to change the base?
Conversation
Swaps out winapi, wio and dwrote for windows-core and windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have my Windows machine handy, so I can't actually run this, so just going through the patch when DirectWrite isn't my thing, so I have a couple of questions below.
The code looks better overall though!
i += 1; | ||
.is_ok() | ||
{ | ||
if let Some(font) = mapped_font { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if there isn't a mapped font? I assume falling through to None
case is fine, but the mapped_len
doesn't seem to be used, instead just skipping a single character.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In reality, we’re not making full use of this function. What we actually want is the font DirectWrite would choose for a specific Unicode script and locale. Since there’s no API for this, we cheat by using a table containing sample text for each script (see
Line 90 in caabcf1
pub const SCRIPT_SAMPLES: &[(Script, &str)] = &[ |
Ideally, we’d hand curate the sample table for comprehensive coverage, but so far it’s seemed to work on all platforms (with some tiny hacks like this one) so I haven’t bothered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a great explanation and a subset of this would be a great thing to have in a comment in this code. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point! I’ll add a comment describing this glorious hack to hopefully avoid confusion for future readers.
Swaps out winapi, wio and dwrote for windows-core and windows.