Skip to content

Commit c23b8b0

Browse files
simoncozenssimoncozens
committed
Improve error/skip story, add fvar regular coords check
1 parent ca042eb commit c23b8b0

File tree

12 files changed

+177
-31
lines changed

12 files changed

+177
-31
lines changed

‎fontspector-checkapi/src/check.rs

+28-5
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
use crate::{
2-
context::Context, font::FontCollection, prelude::FixFnResult, status::CheckFnResult,
2+
context::Context,
3+
font::FontCollection,
4+
prelude::FixFnResult,
5+
status::{CheckError, CheckFnResult},
36
CheckResult, Registry, Status, Testable,
47
};
58

@@ -65,12 +68,22 @@ impl<'a> Check<'a> {
6568
self.check_one.map(|check_one| {
6669
let subresults = match check_one(f, context) {
6770
Ok(results) => results.collect::<Vec<_>>(),
68-
Err(e) => vec![Status::error(&format!("Error: {}", e))],
71+
Err(CheckError::Error(e)) => vec![Status::error(&format!("Error: {}", e))],
72+
Err(CheckError::Skip { code, message }) => vec![Status::skip(&code, &message)],
6973
};
70-
self.status_to_result(subresults, Some(f), section)
74+
self.status_to_result(
75+
if subresults.is_empty() {
76+
Status::just_one_pass().collect()
77+
} else {
78+
subresults
79+
},
80+
Some(f),
81+
section,
82+
)
7183
})
7284
}
7385

86+
/// XXX This repeated code is horrible.
7487
pub fn run_all(
7588
&'a self,
7689
f: &'a FontCollection,
@@ -80,9 +93,19 @@ impl<'a> Check<'a> {
8093
self.check_all.map(|check_all| {
8194
let subresults = match check_all(f, context) {
8295
Ok(results) => results.collect::<Vec<_>>(),
83-
Err(e) => vec![Status::error(&format!("Error: {}", e))],
96+
Err(CheckError::Error(e)) => vec![Status::error(&format!("Error: {}", e))],
97+
Err(CheckError::Skip { code, message }) => vec![Status::skip(&code, &message)],
8498
};
85-
self.status_to_result(subresults, None, section)
99+
100+
self.status_to_result(
101+
if subresults.is_empty() {
102+
Status::just_one_pass().collect()
103+
} else {
104+
subresults
105+
},
106+
None,
107+
section,
108+
)
86109
})
87110
}
88111
}

‎fontspector-checkapi/src/lib.rs

+22-3
Original file line numberDiff line numberDiff line change
@@ -16,14 +16,33 @@ pub use filetype::{FileType, FileTypeConvert};
1616
pub use font::{FontCollection, TestFont, TTF};
1717
pub use profile::{Override, Profile};
1818
pub use registry::Registry;
19-
pub use status::{CheckFnResult, Status, StatusCode, StatusList};
19+
pub use status::{CheckError, CheckFnResult, Status, StatusCode, StatusList};
2020
pub use testable::Testable;
2121

2222
pub mod prelude {
23+
#[macro_export]
24+
macro_rules! testfont {
25+
($f: ident) => {
26+
TTF.from_testable($f)
27+
.ok_or(CheckError::Error("Not a TTF file".to_string()))?
28+
};
29+
}
30+
#[macro_export]
31+
macro_rules! fixfont {
32+
($f: ident) => {
33+
TTF.from_testable($f).ok_or("Not a TTF file")?
34+
};
35+
}
36+
#[macro_export]
37+
macro_rules! skip {
38+
($code: expr, $message: expr) => {
39+
return Ok(Status::just_one_skip($code, $message));
40+
};
41+
}
2342
pub type FixFnResult = Result<bool, String>;
2443
pub use crate::{
25-
return_result, Check, CheckFlags, CheckFnResult, Context, FileType, Profile, Registry,
26-
Status, StatusList, Testable, TTF,
44+
return_result, Check, CheckError, CheckFlags, CheckFnResult, Context, FileType, Profile,
45+
Registry, Status, StatusList, Testable, TTF,
2746
};
2847
}
2948

‎fontspector-checkapi/src/status.rs

+33-1
Original file line numberDiff line numberDiff line change
@@ -71,10 +71,18 @@ impl Status {
7171
Box::new(vec![Status::pass()].into_iter())
7272
}
7373

74+
pub fn just_one_warn(code: &str, message: &str) -> Box<dyn Iterator<Item = Status>> {
75+
Box::new(vec![Status::warn(code, message)].into_iter())
76+
}
77+
7478
pub fn just_one_fail(code: &str, message: &str) -> Box<dyn Iterator<Item = Status>> {
7579
Box::new(vec![Status::fail(code, message)].into_iter())
7680
}
7781

