Merge pull request #15 from felipesanches/issue_14

Split profiles Universal and OpenType

Author: simoncozens

Cargo.toml

@@ -5,6 +5,7 @@ members = [
     "fontspector-cli",
     "fontspector-checkapi",
     "fontspector-checkhelper",
+    "profile-opentype",
     "profile-universal",
     "profile-testplugin",
     "profile-googlefonts",

fontspector-cli/Cargo.toml

@@ -12,6 +12,7 @@ fontspector-checkapi = { path = "../fontspector-checkapi" }
 # These profiles are baked-in
 fontbakery-bridge = { path = "../fontbakery-bridge" }
 profile-universal = { path = "../profile-universal" }
+profile-opentype = { path = "../profile-opentype" }
 profile-googlefonts = { path = "../profile-googlefonts" }
 pluginator = { workspace = true }
 clap = { version = "3.2.5", features = ["derive"] }

profile-opentype/Cargo.toml

@@ -0,0 +1,18 @@
+[package]
+name = "profile-opentype"
+version = "0.1.0"
+edition = "2021"
+authors =  [
+    "Simon Cozens <[email protected]>",
+    "Felipe Sanches <[email protected]>",
+]
+
+[dependencies]
+fontspector-checkapi = { path = "../fontspector-checkapi" }
+fontspector-checkhelper = { workspace = true }
+read-fonts = {workspace = true}
+write-fonts = {workspace = true}
+font-types = {workspace = true}
+skrifa = {workspace = true}
+itertools = "0.13.0"
+unicode-properties = "0.1.3"

profile-universal/src/checks/bold_italic_unique.rs profile-opentype/src/checks/bold_italic_unique.rs

@@ -114,35 +114,6 @@ fn mac_style(f: &Testable, _context: &Context) -> CheckFnResult {
     return_result(problems)
 }
 
-#[check(
-    id = "opentype/family/equal_font_versions",
-    title = "Make sure all font files have the same version value.",
-    rationale = "Within a family released at the same time, all members of the family should have the same version number in the head table.",
-    proposal = "legacy:check/014",
-    implementation = "all"
-)]
-fn equal_font_versions(c: &TestableCollection, context: &Context) -> CheckFnResult {
-    let fonts = TTF.from_collection(c);
-    let versions_names: Result<Vec<_>, ReadError> = fonts
-        .iter()
-        .map(|f| {
-            f.font().head().map(|head| {
-                (
-                    head.font_revision(),
-                    format!("{:.03}", head.font_revision().to_f32()),
-                    f.filename.to_string_lossy(),
-                )
-            })
-        })
-        .collect();
-    assert_all_the_same(
-        context,
-        &versions_names?,
-        "mismatch",
-        "Version info differs among font files of the same font project.",
-    )
-}
-
 #[check(
     id = "opentype/unitsperem",
     proposal = "legacy:check/043",
@@ -180,3 +151,32 @@ fn unitsperem(f: &Testable, _context: &Context) -> CheckFnResult {
         )),
     }
 }
+
+#[check(
+    id = "opentype/family/equal_font_versions",
+    title = "Make sure all font files have the same version value.",
+    rationale = "Within a family released at the same time, all members of the family should have the same version number in the head table.",
+    proposal = "legacy:check/014",
+    implementation = "all"
+)]
+fn equal_font_versions(c: &TestableCollection, context: &Context) -> CheckFnResult {
+    let fonts = TTF.from_collection(c);
+    let versions_names: Result<Vec<_>, ReadError> = fonts
+        .iter()
+        .map(|f| {
+            f.font().head().map(|head| {
+                (
+                    head.font_revision(),
+                    format!("{:.03}", head.font_revision().to_f32()),
+                    f.filename.to_string_lossy(),
+                )
+            })
+        })
+        .collect();
+    assert_all_the_same(
+        context,
+        &versions_names?,
+        "mismatch",
+        "Version info differs among font files of the same font project.",
+    )
+}

profile-universal/src/checks/code_pages.rs profile-opentype/src/checks/code_pages.rs

