Skip to content

Conversation

@acarl005
Copy link

Add more fallible getter methods a. la #62.

self.font_family_by_name(family_name).unwrap()
}

pub fn font_family_by_name(&self, family_name: &str) -> Result<Option<FontFamily>, HRESULT> {
Copy link
Author

@acarl005 acarl005 Feb 25, 2025

Choose a reason for hiding this comment

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

Note that we have a Result<Option<FontFamily>, HRESULT> instead of just a Result<FontFamily, HRESULT>. I think this makes sense, as there may be no font family with the given name, so Ok(None) is returned to designate that the name doesn't exist, and no errors occurred.

An alternative I considered was creating a new error enum, e.g.

enum GetError {
    Win32Error(HRESULT),
    NotFound
}

And we could return Result<FontFamily, GetError> instead.

self.font_from_descriptor(desc).unwrap()
}

// Find a font matching the given font descriptor in this font collection.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Find a font matching the given font descriptor in this font collection.
/// Find a font matching the given font descriptor in this font collection.

@mrobinson
Copy link
Member

@acarl005 I'm not able to modify your branch. Please apply the following diff and I will land this one:

diff --git a/src/font_collection.rs b/src/font_collection.rs
index 3e4974f..2154b28 100644
--- a/src/font_collection.rs
+++ b/src/font_collection.rs
@@ -114,6 +114,7 @@ impl FontCollection {
         self.font_family(index).unwrap()
     }
 
+    /// Returns the [`FontFamily`] at the given index.
     pub fn font_family(&self, index: u32) -> Result<FontFamily, HRESULT> {
         let mut family: *mut IDWriteFontFamily = ptr::null_mut();
         unsafe {
@@ -130,7 +131,7 @@ impl FontCollection {
         self.font_from_descriptor(desc).unwrap()
     }
 
-    // Find a font matching the given font descriptor in this font collection.
+    /// Find a font matching the given font descriptor in this [`FontCollection`].
     pub fn font_from_descriptor(&self, desc: &FontDescriptor) -> Result<Option<Font>, HRESULT> {
         if let Some(family) = self.font_family_by_name(&desc.family_name)? {
             let font = family.first_matching_font(desc.weight, desc.stretch, desc.style)?;
@@ -151,6 +152,7 @@ impl FontCollection {
         self.font_from_face(face).ok()
     }
 
+    /// Get a [`Font`] from the given [`FontFace`].
     pub fn font_from_face(&self, face: &FontFace) -> Result<Font, HRESULT> {
         let mut font: *mut IDWriteFont = ptr::null_mut();
         unsafe {
@@ -167,6 +169,8 @@ impl FontCollection {
         self.font_family_by_name(family_name).unwrap()
     }
 
+    /// Find a [`FontFamily`] with the given name. Returns `None` if no family
+    /// with that name is found.
     pub fn font_family_by_name(&self, family_name: &str) -> Result<Option<FontFamily>, HRESULT> {
         let mut index: u32 = 0;
         let mut exists: BOOL = FALSE;

@acarl005
Copy link
Author

@mrobinson done!

@jdm jdm added this pull request to the merge queue Mar 23, 2025
Merged via the queue into servo:main with commit 494cd98 Mar 23, 2025
2 checks 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.

3 participants