diff --git a/fea-rs/src/compile/compile_ctx.rs b/fea-rs/src/compile/compile_ctx.rs index d3dc8d54..6519c1e9 100644 --- a/fea-rs/src/compile/compile_ctx.rs +++ b/fea-rs/src/compile/compile_ctx.rs @@ -1362,71 +1362,85 @@ impl<'a, F: FeatureProvider, V: VariationInfo> CompilationCtx<'a, F, V> { } fn resolve_stylistic_set_feature(&mut self, tag: Tag, feature: &typed::Feature) { - let mut names = Vec::new(); - if let Some(feature_name) = feature.stylistic_set_feature_names() { - for name_spec in feature_name.statements() { - let resolved = self.resolve_name_spec(&name_spec); - names.push(resolved); + let mut names = BTreeMap::new(); + for item in feature.statements() { + if let Some(feature_name) = typed::FeatureNames::cast(item) { + for name_spec in feature_name.statements() { + let resolved = self.resolve_name_spec(&name_spec); + // in the case of duplicate names, last writer wins, see + // https://github.com/googlefonts/fontc/issues/1019 + let key = resolved.key(); + names.insert(key, resolved); + } + continue; } + self.resolve_statement(item); } if !names.is_empty() { - self.features.stylistic_sets.insert(tag, names); - } - for item in feature - .statements() - .filter(|node| node.kind() != Kind::FeatureNamesKw) - { - self.resolve_statement(item); + self.features + .stylistic_sets + .insert(tag, names.into_values().collect()); } } fn resolve_character_variant_feature(&mut self, tag: Tag, feature: &typed::Feature) { - if let Some(cv_params) = feature.character_variant_params() { - let mut params = CvParams::default(); - if let Some(node) = cv_params.feat_ui_label_name() { - params.feat_ui_label_name = node - .statements() - .map(|x| self.resolve_name_spec(&x)) - .collect(); - } - if let Some(node) = cv_params.feat_tooltip_text_name() { - params.feat_ui_tooltip_text_name = node - .statements() - .map(|x| self.resolve_name_spec(&x)) - .collect(); - } - if let Some(node) = cv_params.sample_text_name() { - params.sample_text_name = node - .statements() - .map(|x| self.resolve_name_spec(&x)) - .collect(); - } - if let Some(node) = cv_params.sample_text_name() { - params.sample_text_name = node - .statements() - .map(|x| self.resolve_name_spec(&x)) - .collect(); - } - for node in cv_params.param_ui_label_name() { - params.param_ui_label_names.push( - node.statements() - .map(|x| self.resolve_name_spec(&x)) - .collect(), - ); - } - for c in cv_params.characters() { - params.characters.push(c.value().parse_char().unwrap()); + let mut seen_cv_params = false; + for item in feature.statements() { + if let Some(cv_params) = typed::CvParameters::cast(item) { + if seen_cv_params { + self.warning( + cv_params.range(), + "Duplicate cvParameters block will be ignored. \ + This is not disallowed by the spec, but is not currently supported.", + ); + } else { + seen_cv_params = true; + let params = self.resolve_cv_params(&cv_params); + self.features.character_variants.insert(tag, params); + } + continue; } - - self.features.character_variants.insert(tag, params); + self.resolve_statement(item); } + } - for item in feature - .statements() - .filter(|node| node.kind() != Kind::CvParametersKw) - { - self.resolve_statement(item); + fn resolve_cv_params(&mut self, cv_params: &typed::CvParameters) -> CvParams { + let mut params = CvParams::default(); + if let Some(node) = cv_params.feat_ui_label_name() { + params.feat_ui_label_name = node + .statements() + .map(|x| self.resolve_name_spec(&x)) + .collect(); + } + if let Some(node) = cv_params.feat_tooltip_text_name() { + params.feat_ui_tooltip_text_name = node + .statements() + .map(|x| self.resolve_name_spec(&x)) + .collect(); + } + if let Some(node) = cv_params.sample_text_name() { + params.sample_text_name = node + .statements() + .map(|x| self.resolve_name_spec(&x)) + .collect(); + } + if let Some(node) = cv_params.sample_text_name() { + params.sample_text_name = node + .statements() + .map(|x| self.resolve_name_spec(&x)) + .collect(); + } + for node in cv_params.param_ui_label_name() { + params.param_ui_label_names.push( + node.statements() + .map(|x| self.resolve_name_spec(&x)) + .collect(), + ); + } + for c in cv_params.characters() { + params.characters.push(c.value().parse_char().unwrap()); } + params } fn resolve_size_feature(&mut self, feature: &typed::Feature) { diff --git a/fea-rs/src/compile/tables/name.rs b/fea-rs/src/compile/tables/name.rs index f047fe29..02169caf 100644 --- a/fea-rs/src/compile/tables/name.rs +++ b/fea-rs/src/compile/tables/name.rs @@ -70,6 +70,12 @@ impl NameSpec { Encoding::new(self.platform_id, self.encoding_id) != Encoding::Unknown } + // used to ensure we only choose one name for a given platform/encoding + // when multiple are provided + pub(crate) fn key(&self) -> (u16, u16, u16) { + (self.platform_id, self.encoding_id, self.language_id) + } + pub fn build(&self, name_id: NameId) -> write_fonts::tables::name::NameRecord { let string = parse_string(self.platform_id, self.string.trim_matches('"')); write_fonts::tables::name::NameRecord::new( diff --git a/fea-rs/src/compile/validate.rs b/fea-rs/src/compile/validate.rs index 1179081f..862475fa 100644 --- a/fea-rs/src/compile/validate.rs +++ b/fea-rs/src/compile/validate.rs @@ -599,7 +599,7 @@ impl<'a, V: VariationInfo> ValidationCtx<'a, V> { feature_tag: Tag, iter: impl Iterator, ) { - let mut is_first_item = true; + let mut has_seen_rule = false; for item in iter { if item.kind() == Kind::ScriptNode || item.kind() == Kind::LanguageNode @@ -613,10 +613,10 @@ impl<'a, V: VariationInfo> ValidationCtx<'a, V> { node.keyword().range(), "cvParameters block only valid in cv01-cv99 features", ); - } else if !is_first_item { + } else if has_seen_rule { self.error( node.keyword().range(), - "cvParameters must be first statement in feature block", + "cvParameters must precede any rules", ); } else { self.validate_character_variant_items(&node); @@ -627,24 +627,28 @@ impl<'a, V: VariationInfo> ValidationCtx<'a, V> { node.keyword().range(), "featureNames block only valid in ss01-ss20 features", ); - } else if !is_first_item { + } else if has_seen_rule { self.error( node.keyword().range(), - "featureNames must be first statement in feature block", + "featureNames must precede any rules", ); } else { self.validate_stylistic_set_items(&node); } } else if let Some(node) = typed::LookupRef::cast(item) { self.validate_lookup_ref(&node); + has_seen_rule = true; } else if let Some(node) = typed::LookupBlock::cast(item) { self.validate_lookup_block(&node, Some(feature_tag)); + has_seen_rule = true; } else if let Some(node) = typed::LookupFlag::cast(item) { self.validate_lookupflag(&node); } else if let Some(node) = typed::GsubStatement::cast(item) { self.validate_gsub_statement(&node); + has_seen_rule = true; } else if let Some(node) = typed::GposStatement::cast(item) { self.validate_gpos_statement(&node); + has_seen_rule = true; } else if let Some(node) = typed::GlyphClassDef::cast(item) { self.validate_glyph_class_def(&node); } else if let Some(node) = typed::MarkClassDef::cast(item) { @@ -662,7 +666,6 @@ impl<'a, V: VariationInfo> ValidationCtx<'a, V> { format!("unhandled item '{}' in feature", item.kind()), ); } - is_first_item = false; } } diff --git a/fea-rs/src/token_tree/typed.rs b/fea-rs/src/token_tree/typed.rs index c68e6ab9..57321ff7 100644 --- a/fea-rs/src/token_tree/typed.rs +++ b/fea-rs/src/token_tree/typed.rs @@ -671,14 +671,6 @@ impl Feature { self.iter().find_map(Tag::cast).unwrap() } - pub(crate) fn stylistic_set_feature_names(&self) -> Option { - self.statements().next().and_then(FeatureNames::cast) - } - - pub(crate) fn character_variant_params(&self) -> Option { - self.statements().next().and_then(CvParameters::cast) - } - pub(crate) fn statements(&self) -> impl Iterator { self.iter() .skip_while(|t| t.kind() != Kind::LBrace) diff --git a/fea-rs/test-data/compile-tests/mini-latin/bad/cv_params.ERR b/fea-rs/test-data/compile-tests/mini-latin/bad/cv_params.ERR index 5faf9351..7159dfd1 100644 --- a/fea-rs/test-data/compile-tests/mini-latin/bad/cv_params.ERR +++ b/fea-rs/test-data/compile-tests/mini-latin/bad/cv_params.ERR @@ -4,7 +4,7 @@ in ./test-data/compile-tests/mini-latin/bad/cv_params.fea at 3:4 3 | cvParameters { | ^^^^^^^^^^^^ -error: cvParameters must be first statement in feature block +error: cvParameters must precede any rules in ./test-data/compile-tests/mini-latin/bad/cv_params.fea at 9:4 | 9 | cvParameters { diff --git a/fea-rs/test-data/compile-tests/mini-latin/bad/stylistic_set_names.ERR b/fea-rs/test-data/compile-tests/mini-latin/bad/stylistic_set_names.ERR index 80eb826b..e270938d 100644 --- a/fea-rs/test-data/compile-tests/mini-latin/bad/stylistic_set_names.ERR +++ b/fea-rs/test-data/compile-tests/mini-latin/bad/stylistic_set_names.ERR @@ -4,7 +4,7 @@ in ./test-data/compile-tests/mini-latin/bad/stylistic_set_names.fea at 3:4 3 | featureNames { | ^^^^^^^^^^^^ -error: featureNames must be first statement in feature block +error: featureNames must precede any rules in ./test-data/compile-tests/mini-latin/bad/stylistic_set_names.fea at 10:4 | 10 | featureNames { diff --git a/fea-rs/test-data/compile-tests/mini-latin/good/duplicate_feature_names.fea b/fea-rs/test-data/compile-tests/mini-latin/good/duplicate_feature_names.fea new file mode 100644 index 00000000..ae54e618 --- /dev/null +++ b/fea-rs/test-data/compile-tests/mini-latin/good/duplicate_feature_names.fea @@ -0,0 +1,9 @@ +# https://github.com/googlefonts/fontc/issues/1019 +feature ss01 { + featureNames { + name "ss17"; + }; + featureNames { + name "Bulgarian letters"; + }; +} ss01; diff --git a/fea-rs/test-data/compile-tests/mini-latin/good/duplicate_feature_names.ttx b/fea-rs/test-data/compile-tests/mini-latin/good/duplicate_feature_names.ttx new file mode 100644 index 00000000..1f1aaa3b --- /dev/null +++ b/fea-rs/test-data/compile-tests/mini-latin/good/duplicate_feature_names.ttx @@ -0,0 +1,10 @@ + + + + + + Bulgarian letters + + + +