82+
pub fn just_one_skip(code: &str, message: &str) -> Box<dyn Iterator<Item = Status>> {
83+
Box::new(vec![Status::skip(code, message)].into_iter())
84+
}
85+
7886
pub fn pass() -> Self {
7987
Self {
8088
message: None,
@@ -89,6 +97,13 @@ impl Status {
8997
severity: StatusCode::Fail,
9098
}
9199
}
100+
pub fn warn(code: &str, message: &str) -> Self {
101+
Self {
102+
message: Some(message.to_string()),
103+
code: Some(code.to_string()),
104+
severity: StatusCode::Warn,
105+
}
106+
}
92107
pub fn skip(code: &str, message: &str) -> Self {
93108
Self {
94109
message: Some(message.to_string()),
@@ -112,5 +127,22 @@ impl Status {
112127
}
113128
}
114129

130+
/// Reflects the result of some kind of early return from a check function
131+
///
132+
/// This may be because there was an error, or because the check was skipped.
133+
pub enum CheckError {
134+
Error(String),
135+
Skip { message: String, code: String },
136+
}
137+
138+
impl<T> From<T> for CheckError
139+
where
140+
T: std::error::Error,
141+
{
142+
fn from(e: T) -> Self {
143+
CheckError::Error(e.to_string())
144+
}
145+
}
146+
115147
pub type StatusList = Box<dyn Iterator<Item = Status>>;
116-
pub type CheckFnResult = Result<StatusList, String>;
148+
pub type CheckFnResult = Result<StatusList, CheckError>;

‎profile-googlefonts/src/family.rs

+7-2
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,15 @@ use std::collections::HashSet;
44
use fontspector_checkapi::{prelude::*, FileTypeConvert};
55

66
fn family_equal_codepoint_coverage(c: &Testable, _context: &Context) -> CheckFnResult {
7-
let f = TTF.from_testable(c).ok_or("Not a TTF file")?;
7+
let f = TTF
8+
.from_testable(c)
9+
.ok_or(CheckError::Error("Not a TTF file".to_string()))?;
810
let siblings = f.siblings();
911
if siblings.is_empty() {
10-
return return_result(vec![Status::skip("no-siblings", "No sibling fonts found")]);
12+
return Err(CheckError::Skip {
13+
code: "no-siblings".to_string(),
14+
message: "No sibling fonts found".to_string(),
15+
});
1116
}
1217
let mut problems = vec![];
1318
let mut we_have_they_dont: HashSet<u32> = HashSet::new();

‎profile-googlefonts/src/metadata.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,9 @@ use fontspector_checkapi::prelude::*;
77

88
fn validate_metadatapb(c: &Testable, _context: &Context) -> CheckFnResult {
99
let mdpb = std::fs::read_to_string(&c.filename)
10-
.map_err(|e| format!("Couldn't open metadata file: {}", e))?;
10+
.map_err(|e| CheckError::Error(format!("Couldn't open metadata file: {}", e)))?;
1111
let msg = protobuf::text_format::parse_from_str::<FamilyProto>(&mdpb)
12-
.map_err(|e| format!("Error parsing METADATA.pb: {}", e))?;
12+
.map_err(|e| CheckError::Error(format!("Error parsing METADATA.pb: {}", e)))?;
1313
let mut problems = vec![];
1414
if let Some(designer) = msg.designer.as_ref() {
1515
if designer.contains('/') {

‎profile-testplugin/src/lib.rs

+2-1
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,8 @@ fn say_hello(_c: &Testable, context: &Context) -> CheckFnResult {
1010
}
1111

1212
fn validate_toml(c: &Testable, _context: &Context) -> CheckFnResult {
13-
let toml = std::fs::read_to_string(&c.filename).map_err(|_| "Couldn't open file")?;
13+
let toml = std::fs::read_to_string(&c.filename)
14+
.map_err(|_| CheckError::Error("Couldn't open file".to_string()))?;
1415
Ok(if toml::from_str::<toml::Value>(&toml).is_ok() {
1516
Status::just_one_pass()
1617
} else {

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

+62
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
use std::{borrow::Cow, collections::HashMap, vec};
2+
3+
use fontspector_checkapi::{prelude::*, skip, testfont, FileTypeConvert};
4+
use skrifa::MetadataProvider;
5+
6+
fn regular_coords_correct(t: &Testable, _context: &Context) -> CheckFnResult {
7+
let f = testfont!(t);
8+
if !f.is_variable_font() {
9+
skip!("not-variable", "Not a variable font");
10+
}
11+
let axes = f.font().axes();
12+
let mut problems = vec![];
13+
let regular_instance = f
14+
.font()
15+
.named_instances()
16+
.iter()
17+
.find(|ni| {
18+
f.font()
19+
.localized_strings(ni.subfamily_name_id())
20+
.any(|s| "Regular" == s.chars().collect::<Cow<str>>())
21+
})
22+
.ok_or(CheckError::Skip {
23+
code: "no-regular".to_string(),
24+
message: "No Regular instance found".to_string(),
25+
})?;
26+
let regular_location: HashMap<String, f32> = regular_instance
27+
.user_coords()
28+
.zip(axes.iter())
29+
.map(|(coord, axis)| (axis.tag().to_string(), coord))
30+
.collect();
31+
let expectations = vec![("wght", 400.0), ("wdth", 100.0), ("slnt", 0.0)];
32+
for (axis, expected) in expectations {
33+
if let Some(actual) = regular_location.get(axis) {
34+
if *actual != expected {
35+
problems.push(Status::fail(
36+
axis,
37+
&format!(
38+
"Regular instance has {} coordinate of {}, expected {}",
39+
axis, actual, expected
40+
),
41+
));
42+
}
43+
}
44+
}
45+
return_result(problems)
46+
}
47+
48+
pub const CHECK_REGULAR_COORDS_CORRECT: Check = Check {
49+
id: "com.google.fonts/check/fvar/regular_coords_correct",
50+
title: "Regular instance coordinates are correct?",
51+
rationale: "According to the Open-Type spec's registered design-variation tags, the Regular instance in a variable font should have certain prescribed values.
52+
If a variable font has a 'wght' (Weight) axis, then the coordinate of its 'Regular' instance is required to be 400.
53+
If a variable font has a 'wdth' (Width) axis, then the coordinate of its 'Regular' instance is required to be 100.
54+
If a variable font has a 'slnt' (Slant) axis, then the coordinate of its 'Regular' instance is required to be 0.",
55+
proposal: "https://github.com/fonttools/fontbakery/issues/1707",
56+
check_one: Some(&regular_coords_correct),
57+
check_all: None,
58+
applies_to: "TTF",
59+
hotfix: None,
60+
fix_source: None,
61+
flags: CheckFlags::default(),
62+
};

‎profile-universal/src/checks/mod.rs

+2
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
mod bold_italic_unique;
2+
mod fvar;
23
mod name_trailing_spaces;
34
mod required_tables;
45
mod unwanted_tables;
6+
pub use fvar::CHECK_REGULAR_COORDS_CORRECT;
57
// pub use bold_italic_unique::CHECK_BOLD_ITALIC_UNIQUE;
68
pub use name_trailing_spaces::CHECK_NAME_TRAILING_SPACES;
79
pub use required_tables::CHECK_REQUIRED_TABLES;

‎profile-universal/src/checks/name_trailing_spaces.rs

+3-3
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
1-
use fontspector_checkapi::{prelude::*, FileTypeConvert};
1+
use fontspector_checkapi::{prelude::*, testfont, FileTypeConvert};
22
use read_fonts::TableProvider;
33

44
fn name_trailing_spaces(f: &Testable, _context: &Context) -> CheckFnResult {
55
let mut problems: Vec<Status> = vec![];
66

7-
if let Ok(name_table) = TTF.from_testable(f).ok_or("Not a TTF file")?.font().name() {
7+
if let Ok(name_table) = testfont!(f).font().name() {
88
for name_record in name_table.name_record().iter() {
99
if name_record
1010
.string(name_table.string_data())
@@ -18,7 +18,7 @@ fn name_trailing_spaces(f: &Testable, _context: &Context) -> CheckFnResult {
1818
name_record.encoding_id,
1919
name_record.language_id,
2020
name_record.name_id,
21-
name_record.string(name_table.string_data()).map_err(|_| "Error reading string".to_string())?,
21+
name_record.string(name_table.string_data()).map_err(|_| CheckError::Error("Error reading string".to_string()))?,
2222
)))
2323
}
2424
}

‎profile-universal/src/checks/required_tables.rs

+8-10
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
1-
use fontspector_checkapi::{prelude::*, FileTypeConvert};
1+
use fontspector_checkapi::{prelude::*, testfont, FileTypeConvert};
22

33
const OPTIONAL_TABLE_TAGS: [&[u8; 4]; 20] = [
44
b"cvt ", b"fpgm", b"loca", b"prep", b"VORG", b"EBDT", b"EBLC", b"EBSC", b"BASE", b"GPOS",
55
b"GSUB", b"JSTF", b"gasp", b"hdmx", b"LTSH", b"PCLT", b"VDMX", b"vhea", b"vmtx", b"kern",
66
];
77

88
fn required_tables(t: &Testable, _context: &Context) -> CheckFnResult {
9-
let f = TTF.from_testable(t).ok_or("Not a TTF file")?;
9+
let f = testfont!(t);
1010
let mut required_table_tags: Vec<_> = vec![
1111
b"cmap", b"head", b"hhea", b"hmtx", b"maxp", b"name", b"OS/2", b"post",
1212
];
@@ -32,10 +32,9 @@ fn required_tables(t: &Testable, _context: &Context) -> CheckFnResult {
3232

3333
for tag in OPTIONAL_TABLE_TAGS {
3434
if f.has_table(tag) {
35-
optional.push(
36-
String::from_utf8(tag.to_vec())
37-
.map_err(|_| format!("Font tag '{:?}' wasn't UTF8?", tag.to_vec()))?,
38-
)
35+
optional.push(String::from_utf8(tag.to_vec()).map_err(|_| {
36+
CheckError::Error(format!("Font tag '{:?}' wasn't UTF8?", tag.to_vec()))
37+
})?)
3938
}
4039
}
4140
if !optional.is_empty() {
@@ -51,10 +50,9 @@ fn required_tables(t: &Testable, _context: &Context) -> CheckFnResult {
5150
let mut missing = vec![];
5251
for tag in required_table_tags {
5352
if !f.has_table(tag) {
54-
missing.push(
55-
String::from_utf8(tag.to_vec())
56-
.map_err(|_| format!("Font tag '{:?}' wasn't UTF8?", tag.to_vec()))?,
57-
);
53+
missing.push(String::from_utf8(tag.to_vec()).map_err(|_| {
54+
CheckError::Error(format!("Font tag '{:?}' wasn't UTF8?", tag.to_vec()))
55+
})?);
5856
}
5957
}
6058

‎profile-universal/src/checks/unwanted_tables.rs

+6-4
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
use font_types::Tag;
2-
use fontspector_checkapi::{prelude::*, FileTypeConvert};
2+
use fontspector_checkapi::{fixfont, prelude::*, testfont, FileTypeConvert};
33
use write_fonts::FontBuilder;
44

5-
const UNWANTED_TABLES: [(Tag, &str); 8] = [
5+
const UNWANTED_TABLES: [(Tag, &str); 9] = [
6+
(Tag::new(b"DSIG"), "This font has a digital signature (DSIG table) which is only required - even if only a placeholder - on old programs like MS Office 2013 in order to work properly.\n
7+
The current recommendation is to completely remove the DSIG table."),
68
(Tag::new(b"FFTM"), "Table contains redundant FontForge timestamp info"),
79
(Tag::new(b"TTFA"), "Redundant TTFAutohint table"),
810
(Tag::new(b"TSI0"), "Table contains data only used in VTT"),
@@ -14,7 +16,7 @@ const UNWANTED_TABLES: [(Tag, &str); 8] = [
1416
];
1517

1618
fn unwanted_tables(t: &Testable, _context: &Context) -> CheckFnResult {
17-
let f = TTF.from_testable(t).ok_or("Not a TTF file")?;
19+
let f = testfont!(t);
1820

1921
let mut reasons = vec![];
2022
for (table, reason) in UNWANTED_TABLES.iter() {
@@ -33,7 +35,7 @@ fn unwanted_tables(t: &Testable, _context: &Context) -> CheckFnResult {
3335
}
3436

3537
fn delete_unwanted_tables(t: &Testable) -> FixFnResult {
36-
let f = TTF.from_testable(t).ok_or("Not a TTF file".to_string())?;
38+
let f = fixfont!(t);
3739
let unwanted_tags = UNWANTED_TABLES
3840
.iter()
3941
.map(|(tag, _)| tag)

‎profile-universal/src/lib.rs

+2
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,15 @@ impl fontspector_checkapi::Plugin for Universal {
99
cr.register_check(checks::CHECK_NAME_TRAILING_SPACES);
1010
cr.register_check(checks::CHECK_UNWANTED_TABLES);
1111
cr.register_check(checks::CHECK_REQUIRED_TABLES);
12+
cr.register_check(checks::CHECK_REGULAR_COORDS_CORRECT);
1213
let profile = Profile::from_toml(
1314
r#"
1415
[sections]
1516
"Universal Profile Checks" = [
1617
"com.google.fonts/check/name/trailing_spaces",
1718
"com.google.fonts/check/unwanted_tables",
1819
"com.google.fonts/check/required_tables",
20+
"com.google.fonts/check/fvar/regular_coords_correct",
1921
]
2022
"#,
2123
)

0 commit comments

Comments
 (0)