@@ -0,0 +1,80 @@
+use fontspector_checkapi::{prelude::*, testfont, FileTypeConvert};
+use read_fonts::TableProvider;
+
+#[check(
+    id = "opentype/maxadvancewidth",
+    title = "MaxAdvanceWidth is consistent with values in the Hmtx and Hhea tables?",
+    rationale = "The 'hhea' table contains a field which specifies the maximum advance width. This value should be consistent with the maximum advance width of all glyphs specified in the 'hmtx' table.",
+    proposal = "legacy:check/073"
+)]
+fn maxadvancewidth(t: &Testable, _context: &Context) -> CheckFnResult {
+    let f = testfont!(t);
+    let hhea_advance_width_max = f.font().hhea()?.advance_width_max().to_u16();
+    let hmtx_advance_width_max = f
+        .font()
+        .hmtx()?
+        .h_metrics()
+        .iter()
+        .map(|m| m.advance.get())
+        .max()
+        .unwrap_or_default();
+    Ok(if hmtx_advance_width_max != hhea_advance_width_max {
+        Status::just_one_fail(
+            "mismatch",
+            &format!(
+                "AdvanceWidthMax mismatch: expected {} from hmtx; got {} for hhea",
+                hmtx_advance_width_max, hhea_advance_width_max
+            ),
+        )
+    } else {
+        Status::just_one_pass()
+    })
+}
+
+#[check(
+    id = "opentype/caret_slope",
+    title = "Check hhea.caretSlopeRise and hhea.caretSlopeRun",
+    proposal = "https://github.com/fonttools/fontbakery/issues/3670",
+    rationale = r#"
+        Checks whether hhea.caretSlopeRise and hhea.caretSlopeRun
+        match with post.italicAngle.
+
+        For Upright fonts, you can set hhea.caretSlopeRise to 1
+        and hhea.caretSlopeRun to 0.
+
+        For Italic fonts, you can set hhea.caretSlopeRise to head.unitsPerEm
+        and calculate hhea.caretSlopeRun like this:
+        round(math.tan(
+          math.radians(-1 * font["post"].italicAngle)) * font["head"].unitsPerEm)
+
+        This check allows for a 0.1° rounding difference between the Italic angle
+        as calculated by the caret slope and post.italicAngle
+    "#
+)]
+fn caret_slope(t: &Testable, _context: &Context) -> CheckFnResult {
+    let f = testfont!(t);
+    let post_italic_angle = f.font().post()?.italic_angle().to_f32();
+    let upem = f.font().head()?.units_per_em();
+    let run = f.font().hhea()?.caret_slope_run();
+    let rise = f.font().hhea()?.caret_slope_rise();
+    if rise == 0 {
+        return Ok(Status::just_one_fail(
+            "zero-rise",
+            "caretSlopeRise must not be zero. Set it to 1 for upright fonts.",
+        ));
+    }
+    let hhea_angle = (-run as f32 / rise as f32).atan().to_degrees();
+    let expected_run = (-post_italic_angle.to_radians().tan() * upem as f32).round() as i16;
+    let expected_rise = if expected_run == 0 { 1 } else { upem };
+    if (post_italic_angle - hhea_angle).abs() > 0.1 {
+        return Ok(Status::just_one_warn(
+            "mismatch",
+            &format!(
+                "hhea.caretSlopeRise and hhea.caretSlopeRun do not match with post.italicAngle.
+                Got caretSlopeRise: {}, caretSlopeRun: {}, expected caretSlopeRise: {}, caretSlopeRun: {}",
+                rise, run, expected_rise, expected_run
+            ),
+        ));
+    }
+    Ok(Status::just_one_pass())
+}

profile-universal/src/checks/fvar.rs profile-opentype/src/checks/fvar.rs

@@ -0,0 +1,13 @@
+pub mod fvar;
+pub mod bold_italic_unique;
+pub mod code_pages;
+pub mod gdef;
+pub mod glyf;
+pub mod head;
+pub mod hhea;
+pub mod kern;
+pub mod layout;
+pub mod name;
+pub mod os2;
+pub mod post;
+pub mod stat;

