Repository navigation
Conversation
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused implementation has behavioral coverage, and only minor test-naming feedback remains.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Adds UndoStage::discard_oldest(count) so editors can trim undo history at
command boundaries while preserving the current scene.
Changes:
- Flushes pending edits, removes up to
countoldest transactions, and returns the number removed. - Tests retained undo behavior, scene preservation, count limits, and pending direct edits.
| File | Description |
|---|---|
crates/openusd/src/usd/capture.rs |
Adds the history-trimming method, documentation, and tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// untouched, the newer transactions still undo, and the discarded edits | ||
| /// become permanent. | ||
| #[test] | ||
| fn discard_oldest_keeps_recent_transactions_and_scene() -> Result<()> { |
| /// `discard_oldest` drains a still-pending direct `layer_mut` edit into the | ||
| /// stack first, so that edit is the one discarded. | ||
| #[test] | ||
| fn discard_oldest_drains_pending_direct_edits() -> Result<()> { |
|
I think the underlying issue is that one logical undo step can span several transactions. What do you think of an API like this: undo.group(|stage| {
stage.define_prim("/World")?.set_type_name("Xform")?;
Ok(())
})?;
undo.undo()?; // Undoes both transactions.Ordinary actions need no wrapper. This lets the editor define undo boundaries directly, preserves immediate visibility of committed edits, and ensures undo and history eviction always operate on complete user actions. |

Another small one from the same batch as #139.
with_capacitydrops the oldest transaction once the stack is full, but in an editor one undo step is usually a few transactions (evendefine_prim+set_type_nameis two). So eviction can cut a command in half, and undoing it later only reverts part of it.This adds
discard_oldest(count)so the stage can stay unbounded and the editor trims the history itself, on its own command boundaries. It's basically a partialreset: it flushes pending edits first, drops up tocountof the oldest transactions, leaves the scene alone and returns how many it dropped.C++ doesn't have an undo stack at all (it just reports undoable edits to
SdfLayerStateDelegateand leaves history to the app), so this is only on the Rust side.