review feedback

- add version() method to ColorOutline
- remove maxp lookup and change iterator to use max glyph in COLR table
- remove glyph_id from ColorOutline and change iterator to return (GlyphId, ColorOutline) tuple

skrifa/src/color/mod.rs

@@ -20,26 +20,29 @@ pub trait ColorPainter {}
 /// Affine transformation matrix.
 #[derive(Copy, Clone, Debug)]
 pub struct Transform {
-    xx: f32,
-    yx: f32,
-    xy: f32,
-    yy: f32,
-    dx: f32,
-    dy: f32,
+    pub xx: f32,
+    pub yx: f32,
+    pub xy: f32,
+    pub yy: f32,
+    pub dx: f32,
+    pub dy: f32,
 }
 
 /// Reference to a paint graph that represents a color glyph outline.
 #[derive(Clone)]
 pub struct ColorOutline<'a> {
     colr: colr::Colr<'a>,
-    glyph_id: GlyphId,
     kind: ColorOutlineKind<'a>,
 }
 
 impl<'a> ColorOutline<'a> {
-    /// Returns the glyph identifier that was used to retrieve this outline.
-    pub fn glyph_id(&self) -> GlyphId {
-        self.glyph_id
+    /// Returns the version of the color table from which this outline was
+    /// selected.
+    pub fn version(&self) -> u32 {
+        match &self.kind {
+            ColorOutlineKind::V0(..) => 0,
+            ColorOutlineKind::V1(..) => 1,
+        }
     }
 
     /// Evaluates the paint graph at the specified location in variation space
@@ -87,19 +90,14 @@ enum ColorOutlineKind<'a> {
 /// Collection of color outlines.
 #[derive(Clone)]
 pub struct ColorOutlineCollection<'a> {
-    glyph_count: u16,
     colr: Option<colr::Colr<'a>>,
 }
 
 impl<'a> ColorOutlineCollection<'a> {
     /// Creates a new color outline collection for the given font.
     pub fn new(font: &impl TableProvider<'a>) -> Self {
-        let glyph_count = font
-            .maxp()
-            .map(|maxp| maxp.num_glyphs())
-            .unwrap_or_default();
         let colr = font.colr().ok();
-        Self { glyph_count, colr }
+        Self { colr }
     }
 
     /// Returns the color outline for the given glyph identifier.
@@ -111,17 +109,38 @@ impl<'a> ColorOutlineCollection<'a> {
             let layer_range = colr.v0_base_glyph(glyph_id).ok()??;
             ColorOutlineKind::V0(layer_range)
         };
-        Some(ColorOutline {
-            colr,
-            glyph_id,
-            kind,
-        })
+        Some(ColorOutline { colr, kind })
     }
 
     /// Returns an iterator over all of the color outlines in the
     /// collection.
-    pub fn iter(&self) -> impl Iterator<Item = ColorOutline<'a>> + 'a + Clone {
+    pub fn iter(&self) -> impl Iterator<Item = (GlyphId, ColorOutline<'a>)> + 'a + Clone {
         let copy = self.clone();
-        (0..self.glyph_count).filter_map(move |gid| copy.get(GlyphId::new(gid)))
+        let max_glyph = copy
+            .colr
+            .as_ref()
+            .map(|colr| {
+                let max_v0 = if let Some(Ok(recs)) = colr.base_glyph_records() {
+                    recs.last()
+                        .map(|rec| rec.glyph_id().to_u16())
+                        .unwrap_or_default()
+                } else {
+                    0
+                };
+                let max_v1 = if let Some(Ok(list)) = colr.base_glyph_list() {
+                    list.base_glyph_paint_records()
+                        .last()
+                        .map(|rec| rec.glyph_id().to_u16())
+                        .unwrap_or_default()
+                } else {
+                    0
+                };
+                max_v0.max(max_v1)
+            })
+            .unwrap_or_default();
+        (0..=max_glyph).filter_map(move |gid| {
+            let gid = GlyphId::new(gid);
+            copy.get(gid).map(|outline| (gid, outline))
+        })
     }
 }