profile-universal/src/checks/gdef.rs profile-opentype/src/checks/gdef.rs

@@ -0,0 +1,41 @@
+use fontspector_checkapi::{prelude::*, testfont, FileTypeConvert};
+use read_fonts::TableProvider;
+
+#[check(
+    id = "name/trailing_spaces",
+    title = "Name table records must not have trailing spaces.",
+    rationale = "This check ensures that no entries in the name table end in spaces;
+                trailing spaces, particularly in font names, can be confusing to users.
+                In most cases this can be fixed by removing trailing spaces from the
+                metadata fields in the font editor.",
+    proposal = "https://github.com/googlefonts/fontbakery/issues/2417",
+    hotfix = fix_trailing_spaces
+)]
+fn name_trailing_spaces(f: &Testable, _context: &Context) -> CheckFnResult {
+    let mut problems: Vec<Status> = vec![];
+
+    if let Ok(name_table) = testfont!(f).font().name() {
+        for name_record in name_table.name_record().iter() {
+            if name_record
+                .string(name_table.string_data())
+                .map(|s| s.to_string())
+                .map(|s| s.trim_end() != s)
+                .unwrap_or(false)
+            {
+                problems.push(Status::fail("trailing-space",&format!(
+                    "Name table record {:}/{:}/{:}/{:} has trailing spaces that must be removed:\n`{:}`",
+                    name_record.platform_id,
+                    name_record.encoding_id,
+                    name_record.language_id,
+                    name_record.name_id,
+                    name_record.string(name_table.string_data()).map_err(|_| CheckError::Error("Error reading string".to_string()))?,
+                )))
+            }
+        }
+    }
+    return_result(problems)
+}
+
+fn fix_trailing_spaces(_f: &Testable) -> FixFnResult {
+    Ok(false)
+}

profile-universal/src/checks/glyf.rs profile-opentype/src/checks/glyf.rs

@@ -258,5 +258,5 @@ fn xavgcharwidth(f: &Testable, _context: &Context) -> CheckFnResult {
                 actual, expected, rule
             )
         )
-    }    )
+    })
 }

profile-universal/src/checks/head.rs profile-opentype/src/checks/head.rs

