From 59ba72f0e09d23c5bc6eff4cf76b28966d036ce7 Mon Sep 17 00:00:00 2001 From: Jeffrey Wilcke Date: Mon, 12 Oct 2026 18:23:15 +0200 Subject: [PATCH] fix: review findings - screen transitions snapshot the previous frame (ratatui swaps buffers after draw, so the old snapshot was always blank); frame loop extracted into a testable Runner - .city loading rejects structurally inconsistent files: Grid deserialize checks size vs data, load_city checks header vs body and runs City::validate (now also checks Net link endpoints) - saving from the map hands the live map screen back instead of a fresh explore screen - signage glyphs covered by the unicode-width test Co-Authored-By: Claude Fable 5.1 --- crates/city/src/app.rs | 136 +++++++++++++++++++++++------ crates/city/src/save.rs | 65 +++++++++++++- crates/city/src/screens/explore.rs | 24 +++-- crates/city/src/screens/files.rs | 45 ++++++++-- crates/citygen/src/geom.rs | 49 ++++++++++- crates/citygen/src/validate.rs | 8 ++ 6 files changed, 286 insertions(+), 41 deletions(-) diff --git a/crates/city/src/app.rs b/crates/city/src/app.rs index 7a92fd5..f8ec186 100644 --- a/crates/city/src/app.rs +++ b/crates/city/src/app.rs @@ -6,6 +6,9 @@ use anyhow::Result; use citygen::{City, GenParams}; use crossterm::event::{self, Event, KeyEvent, KeyEventKind}; use ratatui::Frame; +use ratatui::Terminal; +use ratatui::backend::Backend; +use ratatui::buffer::Buffer; use ratatui::layout::{Alignment, Rect}; use ratatui::style::{Color, Style}; use ratatui::widgets::Paragraph; @@ -90,11 +93,78 @@ pub fn draw_too_small(f: &mut Frame) { ); } -/// The fixed-rate loop: poll input, tick, update, draw. -pub fn run(mut screen: Box, mut st: AppState) -> Result<()> { +/// One screen plus the state the loop threads through frames. +pub struct Runner { + screen: Box, + pub st: AppState, + transition: Option, + /// Our own copy of the last drawn frame; ratatui swaps buffers after every draw. + last_frame: Option, +} + +impl Runner { + pub fn new(screen: Box, st: AppState) -> Self { + Runner { + screen, + st, + transition: None, + last_frame: None, + } + } + + #[cfg(test)] + pub fn transition(&self) -> Option<&Transition> { + self.transition.as_ref() + } + + /// Runs one frame: tick, update with `keys`, draw. Returns false when the app quits. + pub fn frame(&mut self, term: &mut Terminal, keys: &[KeyEvent]) -> Result { + self.st.tick += 1; + if self + .transition + .as_ref() + .is_some_and(|t| t.active(self.st.tick)) + && !keys.is_empty() + { + self.transition = None; + } + match self.screen.update(&mut self.st, keys) { + ScreenCmd::Stay => {} + ScreenCmd::Switch { to, fx } => { + let snapshot = self + .last_frame + .clone() + .unwrap_or_else(|| Buffer::empty(term.get_frame().area())); + self.screen = to; + self.transition = Some(Transition::new(fx, snapshot, self.st.tick)); + } + ScreenCmd::Quit => return Ok(false), + } + let st = &self.st; + let screen = &self.screen; + let transition = &self.transition; + let mut captured: Option = None; + term.draw(|f| { + if f.area().width < MIN_W || f.area().height < MIN_H { + draw_too_small(f); + } else { + screen.draw(st, f); + if let Some(t) = transition { + t.apply(f, st.tick); + } + } + captured = Some(f.buffer_mut().clone()); + })?; + self.last_frame = captured; + Ok(true) + } +} + +/// The fixed-rate loop: poll input, run a frame, sleep the remainder. +pub fn run(screen: Box, st: AppState) -> Result<()> { install_panic_hook(); let (_guard, mut term) = TerminalGuard::enter()?; - let mut transition: Option = None; + let mut runner = Runner::new(screen, st); loop { let start = Instant::now(); let mut keys = Vec::new(); @@ -105,29 +175,9 @@ pub fn run(mut screen: Box, mut st: AppState) -> Result<()> { keys.push(k); } } - st.tick += 1; - if transition.as_ref().is_some_and(|t| t.active(st.tick)) && !keys.is_empty() { - transition = None; + if !runner.frame(&mut term, &keys)? { + break; } - match screen.update(&mut st, &keys) { - ScreenCmd::Stay => {} - ScreenCmd::Switch { to, fx } => { - let snapshot = term.current_buffer_mut().clone(); - screen = to; - transition = Some(Transition::new(fx, snapshot, st.tick)); - } - ScreenCmd::Quit => break, - } - term.draw(|f| { - if f.area().width < MIN_W || f.area().height < MIN_H { - draw_too_small(f); - return; - } - screen.draw(&st, f); - if let Some(t) = &transition { - t.apply(f, st.tick); - } - })?; if let Some(rest) = FRAME.checked_sub(start.elapsed()) { std::thread::sleep(rest); } @@ -164,6 +214,42 @@ mod tests { assert!(text(&t).contains("100")); } + struct Says(&'static str, bool); + + impl Screen for Says { + fn update(&mut self, _st: &mut AppState, keys: &[KeyEvent]) -> ScreenCmd { + if self.1 && !keys.is_empty() { + return ScreenCmd::Switch { + to: Box::new(Says("WORLD", false)), + fx: TransitionKind::SlideUp, + }; + } + ScreenCmd::Stay + } + fn draw(&self, _st: &AppState, f: &mut Frame) { + f.render_widget(ratatui::widgets::Paragraph::new(self.0), f.area()); + } + } + + #[test] + fn switch_snapshots_the_previous_frame() { + let mut t = Terminal::new(TestBackend::new(100, 30)).unwrap(); + let mut runner = Runner::new(Box::new(Says("HELLO", true)), AppState::new()); + assert!(runner.frame(&mut t, &[]).unwrap()); + assert!(text(&t).contains("HELLO")); + let key = KeyEvent::new( + crossterm::event::KeyCode::Enter, + crossterm::event::KeyModifiers::NONE, + ); + assert!(runner.frame(&mut t, &[key]).unwrap()); + let tr = runner.transition().expect("a transition is running"); + let snapshot: String = (0..5) + .map(|x| tr.from[(x, 0)].symbol().to_string()) + .collect(); + assert_eq!(snapshot, "HELLO"); + assert!(text(&t).contains("WORLD") || text(&t).contains("HELLO")); + } + #[test] fn app_state_status_expires() { let mut st = AppState::new(); diff --git a/crates/city/src/save.rs b/crates/city/src/save.rs index 9743bd4..aa196d1 100644 --- a/crates/city/src/save.rs +++ b/crates/city/src/save.rs @@ -81,9 +81,36 @@ fn split<'a>(bytes: &'a [u8], path: &Path) -> Result<(Header, &'a [u8])> { pub fn load_city(path: &Path) -> Result { let bytes = std::fs::read(path).with_context(|| format!("reading {}", path.display()))?; - let (_, body) = split(&bytes, path)?; + let (header, body) = split(&bytes, path)?; let city: City = postcard::from_bytes(body) .with_context(|| format!("{}: corrupt or truncated city data", path.display()))?; + if (header.w, header.h) != (city.size.w, city.size.h) { + bail!( + "{}: header says {}x{} but the city is {}x{}", + path.display(), + header.w, + header.h, + city.size.w, + city.size.h + ); + } + let strata_ok = city.strata.len() == citygen::StratumKind::PHYSICAL.len() + && city + .strata + .iter() + .all(|s| s.tiles.size() == city.size && s.light.size() == city.size); + if !strata_ok { + bail!("{}: strata do not match the city size", path.display()); + } + if let Err(violations) = city.validate() { + let first: Vec = violations.iter().take(3).map(|v| v.to_string()).collect(); + bail!( + "{}: city violates its invariants ({} problems, e.g. {})", + path.display(), + violations.len(), + first.join("; ") + ); + } Ok(city) } @@ -166,6 +193,42 @@ mod tests { assert!(list_cities(&dir.join("missing")).unwrap().is_empty()); } + #[test] + fn rejects_structurally_inconsistent_cities() { + let dir = tmp("inconsistent"); + let p = dir.join("x.city"); + // Header disagrees with the body. + let c = tiny(); + save_city(&p, &c).unwrap(); + let mut bytes = std::fs::read(&p).unwrap(); + let header_len = u32::from_le_bytes([bytes[6], bytes[7], bytes[8], bytes[9]]) as usize; + let mut header: Header = postcard::from_bytes(&bytes[10..10 + header_len]).unwrap(); + header.w = 96; + let hb = postcard::to_allocvec(&header).unwrap(); + let body = bytes.split_off(10 + header_len); + bytes.truncate(6); + bytes.extend_from_slice(&(hb.len() as u32).to_le_bytes()); + bytes.extend_from_slice(&hb); + bytes.extend_from_slice(&body); + std::fs::write(&p, &bytes).unwrap(); + let err = load_city(&p).unwrap_err().to_string(); + assert!(err.contains("header"), "{err}"); + // Body violates the city invariants (tiles reference districts that do not exist). + let mut broken = tiny(); + broken.districts.clear(); + save_city(&p, &broken).unwrap(); + let err = load_city(&p).unwrap_err().to_string(); + assert!( + err.contains("invariant") || err.contains("missing"), + "{err}" + ); + // A grid whose data length disagrees with its size never gets past deserialization. + let mut odd = tiny(); + odd.strata.pop(); + save_city(&p, &odd).unwrap(); + assert!(load_city(&p).is_err()); + } + #[test] fn rejects_bad_magic_version_and_truncation() { let dir = tmp("bad"); diff --git a/crates/city/src/screens/explore.rs b/crates/city/src/screens/explore.rs index 69b3139..1c973ce 100644 --- a/crates/city/src/screens/explore.rs +++ b/crates/city/src/screens/explore.rs @@ -164,8 +164,10 @@ impl MapScreen { KeyCode::Char('?') => self.help = !self.help, KeyCode::Char('s') => { let name = st.city.as_ref().map(|c| c.name.clone())?; + // The save prompt hands this very screen back, state intact. + let live = std::mem::replace(self, MapScreen::explore(st.tick)); return Some(ScreenCmd::Switch { - to: Box::new(Files::save(st.tick, &name)), + to: Box::new(Files::save(st.tick, &name, Some(Box::new(live)))), fx: TransitionKind::None, }); } @@ -510,12 +512,24 @@ mod tests { assert!(buffer_text(t.backend().buffer()).contains("move")); s.update(&mut st, &[key(KeyCode::Esc)]); assert!(!s.help); + // Saving while walking must bring the walk back, not a fresh explore screen. + s.stratum = StratumKind::Undercity; + let ScreenCmd::Switch { to: mut files, .. } = s.update(&mut st, &[key(KeyCode::Char('s'))]) + else { + panic!("s should open the save screen"); + }; + let ScreenCmd::Switch { to: back, .. } = files.update(&mut st, &[key(KeyCode::Esc)]) else { + panic!("esc should return to the map"); + }; + st.tick += 500; // let transient statuses expire so the mode hints show + t.draw(|f| back.draw(&st, f)).unwrap(); + let text = buffer_text(t.backend().buffer()); + assert!( + text.contains("tab camera") && text.contains("Undercity"), + "{text}" + ); s.update(&mut st, &[key(KeyCode::Tab)]); assert_eq!(s.mode, Mode::Explore); - assert!(matches!( - s.update(&mut st, &[key(KeyCode::Char('s'))]), - ScreenCmd::Switch { .. } - )); assert!(matches!( s.update(&mut st, &[key(KeyCode::Char('q'))]), ScreenCmd::Switch { .. } diff --git a/crates/city/src/screens/files.rs b/crates/city/src/screens/files.rs index 80a44b6..1d030bc 100644 --- a/crates/city/src/screens/files.rs +++ b/crates/city/src/screens/files.rs @@ -26,6 +26,8 @@ pub struct Files { entries: Vec, selected: usize, error: Option, + /// The screen to return to after saving; a fresh explore screen when absent. + back: Option>, } /// `YYYY-MM-DD HH:MM` in UTC, without a date crate (civil-from-days algorithm). @@ -58,8 +60,8 @@ impl Files { Self::load_in(default_dir(), _tick) } - pub fn save(tick: u64, name: &str) -> Self { - Self::save_in(default_dir(), tick, name) + pub fn save(tick: u64, name: &str, back: Option>) -> Self { + Self::save_in(default_dir(), tick, name, back) } pub fn load_in(dir: PathBuf, _tick: u64) -> Self { @@ -70,10 +72,11 @@ impl Files { entries, selected: 0, error: None, + back: None, } } - pub fn save_in(dir: PathBuf, _tick: u64, name: &str) -> Self { + pub fn save_in(dir: PathBuf, _tick: u64, name: &str, back: Option>) -> Self { let form = Form { fields: vec![Field::Text { label: "Name".into(), @@ -89,12 +92,17 @@ impl Files { entries: Vec::new(), selected: 0, error: None, + back, } } - fn back_to_map(st: &AppState) -> ScreenCmd { + fn back_to_map(&mut self, st: &AppState) -> ScreenCmd { + let to = self + .back + .take() + .unwrap_or_else(|| crate::screens::explore_screen(st.tick)); ScreenCmd::Switch { - to: crate::screens::explore_screen(st.tick), + to, fx: TransitionKind::None, } } @@ -141,7 +149,7 @@ impl Screen for Files { }, Mode::Save(form) => { if k.code == KeyCode::Esc { - return Self::back_to_map(st); + return self.back_to_map(st); } if k.code == KeyCode::Enter { let name = form.text("Name").unwrap_or("").trim().to_string(); @@ -151,13 +159,13 @@ impl Screen for Files { name }; let Some(city) = st.city.as_ref() else { - return Self::back_to_map(st); + return self.back_to_map(st); }; let path = self.dir.join(format!("{name}.city")); match save_city(&path, city) { Ok(()) => { st.set_status(format!("saved {}", path.display()), 120); - return Self::back_to_map(st); + return self.back_to_map(st); } Err(e) => self.error = Some(e.to_string()), } @@ -299,7 +307,7 @@ mod tests { .unwrap(), ); let name = st.city.as_ref().unwrap().name.clone(); - let mut save = Files::save_in(dir.clone(), 0, &name); + let mut save = Files::save_in(dir.clone(), 0, &name, None); let mut t = Terminal::new(TestBackend::new(100, 30)).unwrap(); t.draw(|f| save.draw(&st, f)).unwrap(); assert!(buffer_text(t.backend().buffer()).contains("SAVE")); @@ -319,6 +327,25 @@ mod tests { ScreenCmd::Switch { .. } )); assert_eq!(st.city.as_ref().map(|c| c.seed), Some(9)); + // Saving from a live map screen hands that same screen back, state intact. + let mut map = crate::screens::explore::MapScreen::explore(0); + map.mode = crate::screens::explore::Mode::Walk; + map.stratum = citygen::StratumKind::Undercity; + map.digital = true; + st.player = Some(crate::walker::Player::spawn(st.city.as_ref().unwrap(), 1)); + let mut save = Files::save_in(dir.clone(), 0, "again", Some(Box::new(map))); + let ScreenCmd::Switch { to, .. } = save.update(&mut st, &[key(KeyCode::Enter)]) else { + panic!("save should switch back"); + }; + st.tick += 500; // let the "saved" status expire so the mode hints show + let mut t3 = Terminal::new(TestBackend::new(120, 40)).unwrap(); + t3.draw(|f| to.draw(&st, f)).unwrap(); + let text = buffer_text(t3.backend().buffer()); + assert!( + text.contains("tab camera") && text.contains("Undercity"), + "{text}" + ); + let mut empty = Files::load_in(tmp("empty"), 0); t.draw(|f| empty.draw(&st, f)).unwrap(); assert!(buffer_text(t.backend().buffer()).contains("no saved cities")); diff --git a/crates/citygen/src/geom.rs b/crates/citygen/src/geom.rs index 6d2d222..abd528a 100644 --- a/crates/citygen/src/geom.rs +++ b/crates/citygen/src/geom.rs @@ -189,13 +189,42 @@ impl Dir { } } -/// Dense row-major grid of `T`. +/// Dense row-major grid of `T`. Deserialization rejects data that does not match `size`. #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(try_from = "RawGrid")] +#[serde(bound(deserialize = "T: Deserialize<'de>"))] pub struct Grid { size: Size, data: Vec, } +/// The on-disk shape of a grid, before the size/data consistency check. +#[derive(Deserialize)] +struct RawGrid { + size: Size, + data: Vec, +} + +impl TryFrom> for Grid { + type Error = String; + + fn try_from(raw: RawGrid) -> Result { + if raw.data.len() != raw.size.area() { + return Err(format!( + "grid of {}x{} needs {} cells, found {}", + raw.size.w, + raw.size.h, + raw.size.area(), + raw.data.len() + )); + } + Ok(Grid { + size: raw.size, + data: raw.data, + }) + } +} + impl Grid { pub fn filled(size: Size, v: T) -> Self { Self { @@ -308,6 +337,24 @@ mod tests { assert_eq!(g.count(|&v| v == 9), 4); } + #[test] + fn grid_rejects_inconsistent_serialized_data() { + #[derive(serde::Serialize)] + struct Fake { + size: Size, + data: Vec, + } + let bad = ron::to_string(&Fake { + size: Size { w: 4, h: 3 }, + data: vec![0; 5], + }) + .unwrap(); + assert!(ron::from_str::>(&bad).is_err()); + let good = ron::to_string(&Grid::filled(Size { w: 4, h: 3 }, 7u8)).unwrap(); + let g: Grid = ron::from_str(&good).unwrap(); + assert_eq!(g.count(|&v| v == 7), 12); + } + #[test] fn dir_delta_opposite() { assert_eq!(Dir::N.delta(), (0, -1)); diff --git a/crates/citygen/src/validate.rs b/crates/citygen/src/validate.rs index ad67991..8a27124 100644 --- a/crates/citygen/src/validate.rs +++ b/crates/citygen/src/validate.rs @@ -152,6 +152,14 @@ impl City { } } + for link in &self.net.links { + for id in [link.0, link.1] { + if id.index() >= self.net.nodes.len() { + out.push(Violation::NetAnchor(id)); + } + } + } + for d in &self.districts { let has_building = self.buildings.iter().any(|b| b.district == d.id); let has_public = self.connectors.iter().any(|c| {