[ttx_diff] Review feedback

- less loops, more iterators - actually just use a BTreeSet for GlyphSet - better printing of anchors - improve integration into ttx_diff script (stop doing weird stdout redirection)
googlefonts · Nov 17, 2023 · 0ccce5b · 0ccce5b
1 parent 0d0d247
commit 0ccce5b
Show file tree

Hide file tree

Showing 3 changed files with 86 additions and 81 deletions.
diff --git a/layout-normalizer/src/common.rs b/layout-normalizer/src/common.rs
@@ -21,7 +21,7 @@ pub(crate) struct Feature {
 #[derive(Clone, Debug, PartialEq, PartialOrd, Eq, Ord)]
 pub(crate) enum GlyphSet {
     Single(GlyphId),
-    Multiple(Vec<GlyphId>),
+    Multiple(BTreeSet<GlyphId>),
 }
 
 impl Feature {
@@ -51,45 +51,47 @@ pub(crate) fn get_lang_systems(
 ) -> Vec<Feature> {
     let data = script_list.offset_data();
 
-    let lang_sys_iter = script_list.script_records().iter().flat_map(|script| {
-        let script_tag = script.script_tag();
-        let script = script.script(data).unwrap();
-        let maybe_default = script
-            .default_lang_sys()
-            .transpose()
-            .unwrap()
-            .map(|dflt| (script_tag, Tag::new(b"dflt"), dflt.feature_indices()));
-        let lang_sys_iter = script.lang_sys_records().iter().map(move |lang_sys| {
-            let lang_tag = lang_sys.lang_sys_tag();
-            let lang = lang_sys.lang_sys(script.offset_data()).unwrap();
-            (script_tag, lang_tag, lang.feature_indices())
-        });
-        maybe_default.into_iter().chain(lang_sys_iter)
-    });
-
-    let mut result = Vec::new();
-    for (script, lang, feature_indices) in lang_sys_iter {
-        let data = feature_list.offset_data();
-
-        for idx in feature_indices {
-            let rec = feature_list
-                .feature_records()
-                .get(idx.get() as usize)
-                .unwrap();
-            let feature = rec.feature(data).unwrap();
-            let lookups = feature
-                .lookup_list_indices()
-                .iter()
-                .map(|x| x.get())
-                .collect();
-            result.push(Feature {
-                feature: rec.feature_tag(),
-                script,
-                lang,
-                lookups,
+    let mut result = script_list
+        .script_records()
+        .iter()
+        // first iterate all (script, lang, feature indices)
+        .flat_map(|script| {
+            let script_tag = script.script_tag();
+            let script = script.script(data).unwrap();
+            let maybe_default = script
+                .default_lang_sys()
+                .transpose()
+                .unwrap()
+                .map(|dflt| (script_tag, Tag::new(b"dflt"), dflt.feature_indices()));
+            let lang_sys_iter = script.lang_sys_records().iter().map(move |lang_sys| {
+                let lang_tag = lang_sys.lang_sys_tag();
+                let lang = lang_sys.lang_sys(script.offset_data()).unwrap();
+                (script_tag, lang_tag, lang.feature_indices())
+            });
+            maybe_default.into_iter().chain(lang_sys_iter)
+        })
+        // then convert these into script/lang/feature/lookup indices
+        .flat_map(|(script, lang, indices)| {
+            indices.iter().map(move |idx| {
+                let rec = feature_list
+                    .feature_records()
+                    .get(idx.get() as usize)
+                    .unwrap();
+                let feature = rec.feature(feature_list.offset_data()).unwrap();
+                let lookups = feature
+                    .lookup_list_indices()
+                    .iter()
+                    .map(|x| x.get())
+                    .collect();
+                Feature {
+                    feature: rec.feature_tag(),
+                    script,
+                    lang,
+                    lookups,
+                }
             })
-        }
-    }
+        })
+        .collect::<Vec<_>>();
 
     result.sort_unstable_by_key(|sys| sys.sort_key());
 
@@ -99,7 +101,7 @@ pub(crate) fn get_lang_systems(
 impl GlyphSet {
     pub(crate) fn make_set(&mut self) {
         if let GlyphSet::Single(gid) = self {
-            *self = GlyphSet::Multiple(vec![*gid])
+            *self = GlyphSet::Multiple(BTreeSet::from([*gid]))
         }
     }
 
@@ -109,8 +111,21 @@ impl GlyphSet {
             unreachable!()
         };
         match other {
-            GlyphSet::Single(gid) => gids.push(gid),
-            GlyphSet::Multiple(multi) => gids.extend(multi),
+            GlyphSet::Single(gid) => {
+                gids.insert(gid);
+            }
+            GlyphSet::Multiple(mut multi) => gids.append(&mut multi),
+        }
+    }
+
+    pub(crate) fn add(&mut self, gid: GlyphId) {
+        // if we're a single glyph, don't turn into a set if we're adding ourselves
+        if matches!(self, GlyphSet::Single(x) if *x == gid) {
+            return;
+        }
+        self.make_set();
+        if let GlyphSet::Multiple(set) = self {
+            set.insert(gid);
         }
     }
 
@@ -163,13 +178,3 @@ impl FromIterator<GlyphId> for GlyphSet {
         GlyphSet::Multiple(iter.into_iter().collect())
     }
 }
-
-impl From<BTreeSet<GlyphId>> for GlyphSet {
-    fn from(src: BTreeSet<GlyphId>) -> GlyphSet {
-        if src.len() == 1 {
-            GlyphSet::Single(src.first().copied().unwrap())
-        } else {
-            src.into_iter().collect()
-        }
-    }
-}
diff --git a/layout-normalizer/src/gpos.rs b/layout-normalizer/src/gpos.rs
@@ -1,6 +1,6 @@
 use std::{
     any::Any,
-    collections::{BTreeMap, BTreeSet, HashMap},
+    collections::{BTreeMap, HashMap},
     fmt::{Debug, Display},
     io,
 };
@@ -655,16 +655,16 @@ fn get_mark_base_rules(
                 let mark_anchor = ResolvedAnchor::new(&mark_anchor, delta_computer)?;
                 marks
                     .entry(mark_anchor)
-                    .or_insert(BTreeSet::new())
-                    .insert(*mark_glyph);
+                    .or_insert_with(|| GlyphSet::from(*mark_glyph))
+                    .add(*mark_glyph);
             }
             let group = MarkAttachmentRule {
                 flags,
                 base: base_glyph,
                 base_anchor,
                 marks: marks
                     .into_iter()
-                    .map(|(anchor, glyphs)| (anchor, glyphs.into()))
+                    .map(|(anchor, glyphs)| (anchor, glyphs))
                     .collect(),
                 kind: LookupType::MarkToBase,
                 filter_set,
@@ -714,16 +714,16 @@ fn get_mark_mark_rules(
                 let mark_anchor = ResolvedAnchor::new(&mark_anchor, delta_computer)?;
                 marks
                     .entry(mark_anchor)
-                    .or_insert(BTreeSet::new())
-                    .insert(*mark_glyph);
+                    .or_insert_with(|| GlyphSet::from(*mark_glyph))
+                    .add(*mark_glyph);
             }
             let group = MarkAttachmentRule {
                 flags,
                 base: base_glyph,
                 base_anchor,
                 marks: marks
                     .into_iter()
-                    .map(|(anchor, glyphs)| (anchor, glyphs.into()))
+                    .map(|(anchor, glyphs)| (anchor, glyphs))
                     .collect(),
                 kind: LookupType::MarkToMark,
                 filter_set,
@@ -780,7 +780,7 @@ impl Display for ResolvedValue {
                 write!(f, " [({start})")?;
                 for (i, adj) in values.iter().enumerate() {
                     if i > 0 {
-                        write!(f, ", ")?;
+                        write!(f, ",")?;
                     }
                     write!(f, "{adj}")?;
                 }
@@ -790,7 +790,7 @@ impl Display for ResolvedValue {
                 write!(f, " {{")?;
                 for (i, var) in deltas.iter().enumerate() {
                     if i > 0 {
-                        write!(f, ", ")?;
+                        write!(f, ",")?;
                     }
                     write!(f, "{var}")?;
                 }
@@ -804,6 +804,6 @@ impl Display for ResolvedValue {
 
 impl Display for ResolvedAnchor {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "@({},{})", self.x, self.y)
+        write!(f, "@(x: {}, y: {})", self.x, self.y)
     }
 }
diff --git a/resources/scripts/ttx_diff.py b/resources/scripts/ttx_diff.py
@@ -22,7 +22,7 @@
 import shutil
 import subprocess
 import sys
-from typing import MutableSequence, Optional
+from typing import MutableSequence
 
 
 _COMPARE_DEFAULTS = "default"
@@ -46,27 +46,17 @@
 )
 
 
-def run(
-    cmd: MutableSequence,
-    working_dir: Path,
-    log_file: str,
-    redirect_stdout: Optional[str] = None,
-    **kwargs,
-):
+def run(cmd: MutableSequence, working_dir: Path, log_file: str, **kwargs):
     cmd_string = " ".join(cmd)
     print(f"  (cd {working_dir} && {cmd_string} > {log_file} 2>&1)")
     log_file = working_dir / log_file
     with open(log_file, "w") as log_file:
-        if redirect_stdout is not None:
-            stdout = open(working_dir / redirect_stdout, "w")
-        else:
-            stdout = log_file
         subprocess.run(
             cmd,
             text=True,
             check=True,
             cwd=working_dir,
-            stdout=stdout,
+            stdout=log_file,
             stderr=log_file,
             **kwargs,
         )
@@ -80,19 +70,29 @@ def ttx(font_file: Path):
         ttx_file.name,
         font_file.name,
     ]
-    run(cmd, font_file.parent, "ttx.log", None)
+    run(cmd, font_file.parent, "ttx.log")
     return ttx_file
 
 
 # generate a simple text repr for gpos for this font
 def simple_gpos_output(font_file: Path, out_path: Path):
-    temppath = font_file.parent / "gpos.txt"
-    cmd = ["cargo", "run", "-p", "layout-normalizer", "--", font_file.name, "--table", "gpos"]
+    temppath = font_file.parent / "markkern.txt"
+    cmd = [
+        "cargo",
+        "run",
+        "-p",
+        "layout-normalizer",
+        "--",
+        font_file.name,
+        "-o",
+        temppath.name,
+        "--table",
+        "gpos",
+    ]
     run(
         cmd,
         font_file.parent,
-        "gpos.log",
-        temppath.name,
+        "kernmark.log",
     )
     copy(temppath, out_path)
     with open(out_path) as f:
@@ -110,7 +110,7 @@ def build(
         if ttx_file.is_file():
             print(f"skipping {build_tool}")
             return ttx_file
-    run(cmd, build_dir, build_tool + ".log", None, **kwargs)
+    run(cmd, build_dir, build_tool + ".log", **kwargs)
     ttfs = ttf_find_fn()
     assert len(ttfs) == 1, ttfs
     return ttfs[0]