From 20f80cf660425a64882afdaff58308fdecd83f55 Mon Sep 17 00:00:00 2001 From: Shane Celis Date: Wed, 21 Feb 2024 04:29:15 -0500 Subject: [PATCH 1/3] bug: Add diagnostic information on font load error I ran into a bug a while back that prevent me from using the plotters library for months. I did finally figure out what was causing it: a file in my font library documented in issue#231. Removing the file fixed my issue, but the opaque problem is sure to ensnare others. This PR adds a `LoadError(Cow<'static, str>)` variant to `SelectionError` enum. It removes `Copy` because `Cow` isn't copyable. And now when I run the test suite with the bad file in place, it reports this error, which is far and away more helpful than before. ```sh > RUST_BACKTRACE=1 cargo test --all ... running 5 tests test loaders::core_text::test::test_core_text_to_css_font_stretch ... ok test loaders::core_text::test::test_core_text_to_css_font_weight ... ok test sources::core_text::test::test_css_to_core_text_font_stretch ... ok test sources::core_text::test::test_css_to_core_text_font_weight ... ok test loaders::core_text::test::test_from_core_graphics_font ... FAILED failures: ---- loaders::core_text::test::test_from_core_graphics_font stdout ---- thread 'loaders::core_text::test::test_from_core_graphics_font' panicked at src/loaders/core_text.rs:940:14: called `Result::unwrap()` on an `Err` value: LoadError("Parse error on path \"/Users/shane/Library/Fonts/Arial\"") stack backtrace: 0: rust_begin_unwind at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/std/src/panicking.rs:645:5 1: core::panicking::panic_fmt at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/panicking.rs:72:14 2: core::result::unwrap_failed at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/result.rs:1649:5 3: core::result::Result::unwrap at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/result.rs:1073:23 4: font_kit::loaders::core_text::test::test_from_core_graphics_font at ./src/loaders/core_text.rs:938:21 5: font_kit::loaders::core_text::test::test_from_core_graphics_font::{{closure}} at ./src/loaders/core_text.rs:937:38 6: core::ops::function::FnOnce::call_once at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/ops/function.rs:250:5 7: core::ops::function::FnOnce::call_once at /rustc/07dca489ac2d933c78d3c5158e3f43beefeb02ce/library/core/src/ops/function.rs:250:5 note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. failures: loaders::core_text::test::test_from_core_graphics_font test result: FAILED. 4 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.04s error: test failed, to rerun pass `--lib` ``` A variant that used `String` would suffice for this case. I tend to use `Cow<'static, str>` on errors since many times they can be `&'static str`. --- src/error.rs | 6 +++++- src/sources/core_text.rs | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/error.rs b/src/error.rs index 27ab805..13cc456 100644 --- a/src/error.rs +++ b/src/error.rs @@ -13,6 +13,7 @@ use std::convert::From; use std::error::Error; use std::io; +use std::borrow::Cow; macro_rules! impl_display { ($enum:ident, {$($variant:pat => $fmt_string:expr),+$(,)* }) => { @@ -91,12 +92,14 @@ impl From for GlyphLoadingError { } /// Reasons why a source might fail to look up a font or fonts. -#[derive(Clone, Copy, PartialEq, Debug)] +#[derive(Clone, PartialEq, Debug)] pub enum SelectionError { /// No font matching the given query was found. NotFound, /// The source was inaccessible because of an I/O or similar error. CannotAccessSource, + /// Could not load the font. + LoadError(Cow<'static, str>), } impl Error for SelectionError {} @@ -104,5 +107,6 @@ impl Error for SelectionError {} impl_display! { SelectionError, { NotFound => "no font found", CannotAccessSource => "failed to access source", + LoadError(cow) => &cow, } } diff --git a/src/sources/core_text.rs b/src/sources/core_text.rs index c323eb6..16f50e8 100644 --- a/src/sources/core_text.rs +++ b/src/sources/core_text.rs @@ -255,7 +255,7 @@ fn create_handle_from_descriptor(descriptor: &CTFontDescriptor) -> Result Ok(Handle::from_memory(font_data, 0)), - Err(_) => Err(SelectionError::CannotAccessSource), + Err(e) => Err(SelectionError::LoadError(format!("{:?} error on path {:?}", e, font_path).into())), } } From b28658a473acd411951981bdd99b4dbc9435c9dd Mon Sep 17 00:00:00 2001 From: Shane Celis Date: Thu, 22 Feb 2024 01:23:22 -0500 Subject: [PATCH 2/3] refactor: Prefer CannotAccessSource to new variant --- src/error.rs | 10 +++++----- src/sources/core_text.rs | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/error.rs b/src/error.rs index 13cc456..b0323ef 100644 --- a/src/error.rs +++ b/src/error.rs @@ -97,16 +97,16 @@ pub enum SelectionError { /// No font matching the given query was found. NotFound, /// The source was inaccessible because of an I/O or similar error. - CannotAccessSource, - /// Could not load the font. - LoadError(Cow<'static, str>), + CannotAccessSource { + /// Additional diagnostic information may include file name + reason: Option> + }, } impl Error for SelectionError {} impl_display! { SelectionError, { NotFound => "no font found", - CannotAccessSource => "failed to access source", - LoadError(cow) => &cow, + CannotAccessSource { reason: ref maybe_cow } => maybe_cow.as_deref().unwrap_or("failed to access source") } } diff --git a/src/sources/core_text.rs b/src/sources/core_text.rs index 16f50e8..455571a 100644 --- a/src/sources/core_text.rs +++ b/src/sources/core_text.rs @@ -230,13 +230,13 @@ fn create_handle_from_descriptor(descriptor: &CTFontDescriptor) -> Result Result Ok(Handle::from_memory(font_data, 0)), - Err(e) => Err(SelectionError::LoadError(format!("{:?} error on path {:?}", e, font_path).into())), + Err(e) => Err(SelectionError::CannotAccessSource { reason: Some(format!("{:?} error on path {:?}", e, font_path).into()) }), } } From ba04613170aba933e775744717f518394796b6cc Mon Sep 17 00:00:00 2001 From: Shane Celis Date: Fri, 8 Mar 2024 13:16:20 -0500 Subject: [PATCH 3/3] chore: Run cargo fmt. --- src/error.rs | 4 ++-- src/sources/core_text.rs | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/error.rs b/src/error.rs index b0323ef..4429dd4 100644 --- a/src/error.rs +++ b/src/error.rs @@ -10,10 +10,10 @@ //! Various types of errors that `font-kit` can return. +use std::borrow::Cow; use std::convert::From; use std::error::Error; use std::io; -use std::borrow::Cow; macro_rules! impl_display { ($enum:ident, {$($variant:pat => $fmt_string:expr),+$(,)* }) => { @@ -99,7 +99,7 @@ pub enum SelectionError { /// The source was inaccessible because of an I/O or similar error. CannotAccessSource { /// Additional diagnostic information may include file name - reason: Option> + reason: Option>, }, } diff --git a/src/sources/core_text.rs b/src/sources/core_text.rs index 455571a..e4b6292 100644 --- a/src/sources/core_text.rs +++ b/src/sources/core_text.rs @@ -255,7 +255,9 @@ fn create_handle_from_descriptor(descriptor: &CTFontDescriptor) -> Result Ok(Handle::from_memory(font_data, 0)), - Err(e) => Err(SelectionError::CannotAccessSource { reason: Some(format!("{:?} error on path {:?}", e, font_path).into()) }), + Err(e) => Err(SelectionError::CannotAccessSource { + reason: Some(format!("{:?} error on path {:?}", e, font_path).into()), + }), } }