Skip to content

Conversation

acarl005
Copy link

@acarl005 acarl005 commented Feb 20, 2025

This closes #61

Some of the methods in font_family::FontFamily can cause a panic. I have added analogous try_ methods for each which will return a Result instead in order to let the caller handle the error.

The next step would be to start migrating callers to use these new methods, e.g.
https://github.com/servo/font-kit/blob/d49041ca57da4e9b412951f96f74cb34e3b6324f/src/sources/directwrite.rs#L44

.unwrap()
}

pub fn try_get_first_matching_font(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Rust idiomatically doesn't use the word get_ in method names, I wonder if a way to do this would be to make the fallible ones names like first_matching_font and then to deprecate the non-fallible versions. That way we could gradually transition to a more idiomatically-Rust-like interface. What do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me! Will update.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I've updated the names and #[deprecated] the old methods.

self.font(index).unwrap()
}

pub fn font(&self, index: u32) -> Result<Font, HRESULT> {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little unsure about the name font for this method. Any suggestions? font_from_index maybe?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

font_at_index or font_from_index both sound good to me.

Copy link
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I think this looks good. I do agree that font_at_index/font_from_index might be a better name, but this could probably land without that as well.

@mrobinson
Copy link
Member

@acarl005 Thanks for the change! Let me know if you like to rename font. If not, I'll land this.

@acarl005
Copy link
Author

@mrobinson nah, I like the name actually.

@mrobinson mrobinson added this pull request to the merge queue Feb 25, 2025
Merged via the queue into servo:main with commit dbce1fb Feb 25, 2025
1 check passed
@jdm jdm mentioned this pull request Mar 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove panicking assert

2 participants