fontique: Update to objc2 0.6 and new CoreFoundation bindings
#252
Conversation
|
@madsmtm This may be of interest to you as well. |
| { | ||
| desc = ui_desc; | ||
| } | ||
| } |
There was a problem hiding this comment.
I didn't think we needed to get the UI front, then get the descriptor from that, then get a font from that descriptor. If we want a UI font, can't we just use the UI font that get from CTFontCreateUIFontForLanguage?
There was a problem hiding this comment.
That seems plausible, not sure why it's done that way right now.
There was a problem hiding this comment.
At some point, this was more involved and tried to select a font with specific properties thus a descriptor was required. The simplification looks good to me.
| }; | ||
| 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.
| 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.
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. This also simplifies some of the code. One simplification assumes that `CTFontCreateUIFontForLanguage` won't fail.
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. |
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.This also simplifies some of the code. One simplification assumes that
CTFontCreateUIFontForLanguagewon't fail.