From 281845e33a23d7a773d520f72a18d1a959d1f86c Mon Sep 17 00:00:00 2001
From: Ben Pfaff <blp@cs.stanford.edu>
Date: Wed, 19 Aug 2026 15:03:13 -0700
Subject: [PATCH] modules cleanup

---
 rust/pspp/src/spv/read/graph.rs        | 277 ++-----------------------
 rust/pspp/src/spv/read/graph/frames.rs | 224 +++++++++++++++++++-
 2 files changed, 235 insertions(+), 266 deletions(-)

diff --git a/rust/pspp/src/spv/read/graph.rs b/rust/pspp/src/spv/read/graph.rs
index 4869e1a599..b5ae47dead 100644
--- a/rust/pspp/src/spv/read/graph.rs
+++ b/rust/pspp/src/spv/read/graph.rs
@@ -63,7 +63,7 @@ use crate::{
         legacy_bin::{DataSeries, DataSource, DataSources, LegacyBin, LegacyBinWarning},
         read::graph::{
             format::Formatter,
-            frames::{Component, Frames, Position},
+            frames::{Component, FrameType, Frames, Position},
             patterns::Patterns,
             style::{Style, StyleCycle},
         },
@@ -148,7 +148,7 @@ pub struct Visualization {
     embedded_sources: Vec<EmbeddedSource>,
     categorical_domains: Vec<CategoricalDomain>,
     interval_domains: Vec<IntervalDomain>,
-    containers: Vec<AnyContainer>,
+    children: Vec<AnyFrame>,
     styles: Vec<Style>,
     style_cycles: Vec<StyleCycle>,
     locations: Vec<Location>,
@@ -173,7 +173,7 @@ impl<'de> Deserialize<'de> for Visualization {
         let mut embedded_sources = Vec::new();
         let mut categorical_domains = Vec::new();
         let mut interval_domains = Vec::new();
-        let mut containers = Vec::new();
+        let mut frames = Vec::new();
         let mut styles = Vec::new();
         let mut style_cycles = Vec::new();
         let mut locations = Vec::new();
@@ -193,13 +193,9 @@ impl<'de> Deserialize<'de> for Visualization {
                     categorical_domains.push(categorical_domain)
                 }
                 VisChild::IntervalDomain(interval_domain) => interval_domains.push(interval_domain),
-                VisChild::Graph(graph) => containers.push(AnyContainer::Graph(graph)),
-                VisChild::LabelFrame(label_frame) => {
-                    containers.push(AnyContainer::LabelFrame(label_frame))
-                }
-                VisChild::Container(container) => {
-                    containers.push(AnyContainer::Container(container))
-                }
+                VisChild::Graph(graph) => frames.push(AnyFrame::Graph(graph)),
+                VisChild::LabelFrame(label_frame) => frames.push(AnyFrame::LabelFrame(label_frame)),
+                VisChild::Container(container) => frames.push(AnyFrame::Container(container)),
                 VisChild::StyleCycle(style_cycle) => style_cycles.push(style_cycle),
                 VisChild::Style(style) => styles.push(style),
                 VisChild::Location(location) => locations.push(location),
@@ -218,7 +214,7 @@ impl<'de> Deserialize<'de> for Visualization {
             embedded_sources,
             categorical_domains,
             interval_domains,
-            containers,
+            children: frames,
             styles,
             style_cycles,
             locations,
@@ -252,12 +248,6 @@ pub struct RawVisualization {
     children: Vec<VisChild>,
 }
 
-impl Locations for Visualization {
-    fn locations(&self) -> Vec<&Location> {
-        self.locations.iter().collect()
-    }
-}
-
 impl Visualization {
     fn all_variables(&self) -> impl Iterator<Item = &dyn Variable> {
         let source_variables = self
@@ -324,140 +314,6 @@ impl Visualization {
         domains
     }
 
-    fn decode_frames<'a>(&'a self) -> Frames<'a> {
-        let mut frames = Vec::new();
-        let mut ids = HashMap::new();
-        frames.push(Frame {
-            id: self.id.clone(),
-            frame_type: FrameType::Visualization(self),
-            parent: None,
-            children: Vec::new(),
-            locations: EnumMap::default(),
-            position: EnumMap::default(),
-        });
-        fn decode_container<'a>(
-            parent: usize,
-            container: &'a Container,
-            frames: &mut Vec<Frame<'a>>,
-            ids: &mut HashMap<&'a String, usize>,
-        ) {
-            let container_index = frames.len();
-            frames[parent].children.push(container_index);
-            if let Some(id) = &container.id {
-                ids.insert(id, container_index);
-            }
-            frames.push(Frame {
-                id: container.id.clone(),
-                frame_type: FrameType::Container(container),
-                parent: Some(parent),
-                children: Vec::new(),
-                locations: EnumMap::default(),
-                position: EnumMap::default(),
-            });
-            for child in &container.children {
-                match child {
-                    AnyContainer::Graph(graph) => {
-                        let index = frames.len();
-                        frames[container_index].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(container_index),
-                            children: Vec::new(),
-                            locations: EnumMap::default(),
-                            position: EnumMap::default(),
-                        });
-                    }
-                    AnyContainer::LabelFrame(label_frame) => {
-                        let index = frames.len();
-                        frames[container_index].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(container_index),
-                            children: Vec::new(),
-                            locations: EnumMap::default(),
-                            position: EnumMap::default(),
-                        });
-                    }
-                    AnyContainer::Legend(legend) => {
-                        let index = frames.len();
-                        frames[container_index].children.push(index);
-                        if let Some(id) = &legend.id {
-                            ids.insert(id, index);
-                        }
-                        frames.push(Frame {
-                            id: legend.id.clone(),
-                            frame_type: FrameType::Legend(legend),
-                            parent: Some(container_index),
-                            children: Vec::new(),
-                            locations: EnumMap::default(),
-                            position: EnumMap::default(),
-                        });
-                    }
-                    AnyContainer::Container(container) => {
-                        decode_container(container_index, container, frames, ids)
-                    }
-                }
-            }
-        }
-
-        for container in &self.containers {
-            match container {
-                AnyContainer::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(),
-                    });
-                }
-                AnyContainer::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(),
-                    });
-                }
-                AnyContainer::Container(container) => {
-                    decode_container(0, container, &mut frames, &mut ids)
-                }
-                AnyContainer::Legend(_) => todo!(),
-            }
-        }
-
-        for frame in &mut frames {
-            for location in frame.frame_type.locations() {
-                frame.locations[location.axis()][location.component()] = location.decode(&ids);
-            }
-        }
-
-        let mut frames = Frames::new(frames);
-        frames.set_fixed_size(enum_map! {Axis2::X => 400.0, Axis2::Y => 300.0});
-        frames
-    }
-
     fn decode_styles(&self) -> HashMap<String, Style> {
         fn inner(mut style: Style, styles: &mut HashMap<String, Style>) {
             for child in take(&mut style.children) {
@@ -1231,7 +1087,7 @@ impl Visualization {
 }
 
 #[derive(Debug)]
-enum AnyContainer {
+enum AnyFrame {
     Container(Container),
     Graph(Graph),
     Legend(Legend),
@@ -4292,17 +4148,11 @@ impl LabelFrame {
     }
 }
 
-impl Locations for LabelFrame {
-    fn locations(&self) -> Vec<&Location> {
-        self.locations.iter().collect()
-    }
-}
-
 #[derive(Debug)]
 struct Container {
     id: Option<String>,
     locations: Vec<Location>,
-    children: Vec<AnyContainer>,
+    children: Vec<AnyFrame>,
 }
 
 impl<'de> Deserialize<'de> for Container {
@@ -4317,12 +4167,12 @@ impl<'de> Deserialize<'de> for Container {
             match child {
                 ContainerChild::Location(location) => locations.push(location),
                 ContainerChild::Container(container) => {
-                    children.push(AnyContainer::Container(container))
+                    children.push(AnyFrame::Container(container))
                 }
-                ContainerChild::Graph(graph) => children.push(AnyContainer::Graph(graph)),
-                ContainerChild::Legend(legend) => children.push(AnyContainer::Legend(legend)),
+                ContainerChild::Graph(graph) => children.push(AnyFrame::Graph(graph)),
+                ContainerChild::Legend(legend) => children.push(AnyFrame::Legend(legend)),
                 ContainerChild::LabelFrame(label_frame) => {
-                    children.push(AnyContainer::LabelFrame(label_frame))
+                    children.push(AnyFrame::LabelFrame(label_frame))
                 }
             }
         }
@@ -4344,15 +4194,6 @@ struct RawContainer {
     children: Vec<ContainerChild>,
 }
 
-impl Locations for RawContainer {
-    fn locations(&self) -> Vec<&Location> {
-        self.children
-            .iter()
-            .filter_map(|child| child.location())
-            .collect()
-    }
-}
-
 #[derive(Deserialize, Debug)]
 #[serde(rename_all = "camelCase")]
 enum ContainerChild {
@@ -4382,96 +4223,6 @@ struct Legend {
     locations: Vec<Location>,
 }
 
-impl Locations for Legend {
-    fn locations(&self) -> Vec<&Location> {
-        self.locations.iter().collect()
-    }
-}
-
-trait Locations {
-    fn locations(&self) -> Vec<&Location>;
-}
-
-#[derive(Copy, Clone, Debug)]
-enum FrameType<'a> {
-    Visualization(&'a Visualization),
-    Container(&'a Container),
-    Graph(&'a Graph),
-    LabelFrame(&'a LabelFrame),
-    Legend(&'a Legend),
-}
-
-impl<'a> FrameType<'a> {
-    fn preferred_size(&self, max: &EnumMap<Axis2, Option<f64>>) -> EnumMap<Axis2, f64> {
-        let default = match self {
-            FrameType::Visualization(_) | FrameType::Container(_) => EnumMap::default(),
-            FrameType::Graph(_) => {
-                enum_map! { Axis2::X => 400.0, Axis2::Y => 300.0 }
-            }
-            FrameType::LabelFrame(label_frame) => {
-                let surface = RecordingSurface::create(cairo::Content::ColorAlpha, None).unwrap();
-                let context = Context::new(&surface).unwrap();
-                let pangocairo_context = pangocairo::functions::create_context(&context);
-                pangocairo::functions::context_set_resolution(&pangocairo_context, 96.0);
-                let layout = pango::Layout::new(&pangocairo_context);
-                let text = label_frame.text().unwrap_or_default();
-                layout.set_text(&text);
-                let (w, h) = layout.size();
-                enum_map! { Axis2::X => w as f64 / 1024.0, Axis2::Y => h as f64 / 1024.0}
-            }
-            FrameType::Legend(_) => enum_map! { Axis2::X => 100.0, Axis2::Y => 200.0 },
-        };
-        zip(default, max.into_values())
-            .map(|((axis, default), max)| {
-                (
-                    axis,
-                    if let Some(max) = max
-                        && default > max
-                    {
-                        max
-                    } else {
-                        default
-                    },
-                )
-            })
-            .collect()
-    }
-
-    fn can_contain_children(&self) -> bool {
-        match self {
-            FrameType::Visualization(_) | FrameType::Container(_) => true,
-            FrameType::Graph(_) | FrameType::LabelFrame(_) | FrameType::Legend(_) => false,
-        }
-    }
-
-    fn locations(&self) -> &[Location] {
-        match self {
-            FrameType::Visualization(visualization) => &visualization.locations,
-            FrameType::Container(container) => &container.locations,
-            FrameType::Graph(graph) => &graph.locations,
-            FrameType::LabelFrame(label_frame) => &label_frame.locations,
-            FrameType::Legend(legend) => &legend.locations,
-        }
-    }
-
-    fn weight(&self) -> f64 {
-        match self {
-            FrameType::Visualization(_) => 1.0,
-            FrameType::Container(_) => 1.0,
-            FrameType::Graph(_) => 100.0,
-            FrameType::LabelFrame(_) => 4.0,
-            FrameType::Legend(_) => 10.0,
-        }
-    }
-
-    fn as_label_frame(&self) -> Option<&LabelFrame> {
-        match self {
-            FrameType::LabelFrame(label_frame) => Some(label_frame),
-            _ => None,
-        }
-    }
-}
-
 #[derive(Copy, Clone, Debug, PartialEq)]
 struct LocationLengthRange {
     base: LocationLength,
@@ -4560,7 +4311,7 @@ impl<'de> Deserialize<'de> for LocationLength {
 }
 
 #[derive(Debug)]
-struct Frame<'a> {
+pub struct Frame<'a> {
     id: Option<String>,
     frame_type: FrameType<'a>,
     parent: Option<usize>,
diff --git a/rust/pspp/src/spv/read/graph/frames.rs b/rust/pspp/src/spv/read/graph/frames.rs
index 0f47ef5682..f571479a70 100644
--- a/rust/pspp/src/spv/read/graph/frames.rs
+++ b/rust/pspp/src/spv/read/graph/frames.rs
@@ -1,5 +1,6 @@
-use std::{fmt::Debug, ops::Range};
+use std::{collections::HashMap, fmt::Debug, iter::zip, ops::Range};
 
+use cairo::{Context, RecordingSurface};
 use enum_iterator::all;
 use enum_map::{Enum, EnumMap, enum_map};
 use svg::{
@@ -10,11 +11,228 @@ use svg::{
 use crate::{
     output::pivot::Axis2,
     spv::read::graph::{
-        Frame, FrameType, Graph, Location2, LocationLength, Span,
+        AnyFrame, Container, Frame, Graph, LabelFrame, Legend, Location, Location2, LocationLength,
+        Span, Visualization,
         frames::simplex::{Point, minimize},
     },
 };
 
+impl Visualization {
+    pub fn decode_frames<'a>(&'a self) -> Frames<'a> {
+        let mut frames = Vec::new();
+        let mut ids = HashMap::new();
+        frames.push(Frame {
+            id: self.id.clone(),
+            frame_type: FrameType::Visualization(self),
+            parent: None,
+            children: Vec::new(),
+            locations: EnumMap::default(),
+            position: EnumMap::default(),
+        });
+        fn decode_container<'a>(
+            parent: usize,
+            container: &'a Container,
+            frames: &mut Vec<Frame<'a>>,
+            ids: &mut HashMap<&'a String, usize>,
+        ) {
+            let container_index = frames.len();
+            frames[parent].children.push(container_index);
+            if let Some(id) = &container.id {
+                ids.insert(id, container_index);
+            }
+            frames.push(Frame {
+                id: container.id.clone(),
+                frame_type: FrameType::Container(container),
+                parent: Some(parent),
+                children: Vec::new(),
+                locations: EnumMap::default(),
+                position: EnumMap::default(),
+            });
+            for child in &container.children {
+                match child {
+                    AnyFrame::Graph(graph) => {
+                        let index = frames.len();
+                        frames[container_index].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(container_index),
+                            children: Vec::new(),
+                            locations: EnumMap::default(),
+                            position: EnumMap::default(),
+                        });
+                    }
+                    AnyFrame::LabelFrame(label_frame) => {
+                        let index = frames.len();
+                        frames[container_index].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(container_index),
+                            children: Vec::new(),
+                            locations: EnumMap::default(),
+                            position: EnumMap::default(),
+                        });
+                    }
+                    AnyFrame::Legend(legend) => {
+                        let index = frames.len();
+                        frames[container_index].children.push(index);
+                        if let Some(id) = &legend.id {
+                            ids.insert(id, index);
+                        }
+                        frames.push(Frame {
+                            id: legend.id.clone(),
+                            frame_type: FrameType::Legend(legend),
+                            parent: Some(container_index),
+                            children: Vec::new(),
+                            locations: EnumMap::default(),
+                            position: EnumMap::default(),
+                        });
+                    }
+                    AnyFrame::Container(container) => {
+                        decode_container(container_index, container, frames, ids)
+                    }
+                }
+            }
+        }
+
+        for container in &self.children {
+            match container {
+                AnyFrame::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(),
+                    });
+                }
+                AnyFrame::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(),
+                    });
+                }
+                AnyFrame::Container(container) => {
+                    decode_container(0, container, &mut frames, &mut ids)
+                }
+                AnyFrame::Legend(_) => todo!(),
+            }
+        }
+
+        for frame in &mut frames {
+            for location in frame.frame_type.locations() {
+                frame.locations[location.axis()][location.component()] = location.decode(&ids);
+            }
+        }
+
+        let mut frames = Frames::new(frames);
+        frames.set_fixed_size(enum_map! {Axis2::X => 400.0, Axis2::Y => 300.0});
+        frames
+    }
+}
+
+#[derive(Copy, Clone, Debug)]
+pub enum FrameType<'a> {
+    Visualization(&'a Visualization),
+    Container(&'a Container),
+    Graph(&'a Graph),
+    LabelFrame(&'a LabelFrame),
+    Legend(&'a Legend),
+}
+
+impl<'a> FrameType<'a> {
+    fn preferred_size(&self, max: &EnumMap<Axis2, Option<f64>>) -> EnumMap<Axis2, f64> {
+        let default = match self {
+            FrameType::Visualization(_) | FrameType::Container(_) => EnumMap::default(),
+            FrameType::Graph(_) => {
+                enum_map! { Axis2::X => 400.0, Axis2::Y => 300.0 }
+            }
+            FrameType::LabelFrame(label_frame) => {
+                let surface = RecordingSurface::create(cairo::Content::ColorAlpha, None).unwrap();
+                let context = Context::new(&surface).unwrap();
+                let pangocairo_context = pangocairo::functions::create_context(&context);
+                pangocairo::functions::context_set_resolution(&pangocairo_context, 96.0);
+                let layout = pango::Layout::new(&pangocairo_context);
+                let text = label_frame.text().unwrap_or_default();
+                layout.set_text(&text);
+                let (w, h) = layout.size();
+                enum_map! { Axis2::X => w as f64 / 1024.0, Axis2::Y => h as f64 / 1024.0}
+            }
+            FrameType::Legend(_) => enum_map! { Axis2::X => 100.0, Axis2::Y => 200.0 },
+        };
+        zip(default, max.into_values())
+            .map(|((axis, default), max)| {
+                (
+                    axis,
+                    if let Some(max) = max
+                        && default > max
+                    {
+                        max
+                    } else {
+                        default
+                    },
+                )
+            })
+            .collect()
+    }
+
+    fn can_contain_children(&self) -> bool {
+        match self {
+            FrameType::Visualization(_) | FrameType::Container(_) => true,
+            FrameType::Graph(_) | FrameType::LabelFrame(_) | FrameType::Legend(_) => false,
+        }
+    }
+
+    pub fn locations(&self) -> &[Location] {
+        match self {
+            FrameType::Visualization(visualization) => &visualization.locations,
+            FrameType::Container(container) => &container.locations,
+            FrameType::Graph(graph) => &graph.locations,
+            FrameType::LabelFrame(label_frame) => &label_frame.locations,
+            FrameType::Legend(legend) => &legend.locations,
+        }
+    }
+
+    pub fn weight(&self) -> f64 {
+        match self {
+            FrameType::Visualization(_) => 1.0,
+            FrameType::Container(_) => 1.0,
+            FrameType::Graph(_) => 100.0,
+            FrameType::LabelFrame(_) => 4.0,
+            FrameType::Legend(_) => 10.0,
+        }
+    }
+
+    pub fn as_label_frame(&self) -> Option<&LabelFrame> {
+        match self {
+            FrameType::LabelFrame(label_frame) => Some(label_frame),
+            _ => None,
+        }
+    }
+}
+
 pub struct Frames<'a> {
     pub frames: Vec<Frame<'a>>,
 }
@@ -379,7 +597,7 @@ impl<'a> Frames<'a> {
         todo!()
     }
 
-    pub fn graphs(&self) -> impl Iterator<Item = (&Frame<'_>, &Graph)> {
+    pub(super) fn graphs(&self) -> impl Iterator<Item = (&Frame<'_>, &Graph)> {
         self.frames
             .iter()
             .filter_map(|frame| match &frame.frame_type {
-- 
2.47.3