Merge pull request #261 from linebender/groups-kerning-use-name

Use Name instead of String in Groups and Kerning

src/error.rs

@@ -9,6 +9,7 @@ use thiserror::Error;
 
 pub use crate::shared_types::ColorError;
 use crate::write::CustomSerializationError;
+use crate::Name;
 
 /// An error representing a failure to (re)name something.
 #[derive(Debug, Error)]
@@ -311,9 +312,9 @@ pub enum GroupsValidationError {
     #[error("the glyph '{glyph_name}' appears in more than one kerning group. Last found in '{group_name}'")]
     OverlappingKerningGroups {
         /// The glyph name.
-        glyph_name: String,
+        glyph_name: Name,
         /// The group name.
-        group_name: String,
+        group_name: Name,
     },
 }
 

src/groups.rs

@@ -6,7 +6,7 @@ use crate::Name;
 /// A map of group name to a list of glyph names.
 ///
 /// We use a [`BTreeMap`] because we need sorting for serialization.
-pub type Groups = BTreeMap<String, Vec<Name>>;
+pub type Groups = BTreeMap<Name, Vec<Name>>;
 
 /// Validate the contents of the groups.plist file according to the rules in the
 /// [Unified Font Object v3 specification for groups.plist](http://unifiedfontobject.org/versions/ufo3/groups.plist/#specification).
@@ -26,8 +26,8 @@ pub(crate) fn validate_groups(groups_map: &Groups) -> Result<(), GroupsValidatio
             for glyph_name in group_glyph_names {
                 if !kern1_set.insert(glyph_name) {
                     return Err(GroupsValidationError::OverlappingKerningGroups {
-                        glyph_name: glyph_name.to_string(),
-                        group_name: group_name.to_string(),
+                        glyph_name: glyph_name.clone(),
+                        group_name: group_name.clone(),
                     });
                 }
             }
@@ -39,8 +39,8 @@ pub(crate) fn validate_groups(groups_map: &Groups) -> Result<(), GroupsValidatio
             for glyph_name in group_glyph_names {
                 if !kern2_set.insert(glyph_name) {
                     return Err(GroupsValidationError::OverlappingKerningGroups {
-                        glyph_name: glyph_name.to_string(),
-                        group_name: group_name.to_string(),
+                        glyph_name: glyph_name.clone(),
+                        group_name: group_name.clone(),
                     });
                 }
             }

src/kerning.rs

@@ -3,14 +3,16 @@ use std::collections::BTreeMap;
 use serde::ser::{SerializeMap, Serializer};
 use serde::Serialize;
 
+use crate::Name;
+
 /// A map of kerning pairs.
 ///
 /// This is represented as a map of first half of a kerning pair (glyph name or group name)
 /// to the second half of a pair (glyph name or group name), which maps to the kerning value
 /// (high-level view: (first, second) => value).
 ///
 /// We use a [`BTreeMap`] because we need sorting for serialization.
-pub type Kerning = BTreeMap<String, BTreeMap<String, f64>>;
+pub type Kerning = BTreeMap<Name, BTreeMap<Name, f64>>;
 
 /// A helper for serializing kerning values.
 ///
@@ -22,7 +24,7 @@ pub(crate) struct KerningSerializer<'a> {
 }
 
 struct KerningInnerSerializer<'a> {
