Skip to content
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

We no longer have to avoid reusing non-font-specific names #1119

Merged
merged 1 commit into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions fontbe/src/fvar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,6 @@ fn generate_fvar(static_metadata: &StaticMetadata) -> Option<Fvar> {
let reverse_names: HashMap<_, _> = static_metadata
.names
.iter()
// To match fontmake we should use the font-specific name range and not reuse
// a well-known name, even if the name matches.
.filter(|(key, _)| key.name_id.to_u16() > 255)
.map(|(key, name)| (name.as_str(), key.name_id))
.collect();

Expand Down
3 changes: 0 additions & 3 deletions fontbe/src/stat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,6 @@ impl Work<Context, AnyWorkId, Error> for StatWork {
let reverse_names: HashMap<_, _> = static_metadata
.names
.iter()
// To match fontmake we should use the font-specific name range and not reuse
// a well-known name, even if the name matches.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

happy to delete a confusing special case!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confused me when I put that code in and again today rofl

.filter(|(key, _)| key.name_id.to_u16() > 255)
.map(|(key, name)| (name.as_str(), key.name_id))
.collect();

Expand Down
16 changes: 16 additions & 0 deletions fontc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3306,4 +3306,20 @@ mod tests {

assert_eq!(vec![expected_rot30_bbox, expected_rot60more_bbox], boxes);
}

#[test]
fn unique_names() {
// Axis name matches subfamily; they should share
let compile = TestCompile::compile_source("glyphs3/DuplicateNames.glyphs");
let axis_name_ids = compile
.font()
.fvar()
.unwrap()
.axes()
.unwrap()
.iter()
.map(|a| a.axis_name_id())
.collect::<Vec<_>>();
assert_eq!(vec![NameId::SUBFAMILY_NAME], axis_name_ids);
}
}
4 changes: 2 additions & 2 deletions fontir/src/ir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -424,14 +424,14 @@ impl StaticMetadata {
// Claim names for axes and named instances
let mut name_id_gen = 255;
let mut key_to_name = names;
let mut visited = HashSet::new();
let mut visited = key_to_name.values().cloned().collect::<HashSet<_>>();

variable_axes
.iter()
.map(|axis| axis.ui_label_name())
.chain(named_instances.iter().map(|ni| ni.name.as_ref()))
.for_each(|name| {
if !visited.insert(name) {
if !visited.insert(name.to_string()) {
return;
}
name_id_gen += 1;
Expand Down
45 changes: 45 additions & 0 deletions resources/testdata/glyphs3/DuplicateNames.glyphs
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
{
.formatVersion = 3;
axes = (
{
name = Regular;
tag = wght;
}
);
familyName = DuplicateNames;
fontMaster = (
{
axesValues = (
400
);
id = m01;
name = Regular;
},
{
axesValues = (
700
);
iconName = Bold;
id = "E09E0C54-128D-4FEA-B209-1B70BEFE300B";
name = Bold;
}
);
glyphs = (
{
glyphname = space;
lastChange = "2022-12-01 04:58:12 +0000";
layers = (
{
layerId = m01;
width = 200;
},
{
layerId = "E09E0C54-128D-4FEA-B209-1B70BEFE300B";
width = 600;
}
);
unicode = 32;
},
);
unitsPerEm = 1000;
}
Loading