aleleba/gridcraft
1
0
Fork 0
mirror of https://github.com/storytold/gridcraft.git synced 2026-10-30 05:37:00 -06:00
Code Issues Packages Projects Releases Wiki Activity

Engine: a dry run journals nothing, so the journal and Record Actions' start stay as they were

Browse Source 
Every nested command a dry run executed was journaled. Past the journal's limits that evicted
the person's own oldest entries, and the restore could only cut back what was left, so one
`macro.check` of a long macro could empty the journal and move a Record Actions recording's
start. While `Saved` holds the session's state, commands are not journaled
(`Session::journal_paused`), and the restore puts the journal and the recording start back
exactly; the count of dropped entries it relied on is gone. The fallible calls of a dry run come
before the pause, and a panic at the top level turns journaling back on.

- Test: `a_dry_run_leaves_the_journal_and_the_recording_start_as_they_were` (a full journal, a
  Record Actions start, and a check of 12,000 nested steps; replaces the test that accepted the
  loss).
This commit is contained in:
prxnvt
2026-10-25 06:02:18 -04:00
parent 891986bf66
commit 361fff982a
3 changed files with 35 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/engine/src/lib.rs
+8 -10
View File
@@ -445,9 +445,9 @@ pub struct Session {
/// Serialized size of each journal entry, measured once when it is pushed, and their sum.
journal_sizes: Vec<usize>,
journal_bytes: usize,
/// Entries the journal has dropped from its front to stay within its limits, ever; a dry run
/// that puts the journal back needs to know how many went while it ran.
journal_dropped: u64,
/// While a dry run runs (the footprint, `macro.check`), its commands are not journaled: the
/// dry run is put back, and journaling it would evict the person's own oldest entries.
pub(crate) journal_paused: u32,
pub ui_requests: Vec<UiRequest>,
/// Format painter source style (sticky when double-clicked).
pub format_painter: Option<(Arc<Workbook>, usize, RangeRef, bool)>,
@@ -572,16 +572,10 @@ impl Session {
self.journal.drain(..n);
self.journal_sizes.drain(..n);
self.journal_bytes = kept;
self.journal_dropped = self.journal_dropped.saturating_add(n as u64);
self.recording = self.recording.map(|r| r.saturating_sub(n));
}
}
/// Entries dropped from the journal's front so far, for a caller that puts the journal back.
pub(crate) fn journal_dropped(&self) -> u64 {
self.journal_dropped
}
/// Cuts the journal back to its first `keep` entries, with its size bookkeeping.
pub(crate) fn truncate_journal(&mut self, keep: usize) {
if self.journal_sizes.len() == self.journal.len() {
@@ -675,6 +669,8 @@ impl Session {
if self.depth == 0 {
self.trim_history();
self.group_step = None;
// A dry run the panic cut short never put journaling back on.
self.journal_paused = 0;
}
return Err(EngineError::Internal(id.to_string(), msg));
}
@@ -700,7 +696,9 @@ impl Session {
{
rec.push(record::RecordedStep { doc, sheet, sheet_name, selection, id: id.to_string(), params: cmd::journal_params(&params) });
}
self.push_journal(id, cmd::journal_params(&params));
if self.journal_paused == 0 {
self.push_journal(id, cmd::journal_params(&params));
}
}
// Inside an undo group the group owns the step. A command that ended a group open before
// it leaves the group its step and makes one of its own for what it changed after the end.
crates/engine/src/macros/run.rs
+13 -13
View File
@@ -1008,10 +1008,9 @@ struct Saved {
clipboard: Option<crate::Clipboard>,
prefs: crate::Prefs,
mode: crate::Mode,
/// The journal's length and how many entries it had dropped from its front; a dry run that
/// pushes past the journal's limits drops more, which a length alone cannot see.
/// The journal's length: the dry run journals nothing (`Session::journal_paused`), so nothing
/// of the person's is evicted, and the length puts back exactly what was there.
journal: usize,
journal_dropped: u64,
recording: Option<usize>,
ui_requests: usize,
format_painter: Option<(Arc<Workbook>, usize, RangeRef, bool)>,
@@ -1024,9 +1023,10 @@ struct Saved {
}
impl Saved {
/// Takes the state and stops journaling until [`Saved::restore`].
fn take(s: &mut Session, doc: u64) -> Saved {
let d = s.docs.iter().find(|d| d.uid == doc);
Saved {
let saved = Saved {
active: s.active,
docs: s.docs.iter().map(|d| d.uid).collect(),
saved: d.map(|d| d.saved.clone()),
@@ -1039,7 +1039,6 @@ impl Saved {
prefs: s.prefs.clone(),
mode: s.mode,
journal: s.journal.len(),
journal_dropped: s.journal_dropped(),
recording: s.recording,
ui_requests: s.ui_requests.len(),
format_painter: s.format_painter.clone(),
@@ -1049,7 +1048,9 @@ impl Saved {
draw: (s.draw_tool.clone(), s.draw_color.clone(), s.draw_width),
comments_pane: s.comments_pane,
show_all_notes: s.show_all_notes,
}
};
s.journal_paused = s.journal_paused.saturating_add(1);
saved
}
fn restore(self, s: &mut Session, doc: u64) {
@@ -1070,11 +1071,9 @@ impl Saved {
s.clipboard = self.clipboard;
s.prefs = self.prefs;
s.mode = self.mode;
// Entries the dry run pushed go; entries the journal dropped meanwhile are gone for good,
// and the recording's start moves back with them.
let dropped = usize::try_from(s.journal_dropped().saturating_sub(self.journal_dropped)).unwrap_or(usize::MAX);
s.truncate_journal(self.journal.saturating_sub(dropped));
s.recording = self.recording.map(|r| r.saturating_sub(dropped));
s.truncate_journal(self.journal);
s.recording = self.recording;
s.journal_paused = s.journal_paused.saturating_sub(1);
s.ui_requests.truncate(self.ui_requests);
s.format_painter = self.format_painter;
s.last_find = self.last_find;
@@ -1092,9 +1091,10 @@ impl Saved {
pub fn footprint(s: &mut Session, p: &Prepared, run: &Machine) -> Result<Footprint> {
let i = s.docs.iter().position(|d| d.uid == p.doc).ok_or(EngineError::NoDocument)?;
s.set_active(i);
let saved = Saved::take(s, p.doc);
let before = s.doc()?.wb.clone();
let token = s.group_begin("Check Macro")?;
// After the calls that can fail, so nothing returns between taking and putting back.
let saved = Saved::take(s, p.doc);
if let Some(d) = s.docs.iter_mut().find(|d| d.uid == p.doc) {
d.touched = Some(Vec::new());
}
@@ -1498,8 +1498,8 @@ fn conflict_rule(p: &Json, m: &Macro) -> Result<OnConflict> {
/// For `macro.check`, as the run would: shifts the data, checks the footprint again, and puts
/// everything back. What still stands in the way, or `None`.
fn shifted_conflicts(s: &mut Session, fp: &Footprint, p: &Prepared, budget: &Machine, down: bool) -> Result<Option<String>> {
let saved = Saved::take(s, p.doc);
let token = s.group_begin("Check Macro")?;
let saved = Saved::take(s, p.doc);
let mut spent = budget.clone();
(spent.executed, spent.param_bytes) = (fp.executed, fp.param_bytes);
let again = make_room(s, fp, p, down).and_then(|()| footprint(s, p, &spent));
crates/engine/src/tests/macros.rs
+14 -18
View File
@@ -418,33 +418,29 @@ fn a_dry_run_leaves_a_clean_workbook_clean() {
}
#[test]
fn a_dry_run_puts_the_journal_back_even_after_it_trimmed_it() {
// Every nested command a dry run executes is journaled, so a long macro pushes the journal
// past its limit and the journal drops its oldest entries. Putting the journal back by its
// old length would then keep entries from the dry run and point a recording at the wrong
// place; the restore counts what was dropped.
fn a_dry_run_leaves_the_journal_and_the_recording_start_as_they_were() {
// A dry run journals nothing: journaling its nested commands would push a full journal past
// its limit and evict the person's own oldest entries, and move Record Actions' start.
let mut s = s();
data(&mut s);
s.execute("selection.set", json!({"range": "A1"})).unwrap();
let journal = s.journal.clone();
s.recording = Some(journal.len().saturating_sub(1));
for i in 0..crate::JOURNAL_MAX_ENTRIES {
s.execute("selection.set", json!({"range": format!("A{}", i % 50 + 1)})).unwrap();
}
s.recording = Some(s.journal.len().saturating_sub(100));
put(
&mut s,
json!({"name": "Long", "steps": [{"type": "repeat", "over": "A1:A200", "body": [
{"type": "repeat", "over": "A1:A60", "body": [{"command": "cell.set", "params": {"cell": "Row.cell(1,1)", "input": "x"}}]}
]}]}),
);
s.execute("macro.check", json!({"name": "Long"})).unwrap();
let sets = |j: &[(String, Json)]| j.iter().filter(|(id, _)| id == "cell.set").count();
assert!(sets(&s.journal) <= sets(&journal), "no dry-run entry stays");
assert!(s.journal.len() <= journal.len(), "{} entries, had {}", s.journal.len(), journal.len());
assert!(journal.ends_with(&s.journal), "what stays is the journal's own tail");
assert!(s.recording.is_some_and(|r| r <= s.journal.len()), "{:?}", s.recording);
// A short dry run leaves the journal exactly as it was.
put(&mut s, json!({"name": "Short", "steps": [{"command": "cell.set", "params": {"cell": "H1", "input": "x"}}]}));
let (journal, recording) = (s.journal.clone(), s.recording);
s.execute("macro.check", json!({"name": "Short"})).unwrap();
assert_eq!((&s.journal, s.recording), (&journal, recording));
let c = s.execute("macro.check", json!({"name": "Long"})).unwrap();
assert!(c.get("footprint").is_some(), "{c}");
assert_eq!((s.journal.len(), s.recording), (journal.len(), recording));
assert!(s.journal == journal, "the journal is as it was");
// The check itself is a query and isn't journaled; a command after it is.
s.execute("selection.set", json!({"range": "B1"})).unwrap();
assert_eq!(s.journal.last().map(|(id, _)| id.as_str()), Some("selection.set"));
}
#[test]