-    inner_kerning: &'a BTreeMap<String, f64>,
+    inner_kerning: &'a BTreeMap<Name, f64>,
 }
 
 impl<'a> Serialize for KerningSerializer<'a> {

src/upconversion.rs

@@ -7,6 +7,7 @@ use crate::fontinfo::FontInfo;
 use crate::groups::Groups;
 use crate::kerning::Kerning;
 use crate::names::NameList;
+use crate::Name;
 
 /// Convert kerning groups and pairs from v1 and v2 informal conventions to
 /// v3 formal conventions. Converted groups are added (duplicated) rather than
@@ -27,40 +28,40 @@ pub(crate) fn upconvert_kerning(
     // Make lists of groups referenced in kerning pairs, based on their side.
     for (first, seconds) in kerning {
         if groups.contains_key(first)
-            && !glyph_set.contains(first.as_str())
+            && !glyph_set.contains(&first)
             && !first.starts_with("public.kern1.")
         {
-            groups_first.insert(first.to_string());
+            groups_first.insert(first.clone());
         }
         for second in seconds.keys() {
             if groups.contains_key(second)
-                && !glyph_set.contains(second.as_str())
+                && !glyph_set.contains(&second)
                 && !second.starts_with("public.kern2.")
             {
-                groups_second.insert(second.to_string());
+                groups_second.insert(second.clone());
             }
         }
     }
 
     // Duplicate kerning groups with a new name.
     let mut groups_new = groups.clone();
 
-    let mut groups_first_old_to_new: HashMap<&String, String> = HashMap::new();
+    let mut groups_first_old_to_new: HashMap<Name, Name> = HashMap::new();
     for first in &groups_first {
         let first_new = make_unique_group_name(
-            format!("public.kern1.{}", first.replace("@MMK_L_", "")),
+            Name::new(&format!("public.kern1.{}", first.replace("@MMK_L_", ""))).unwrap(),
             &groups_new,
         );
-        groups_first_old_to_new.insert(first, first_new.to_string());
+        groups_first_old_to_new.insert(first.clone(), first_new.clone());
         groups_new.insert(first_new, groups_new.get(first).unwrap().clone());
     }
-    let mut groups_second_old_to_new: HashMap<&String, String> = HashMap::new();
+    let mut groups_second_old_to_new: HashMap<Name, Name> = HashMap::new();
     for second in &groups_second {
         let second_new = make_unique_group_name(
-            format!("public.kern2.{}", second.replace("@MMK_R_", "")),
+            Name::new(&format!("public.kern2.{}", second.replace("@MMK_R_", ""))).unwrap(),
             &groups_new,
         );
-        groups_second_old_to_new.insert(second, second_new.to_string());
+        groups_second_old_to_new.insert(second.clone(), second_new.clone());
         groups_new.insert(second_new, groups_new.get(second).unwrap().clone());
     }
 
@@ -69,41 +70,41 @@ pub(crate) fn upconvert_kerning(
 
     for (first, seconds) in kerning {
         let first_new = groups_first_old_to_new.get(first).unwrap_or(first);
-        let mut seconds_new: BTreeMap<String, f64> = BTreeMap::new();
+        let mut seconds_new: BTreeMap<Name, f64> = BTreeMap::new();
         for (second, value) in seconds {
             let second_new = groups_second_old_to_new.get(second).unwrap_or(second);
-            seconds_new.insert(second_new.to_string(), *value);
+            seconds_new.insert(second_new.clone(), *value);
         }
-        kerning_new.insert(first_new.to_string(), seconds_new);
+        kerning_new.insert(first_new.clone(), seconds_new);
     }
 
     (groups_new, kerning_new)
 }
 
-fn make_unique_group_name(name: String, existing_groups: &Groups) -> String {
+fn make_unique_group_name(name: Name, existing_groups: &Groups) -> Name {
     if !existing_groups.contains_key(&name) {
         return name;
     }
 
     let mut counter = 1;
-    let mut new_name = name.to_string();
+    let mut new_name = name.clone();
     while existing_groups.contains_key(&new_name) {
-        new_name = format!("{}{}", name, counter);
+        new_name = Name::new(&format!("{}{}", name, counter)).unwrap();
         counter += 1;
     }
 
     new_name
 }
 
-fn find_known_kerning_groups(groups: &Groups) -> (HashSet<String>, HashSet<String>) {
-    let mut groups_first: HashSet<String> = HashSet::new();
-    let mut groups_second: HashSet<String> = HashSet::new();
+fn find_known_kerning_groups(groups: &Groups) -> (HashSet<Name>, HashSet<Name>) {
+    let mut groups_first: HashSet<Name> = HashSet::new();
+    let mut groups_second: HashSet<Name> = HashSet::new();
 
     for name in groups.keys() {
         if name.starts_with("@MMK_L_") {
-            groups_first.insert(name.to_string());
+            groups_first.insert(name.clone());
         } else if name.starts_with("@MMK_R_") {
-            groups_second.insert(name.to_string());
+            groups_second.insert(name.clone());
         }
     }