Use more objc2 framework crates
#74
Conversation
Use objc2-core-foundation, objc2-core-text and objc2-core-graphics.
| ct_descriptor: desc, | ||
| } | ||
| fn new(desc: &CTFontDescriptor) -> Descriptor { | ||
| let style_name = unsafe { desc.attribute(kCTFontStyleNameAttribute) } |
There was a problem hiding this comment.
This is a rather unusual unsafe usage, I think I'd prefer it being assigned to its own variable over chaining off its scope block.
Also what exactly makes this unsafe? I'd assume it's due to the parameter potentially not existing? Might be a good idea to have a safety comment on it.
| let name = CFString::from_static_str("Apple Symbols"); | ||
| let apple_symbols = unsafe { CTFont::with_name(&name, size, ptr::null_mut()) }; | ||
| fallbacks.push(Font { ct_font: apple_symbols, fallbacks: Vec::new() }); |
There was a problem hiding this comment.
What if the font doesn't exist? It looks like previously we were handling failure here.
| let list = ct_cascade_list_for_languages(ct_font, &langarr); | ||
| let list = | ||
| unsafe { ct_font.default_cascade_list_for_languages(Some(langarr.as_opaque())) }.unwrap(); | ||
| // CFArray of CTFontDescriptorRef. |
There was a problem hiding this comment.
Not really necessary it's already obvious in the signature.
| let Some(attr_val) = (unsafe { fontdesc.attribute(kCTFontEnabledAttribute) }) else { | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
No let else. Please use a match statement.
| let Ok(attr_val) = attr_val.downcast::<CFNumber>() else { | ||
| return false; | ||
| }; |
There was a problem hiding this comment.
Again, please don't use let else.
| if let Some(descriptors) = descriptors { | ||
| for descriptor in descriptors.iter() { | ||
| out.push(Descriptor::new(descriptor.clone())); | ||
| // CFArray of CTFontDescriptorRef (i think). |
There was a problem hiding this comment.
| // CFArray of CTFontDescriptorRef (i think). |
Was never really a good comment. I think? Really? 😄
| } | ||
| } | ||
|
|
||
| out | ||
| } | ||
|
|
||
| pub fn create_for_family(family: &str) -> Option<CFRetained<CTFontCollection>> { |
There was a problem hiding this comment.
A doc comment would go a long way on this function.
| let glyphs = [glyph_index as CGGlyph]; | ||
| let bounds = unsafe { | ||
| self.ct_font.bounding_rects_for_glyphs( | ||
| CTFontOrientation::Default, | ||
| NonNull::from(&glyphs[0]), | ||
| ptr::null_mut(), | ||
| 1, | ||
| ) | ||
| }; |
There was a problem hiding this comment.
Sorry but what's the point of creating an array just to immediately go and only retrieve its first element?
| NonNull::from(&[glyph_index as CGGlyph][0]), | ||
| NonNull::from(&[rasterization_origin][0]), |
There was a problem hiding this comment.
Again, why are we creating arrays just to immediately access their only element?
| .field( | ||
| "render_mode", | ||
| &match self.render_mode { | ||
| freetype::RenderMode::Normal => "Normal", | ||
| freetype::RenderMode::Light => "Light", | ||
| freetype::RenderMode::Mono => "Mono", | ||
| freetype::RenderMode::Lcd => "Lcd", | ||
| freetype::RenderMode::LcdV => "LcdV", | ||
| freetype::RenderMode::Max => "Max", | ||
| freetype::RenderMode::Sdf => "Sdf", | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Did cargo fmt change this for you? It's certainly not an improvement and I'm not sure why it would have changed.
Remove a few unnecessary dependencies, update
objc2, and use more of the framework crates from that.Motivation: The
core-foundation,core-textandcore-graphicscrates are being deprecated, see servo/core-foundation-rs#729.It'd be nice if we could get a release after this.