Skip to content

refactor(fontweight): allow unknown fontweight value #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 9, 2018

Conversation

kwonoj
Copy link

@kwonoj kwonoj commented Jul 2, 2018

Related with servo/servo#21076.

Given reproducible code snippet

extern crate dwrote;
use dwrote::{FontCollection};

fn main() {
    let system_fc = FontCollection::system();

    if let Some(family) = system_fc.get_font_family_by_name("Microsoft YaHei") {
        let count = family.get_font_count();
        println!("fount count: {}", count);

        for i in 0..count {
            let font = family.get_font(i);
            {
                let descriptor = font.to_descriptor();

                let font = FontCollection::system().get_font_from_descriptor(&descriptor).unwrap();
                println!("reading [{}] {}: {}", i, font.family_name(), font.face_name());

                let style = font.style();
                let weight = font.weight();
                let stretch = font.stretch();
                if font.face_name().starts_with("Light") {
                    println!("\t read style: {:?}, stretch: {:?}", style, stretch);
                    //println!("{:?}", weight); //This will blow up
                } else {
                    println!("\t read style: {:?}, weight: {:?}, stretch: {:?}", style, weight, stretch);
                }

            }
        }
    }
}

When try to read weight from Microsoft Yahei, only on Light weight will throw exception. surprisingly while DWRITE_FONT_WEIGHT enum doesn't specify it in documented list Yahei's light fontweight is not 300:

image

but having adjusted value of 290 and enum conversion fails since there isn't matching value.

This PR refactors FontWeight enum to not use discriminator values, and add FontWeight::Unknown(u32) type to allow fallback if C enum contains non-matching values to enum values. There are couple of way I tried, but wasn't entirely sure around each approaches - except require manual matching from u32 value to enum and vice versa, this approach provides simplest way to resolve issue.

  1. DWRITE_FONT_WEIGHT enum doesn't have huge values, manual matching doesn't require lot of boilerplate
  2. Potentially those enum value won't change quite frequently, also Unknown(u32) will provide fallback even if it occurs

Still, technically this can be breaking change if any consumer uses direct casting to u32 of enum values instead of using to_u32() interface: but so far I know there doesn't seem way to create non-breaking change if we'd like to have fallback values.

Note: this doesn't resolve servo side crash immediately - servo need to implement to handle fallback values.

@jdm
Copy link
Member

jdm commented Jul 2, 2018

https://searchfox.org/mozilla-central/source/gfx/thebes/gfxDWriteFontList.h#98-133 may be the code that Firefox uses to deal with this.

@kwonoj
Copy link
Author

kwonoj commented Jul 2, 2018

@jdm, yes, looks somewhat related. For me legit way looks like

  1. dwrote forward any unknown value as-is, i.e enum FontWeight { ... UNKNOWN(u32) } along with any documented value
  2. servo takes responsibility of matching values as similar to firefox does for u32 value of fontweight

I tried 1 quickly but rust doesn't offer out of box way to mix with c-style enum for those with discriminator values can only be used with a c-like enum. If those sound right approach, I'll try dig bit more. (or it'd appreciate to suggest other way around)

@jdm
Copy link
Member

jdm commented Jul 3, 2018

That does sound like the correct way to deal with it.

@kwonoj kwonoj changed the title fix(fontweight): support undocumented font weight value refactor(fontweight): allow unknown fontweight value Jul 8, 2018
@kwonoj
Copy link
Author

kwonoj commented Jul 8, 2018

/cc @jdm for visibility.

@jdm
Copy link
Member

jdm commented Jul 9, 2018

The CI problem is not caused by this PR; I've opened #10 for that.

@jdm jdm merged commit 28e6f7e into servo:master Jul 9, 2018
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.

2 participants