fontique: Update to the new objc2 CoreFoundation/Text bindings
#252
Conversation
|
@madsmtm This may be of interest to you as well. |
fontique/src/backend/coretext.rs
Outdated
| }; | ||
| let locale = locale.map(CFString::from_str); | ||
| let base_font = if prefer_ui { | ||
| unsafe { CTFontCreateUIFontForLanguage(CTFontUIFontType::System, 0.0, None).unwrap() } |
There was a problem hiding this comment.
I assume this can't fail. I could be wrong?
There was a problem hiding this comment.
I mean, since we are giving no language then presumably it is never going to fail in practice... though it is technically fallible.
There was a problem hiding this comment.
I believe you always get the last resort font when matching fails but I try to avoid unwraps in cases where I can’t be 100% certain. My general philosophy is that libraries should never panic, particularly in cases like this where the user has no control over the behavior.
There was a problem hiding this comment.
I will adjust this to remove the unwrap.
fontique/src/backend/coretext.rs
Outdated
| unsafe { | ||
| let attrs = CFDictionaryCreate(None, null_mut(), null_mut(), 0, null(), null()); | ||
| let desc = CTFontDescriptorCreateWithAttributes(&attrs.unwrap()); | ||
| CTFontCreateWithFontDescriptor(&desc, 0.0, null()) |
There was a problem hiding this comment.
It is interesting that we don't pass any attributes here. What is the font that gets returned from this sort of thing?
There was a problem hiding this comment.
I believe in practice it gets you 12pt San Francisco (or Helvetica back-when). CTFontCreateWithFontDescriptor takes an unspecified descriptor as an opportunity to default on every field. It defaults to an ordinary text font.
fontique/Cargo.toml
Outdated
| objc2-foundation = { version = "0.2.2", features = ["NSArray", "NSEnumerator", "NSPathUtilities", "NSString"], optional = true } | ||
| objc2-foundation = { version = "0.3.0", optional = true, default-features = false, features = ["alloc", "NSArray", "NSEnumerator", "NSPathUtilities", "NSString"] } | ||
| objc2-core-foundation = { version = "0.3.0", optional = true, default-features = false, features = ["CFBase"] } | ||
| objc2-core-text = { version = "0.3.0", optional = true } |
There was a problem hiding this comment.
I still need to turn off default-features here and then update and re-format the above.
waywardmonkeys
force-pushed
the
update-objc2
branch
from
18383c4 to
52b4233
Compare
There was a problem hiding this comment.
I cannot say anything about the specifics of the API (I kinda just do the bindings), but otherwise it looks good 👍.
Btw, I hope you didn't decide to stray away from using CFDictionary for the attributes just because we don't provide a nice binding for creating those yet? If so, that's definitely unfortunate, then I'd rather that you wait with using objc2-core-graphics until we provide feature-parity (I don't want people to use the crates when their usage ends up less flexible ;) ).
I wanted your input on if I was using them correctly and maybe whether or not there is something that could be better (now or in the future).
Nope! We weren't passing in any actual attributes in the old code either, so there's no functional change there really. |
|
I can't meaningfully review this code. I'm happy to formalise the approval from Mads, if needed. |
waywardmonkeys
force-pushed
the
update-objc2
branch
from
52b4233 to
73385df
Compare
waywardmonkeys
changed the title
objc2 0.6 and new CoreFoundation bindingsobjc2 CoreFoundation/Text bindings
waywardmonkeys
force-pushed
the
update-objc2
branch
from
73385df to
d4bb51b
Compare
|
Updated to remove the unwrap. Moved some stuff back to the top level to clean it up. The diff is terrible to read, sorry. |
| let text = CFString::from_str(text); | ||
| let text_range = CFRange { | ||
| location: 0, | ||
| length: unsafe { CFStringGetLength(&text) }, |
There was a problem hiding this comment.
Out of interest, is this the same as (before shadowing) text.chars().count()?
There was a problem hiding this comment.
I honesty don't know.
There was a problem hiding this comment.
I think it would be: text.chars().map(char::len_utf16).sum() (docs)
The `objc2` family of crates now provides bindings for CoreFoundation and CoreText (and much more), so we can use these rather than the old `core-foundation` and `core-text` crates.
waywardmonkeys
force-pushed
the
update-objc2
branch
from
d4bb51b to
73d6b24
Compare
The
objc2family of crates now provides bindings for CoreFoundation and CoreText (and much more), so we can use these rather than the oldcore-foundationandcore-textcrates.