From 583cc826ebe4eac03581c31eec576580e514aa71 Mon Sep 17 00:00:00 2001
From: Ben Pfaff <blp@cs.stanford.edu>
Date: Tue, 18 Aug 2026 17:38:20 -0700
Subject: [PATCH] more types cleanup

---
 rust/pspp/src/spv/read/graph.rs | 299 ++++++++++++++++++--------------
 1 file changed, 166 insertions(+), 133 deletions(-)

diff --git a/rust/pspp/src/spv/read/graph.rs b/rust/pspp/src/spv/read/graph.rs
index 88cf30c204..61ca63f5f0 100644
--- a/rust/pspp/src/spv/read/graph.rs
+++ b/rust/pspp/src/spv/read/graph.rs
@@ -122,20 +122,116 @@ pub enum GraphError {
     /// Error parsing XML data: {0}
     XmlError(#[from] serde_path_to_error::Error<quick_xml::DeError>),
 
-    /// Graph has no `coordinates` element.
-    NoCoordinates,
-
-    /// Graph has multiple `{0}` child elements.
-    DuplicateGraphChild(&'static str),
-
     /// Graph references missing variable `{0}`.
     MissingVariable(String),
 }
 
+#[derive(Debug)]
+pub struct Visualization {
+    /// Locale used for output, e.g. `en-US`.
+    lang: Option<String>,
+
+    /// The `id` for the base [Style] for the graph.
+    _style: Option<String>,
+
+    /// Title or caption.
+    _description: Option<String>,
+
+    /// VizML Schema version number.
+    _version: Option<String>,
+
+    id: Option<String>,
+
+    source_variables: Vec<SourceVariable>,
+    derived_variables: Vec<DerivedVariable>,
+    delimited_file_sources: Vec<DelimitedFileSource>,
+    embedded_sources: Vec<EmbeddedSource>,
+    categorical_domains: Vec<CategoricalDomain>,
+    interval_domains: Vec<IntervalDomain>,
+    graphs: Vec<Graph>,
+    label_frames: Vec<LabelFrame>,
+    containers: Vec<Container>,
+    styles: Vec<Style>,
+    style_cycles: Vec<StyleCycle>,
+    locations: Vec<Location>,
+}
+
+impl<'de> Deserialize<'de> for Visualization {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: Deserializer<'de>,
+    {
+        let RawVisualization {
+            lang,
+            _style,
+            _description,
+            _version,
+            id,
+            children,
+        } = RawVisualization::deserialize(deserializer)?;
+        let mut source_variables = Vec::new();
+        let mut derived_variables = Vec::new();
+        let mut delimited_file_sources = Vec::new();
+        let mut embedded_sources = Vec::new();
+        let mut categorical_domains = Vec::new();
+        let mut interval_domains = Vec::new();
+        let mut graphs = Vec::new();
+        let mut label_frames = Vec::new();
+        let mut containers = Vec::new();
+        let mut styles = Vec::new();
+        let mut style_cycles = Vec::new();
+        let mut locations = Vec::new();
+
+        for child in children {
+            match child {
+                VisChild::SourceVariable(source_variable) => source_variables.push(source_variable),
+                VisChild::DerivedVariable(derived_variable) => {
+                    derived_variables.push(derived_variable)
+                }
+                VisChild::UserSource => (),
+                VisChild::DelimitedFileSource(delimited_file_source) => {
+                    delimited_file_sources.push(delimited_file_source)
+                }
+                VisChild::EmbeddedSource(embedded_source) => embedded_sources.push(embedded_source),
+                VisChild::CategoricalDomain(categorical_domain) => {
+                    categorical_domains.push(categorical_domain)
+                }
+                VisChild::IntervalDomain(interval_domain) => interval_domains.push(interval_domain),
+                VisChild::Graph(graph) => graphs.push(graph),
+                VisChild::LabelFrame(label_frame) => label_frames.push(label_frame),
+                VisChild::Container(container) => containers.push(container),
+                VisChild::StyleCycle(style_cycle) => style_cycles.push(style_cycle),
+                VisChild::Style(style) => styles.push(style),
+                VisChild::Location(location) => locations.push(location),
+                VisChild::Other => (),
+            }
+        }
+        Ok(Self {
+            lang,
+            _style,
+            _description,
+            _version,
+            id,
+            source_variables,
+            derived_variables,
+            delimited_file_sources,
+            embedded_sources,
+            categorical_domains,
+            interval_domains,
+            graphs,
+            label_frames,
+            containers,
+            styles,
+            style_cycles,
+            locations,
+        })
+    }
+}
+
 #[derive(Deserialize, Debug)]
 #[serde(rename_all = "camelCase")]
-pub struct Visualization {
-    // Locale used for output, e.g. `en-US`.
+pub struct RawVisualization {
+    /// Locale used for output, e.g. `en-US`.
     #[serde(rename = "@lang")]
     lang: Option<String>,
 
@@ -160,25 +256,30 @@ pub struct Visualization {
 
 impl Locations for Visualization {
     fn locations(&self) -> Vec<&Location> {
-        self.children
-            .iter()
-            .filter_map(|child| child.location())
-            .collect()
+        self.locations.iter().collect()
     }
 }
 
 impl Visualization {
+    fn all_variables(&self) -> impl Iterator<Item = &dyn Variable> {
+        let source_variables = self
+            .source_variables
+            .iter()
+            .map(|variable| variable as &dyn Variable);
+        let derived_variables = self
+            .derived_variables
+            .iter()
+            .map(|variable| variable as &dyn Variable);
+        source_variables.chain(derived_variables)
+    }
+
     fn decode_series(
         &self,
         locale: &'static Locale<'static>,
         data: DataSources,
         warn: &mut dyn FnMut(GraphWarning),
     ) -> BTreeMap<&str, Series> {
-        let mut variables: Vec<&dyn Variable> = self
-            .children
-            .iter()
-            .filter_map(|child| child.variable())
-            .collect();
+        let mut variables = self.all_variables().collect_vec();
         let mut series = BTreeMap::<&str, Series>::new();
         while !variables.is_empty() {
             let n = variables.len();
@@ -196,16 +297,24 @@ impl Visualization {
         series
     }
 
+    fn all_domains(&self) -> impl Iterator<Item = &dyn RawDomain> {
+        let categorical_domains = self
+            .categorical_domains
+            .iter()
+            .map(|domain| domain as &dyn RawDomain);
+        let interval_domains = self
+            .interval_domains
+            .iter()
+            .map(|domain| domain as &dyn RawDomain);
+        categorical_domains.chain(interval_domains)
+    }
+
     fn decode_domains(
         &self,
         series: &BTreeMap<&str, Series>,
         _warn: &mut dyn FnMut(GraphWarning),
     ) -> BTreeMap<&str, Domain> {
-        let mut inputs = self
-            .children
-            .iter()
-            .filter_map(|child| child.categorical_domain())
-            .collect::<Vec<_>>();
+        let mut inputs = self.all_domains().collect_vec();
         let mut domains = BTreeMap::<&str, Domain>::new();
         while !inputs.is_empty() {
             let n = inputs.len();
@@ -301,43 +410,38 @@ impl Visualization {
                 }
             }
         }
-        for child in &self.children {
-            match child {
-                VisChild::Graph(graph) => {
-                    let index = frames.len();
-                    frames[0].children.push(index);
-                    if let Some(id) = &graph.id {
-                        ids.insert(id, index);
-                    }
-                    frames.push(Frame {
-                        id: graph.id.clone(),
-                        frame_type: FrameType::Graph(graph),
-                        parent: Some(0),
-                        children: Vec::new(),
-                        locations: EnumMap::default(),
-                        position: EnumMap::default(),
-                    });
-                }
-                VisChild::LabelFrame(label_frame) => {
-                    let index = frames.len();
-                    frames[0].children.push(index);
-                    if let Some(id) = &label_frame.id {
-                        ids.insert(id, index);
-                    }
-                    frames.push(Frame {
-                        id: label_frame.id.clone(),
-                        frame_type: FrameType::LabelFrame(label_frame),
-                        parent: Some(0),
-                        children: Vec::new(),
-                        locations: EnumMap::default(),
-                        position: EnumMap::default(),
-                    });
-                }
-                VisChild::Container(container) => {
-                    decode_container(0, container, &mut frames, &mut ids)
-                }
-                _ => (),
+        for graph in &self.graphs {
+            let index = frames.len();
+            frames[0].children.push(index);
+            if let Some(id) = &graph.id {
+                ids.insert(id, index);
+            }
+            frames.push(Frame {
+                id: graph.id.clone(),
+                frame_type: FrameType::Graph(graph),
+                parent: Some(0),
+                children: Vec::new(),
+                locations: EnumMap::default(),
+                position: EnumMap::default(),
+            });
+        }
+        for label_frame in &self.label_frames {
+            let index = frames.len();
+            frames[0].children.push(index);
+            if let Some(id) = &label_frame.id {
+                ids.insert(id, index);
             }
+            frames.push(Frame {
+                id: label_frame.id.clone(),
+                frame_type: FrameType::LabelFrame(label_frame),
+                parent: Some(0),
+                children: Vec::new(),
+                locations: EnumMap::default(),
+                position: EnumMap::default(),
+            });
+        }
+        for container in &self.containers {
+            decode_container(0, container, &mut frames, &mut ids)
         }
 
         for frame in &mut frames {
@@ -362,16 +466,15 @@ impl Visualization {
         }
 
         let mut styles = HashMap::new();
-        for node in self.children.iter().filter_map(|child| child.style()) {
+        for node in &self.styles {
             inner(node.clone(), &mut styles);
         }
         styles
     }
 
     fn decode_style_cycles(&self) -> HashMap<&str, &StyleCycle> {
-        self.children
+        self.style_cycles
             .iter()
-            .filter_map(|child| child.style_cycle())
             .filter_map(|style_cycle| style_cycle.id.as_deref().map(|s| (s, style_cycle)))
             .collect()
     }
@@ -499,12 +602,6 @@ impl Visualization {
         Ok(elements)
     }
 
-    fn delimited_file_source(&self) -> Option<&DelimitedFileSource> {
-        self.children
-            .iter()
-            .find_map(|child| child.delimited_file_source())
-    }
-
     pub fn decode(
         &self,
         mut data: DataSources,
@@ -512,11 +609,7 @@ impl Visualization {
         warn: &mut dyn FnMut(GraphWarning),
     ) -> Result<svg::Document, GraphError> {
         let style_cycles = self.decode_style_cycles();
-        for delimited_source in self
-            .children
-            .iter()
-            .filter_map(|child| child.delimited_file_source())
-        {
+        for delimited_source in &self.delimited_file_sources {
             if let Some(csv_data) = csv_data.take() {
                 data.0.insert(delimited_source.id.clone(), csv_data);
             } else {
@@ -526,11 +619,7 @@ impl Visualization {
         if csv_data.is_some() {
             warn(GraphWarning::CsvDataWithoutDelimitedSource);
         }
-        for embedded_source in self
-            .children
-            .iter()
-            .filter_map(|child| child.embedded_source())
-        {
+        for embedded_source in &self.embedded_sources {
             data.0
                 .insert(embedded_source.id.clone(), embedded_source.decode());
         }
@@ -1264,53 +1353,6 @@ enum VisChild {
     Other,
 }
 
-impl VisChild {
-    fn variable(&self) -> Option<&dyn Variable> {
-        match self {
-            VisChild::SourceVariable(source_variable) => Some(source_variable),
-            VisChild::DerivedVariable(derived_variable) => Some(derived_variable),
-            _ => None,
-        }
-    }
-    fn categorical_domain(&self) -> Option<&dyn RawDomain> {
-        match self {
-            VisChild::CategoricalDomain(categorical_domain) => Some(categorical_domain),
-            VisChild::IntervalDomain(interval_domain) => Some(interval_domain),
-            _ => None,
-        }
-    }
-    fn style(&self) -> Option<&Style> {
-        match self {
-            Self::Style(style) => Some(style),
-            _ => None,
-        }
-    }
-    fn location(&self) -> Option<&Location> {
-        match self {
-            Self::Location(location) => Some(location),
-            _ => None,
-        }
-    }
-    fn delimited_file_source(&self) -> Option<&DelimitedFileSource> {
-        match self {
-            Self::DelimitedFileSource(delimited_file_source) => Some(delimited_file_source),
-            _ => None,
-        }
-    }
-    fn embedded_source(&self) -> Option<&EmbeddedSource> {
-        match self {
-            Self::EmbeddedSource(embedded_source) => Some(embedded_source),
-            _ => None,
-        }
-    }
-    fn style_cycle(&self) -> Option<&StyleCycle> {
-        match self {
-            Self::StyleCycle(style_cycle) => Some(style_cycle),
-            _ => None,
-        }
-    }
-}
-
 #[derive(Deserialize, Debug)]
 #[serde(rename_all = "camelCase")]
 struct DelimitedFileSource {
@@ -2993,15 +3035,6 @@ impl RawGraph {
     }
 }
 
-impl Locations for RawGraph {
-    fn locations(&self) -> Vec<&Location> {
-        self.children
-            .iter()
-            .filter_map(|child| child.location())
-            .collect()
-    }
-}
-
 #[derive(Deserialize, Debug)]
 #[serde(rename_all = "camelCase")]
 enum GraphChild {
-- 
2.47.3