servo / font-kit

A cross-platform font loading library written in Rust
Apache License 2.0
660 stars 98 forks source link

bug: Add diagnostic information on font load error #235

Closed shanecelis closed 4 months ago

shanecelis commented 5 months ago

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.

> 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<T,E>::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.

jdm commented 4 months ago

Can you run cargo fmt on these changes as well?

shanecelis commented 4 months ago

No problem.