@@ -0,0 +1,120 @@
+use fontspector_checkapi::{prelude::*, testfont, FileTypeConvert};
+
+const OPTIONAL_TABLE_TAGS: [&[u8; 4]; 20] = [
+    b"cvt ", b"fpgm", b"loca", b"prep", b"VORG", b"EBDT", b"EBLC", b"EBSC", b"BASE", b"GPOS",
+    b"GSUB", b"JSTF", b"gasp", b"hdmx", b"LTSH", b"PCLT", b"VDMX", b"vhea", b"vmtx", b"kern",
+];
+
+#[check(
+    id = "required_tables",
+    title = "Font contains all required tables?",
+    rationale = "
+        According to the OpenType spec
+        https://docs.microsoft.com/en-us/typography/opentype/spec/otff#required-tables
+
+        Whether TrueType or CFF outlines are used in an OpenType font, the following
+        tables are required for the font to function correctly:
+
+        - cmap (Character to glyph mapping)⏎
+        - head (Font header)⏎
+        - hhea (Horizontal header)⏎
+        - hmtx (Horizontal metrics)⏎
+        - maxp (Maximum profile)⏎
+        - name (Naming table)⏎
+        - OS/2 (OS/2 and Windows specific metrics)⏎
+        - post (PostScript information)
+
+        The spec also documents that variable fonts require the following table:
+
+        - STAT (Style attributes)
+
+        Depending on the typeface and coverage of a font, certain tables are
+        recommended for optimum quality.
+
+        For example:⏎
+        - the performance of a non-linear font is improved if the VDMX, LTSH,
+          and hdmx tables are present.⏎
+        - Non-monospaced Latin fonts should have a kern table.⏎
+        - A gasp table is necessary if a designer wants to influence the sizes
+          at which grayscaling is used under Windows. Etc.
+    ",
+    proposal = "legacy:check/052"
+)]
+fn required_tables(t: &Testable, _context: &Context) -> CheckFnResult {
+    let f = testfont!(t);
+    let mut required_table_tags: Vec<_> = vec![
+        b"cmap", b"head", b"hhea", b"hmtx", b"maxp", b"name", b"OS/2", b"post",
+    ];
+
+    if f.is_variable_font() {
+        // According to https://github.com/fonttools/fontbakery/issues/1671
+        // STAT table is required on WebKit on MacOS 10.12 for variable fonts.
+        required_table_tags.push(b"STAT");
+    }
+
+    // See https://github.com/fonttools/fontbakery/issues/617
+    //
+    // We should collect the rationale behind the need for each of the
+    // required tables above. Perhaps split it into individual checks
+    // with the correspondent rationales for each subset of required tables.
+    //
+    // opentype/kern_table is a good example of a separate
+    // check for a specific table providing a detailed description of
+    // the rationale behind it.
+
+    let mut problems: Vec<Status> = vec![];
+    let mut optional: Vec<String> = vec![];
+
+    for tag in OPTIONAL_TABLE_TAGS {
+        if f.has_table(tag) {
+            optional.push(String::from_utf8(tag.to_vec()).map_err(|_| {
+                CheckError::Error(format!("Font tag '{:?}' wasn't UTF8?", tag.to_vec()))
+            })?)
+        }
+    }
+    if !optional.is_empty() {
+        problems.push(Status::info(
+            "optional-tables",
+            &format!(
+                "This font contains the following optional tables:\n\n    {}",
+                optional.join("\n    ")
+            ),
+        ))
+    }
+
+    let mut missing = vec![];
+    for tag in required_table_tags {
+        if !f.has_table(tag) {
+            missing.push(String::from_utf8(tag.to_vec()).map_err(|_| {
+                CheckError::Error(format!("Font tag '{:?}' wasn't UTF8?", tag.to_vec()))
+            })?);
+        }
+    }
+
+    // Note (from the OpenType spec):
+    // OpenType fonts that contain TrueType outlines should use the value of 0x00010000
+    // for sfntVersion. OpenType fonts containing CFF data (version 1 or 2) should use
+    // 0x4F54544F ('OTTO', when re-interpreted as a Tag) for sfntVersion.
+    let version = f.font().table_directory.sfnt_version();
+    if version == 0x4F54544F && (!f.has_table(b"CFF ") && !f.has_table(b"CFF2")) {
+        if f.has_table(b"fvar") {
+            missing.push("CFF2".to_string());
+        } else {
+            missing.push("CFF ".to_string());
+        }
+    } else if version == 0x00010000 && !f.has_table(b"glyf") {
+        missing.push("glyf".to_string());
+    }
+
+    if !missing.is_empty() {
+        problems.push(Status::fail(
+            "required-tables",
+            &format!(
+                "This font is missing the following required tables:\n\n    {}",
+                missing.join("\n    ")
+            ),
+        ))
+    }
+
+    return_result(problems)
+}

profile-opentype/src/checks/hhea.rs

@@ -1,7 +1,7 @@
 use std::collections::{HashMap, HashSet};
 
-use font_types::Fixed;
 use fontspector_checkapi::{prelude::*, skip, testfont, FileTypeConvert, TestFont};
+use font_types::Fixed;
 use read_fonts::{
     tables::stat::{AxisValue, AxisValueTableFlags},
     ReadError, TableProvider,
@@ -30,8 +30,7 @@ fn stat_axis_record(t: &Testable, context: &Context) -> CheckFnResult {
         .collect();
     let stat_axis_tags: HashSet<_> = f
         .font()
-        .stat()
-        .map_err(|_| CheckError::skip("no-stat", "No STAT table"))?
+        .stat()?
         .design_axes()?
         .iter()
         .map(|axis_record| axis_record.axis_tag().to_string())