diff --git a/src/libslic3r/CadDocument.cpp b/src/libslic3r/CadDocument.cpp index 23eb598c45..bc591d60e6 100644 --- a/src/libslic3r/CadDocument.cpp +++ b/src/libslic3r/CadDocument.cpp @@ -1728,8 +1728,8 @@ void CadDocument::clear() void CadDocument::checkpoint() { - m_undo.push_back(features); // snapshot the pre-mutation recipe - m_redo.clear(); // any new action invalidates the redo branch + m_undo.push_back(Snapshot{features, variables}); // snapshot the pre-mutation recipe + m_redo.clear(); // any new action invalidates the redo branch if (m_undo.size() > k_undo_cap) m_undo.erase(m_undo.begin()); } @@ -1738,8 +1738,9 @@ bool CadDocument::undo() { if (m_undo.empty()) return false; - m_redo.push_back(std::move(features)); // current state becomes redoable - features = std::move(m_undo.back()); + m_redo.push_back(Snapshot{std::move(features), std::move(variables)}); // current state becomes redoable + features = std::move(m_undo.back().features); + variables = std::move(m_undo.back().variables); m_undo.pop_back(); recompute(); // benign-empty (only a sketch / empty doc) is a valid undo target return true; @@ -1749,8 +1750,9 @@ bool CadDocument::redo() { if (m_redo.empty()) return false; - m_undo.push_back(std::move(features)); - features = std::move(m_redo.back()); + m_undo.push_back(Snapshot{std::move(features), std::move(variables)}); + features = std::move(m_redo.back().features); + variables = std::move(m_redo.back().variables); m_redo.pop_back(); recompute(); return true; diff --git a/src/libslic3r/CadDocument.hpp b/src/libslic3r/CadDocument.hpp index 4419e9329a..f87813cf85 100644 --- a/src/libslic3r/CadDocument.hpp +++ b/src/libslic3r/CadDocument.hpp @@ -662,11 +662,21 @@ private: static DatumCoordSys datum_frame(const std::vector& bodies, const CadFeature& f); void apply_mate(std::vector& bodies, const CadFeature& f) const; - // Undo/redo stacks of feature-list snapshots. checkpoint() pushes onto m_undo and - // clears m_redo; undo()/redo() shuffle the current state between them. Capped so a - // long session can't grow unbounded. - std::vector> m_undo; - std::vector> m_redo; + // Undo/redo stacks of recipe snapshots. checkpoint() pushes onto m_undo and clears + // m_redo; undo()/redo() shuffle the current state between them. Capped so a long + // session can't grow unbounded. + // + // The snapshot MUST carry `variables` as well as `features`: a caller that sets a bad + // variable, sees recompute() fail and calls undo() to roll it back would otherwise be + // left with the bad variable still in the document, so every later recompute fails — + // the exact corruption the checkpoint/undo pattern exists to prevent. Not serialized, + // so this changes no on-disk format. + struct Snapshot { + std::vector features; + std::map variables; + }; + std::vector m_undo; + std::vector m_redo; static constexpr size_t k_undo_cap = 200; }; diff --git a/src/slic3r/GUI/DesignPanel.cpp b/src/slic3r/GUI/DesignPanel.cpp index d6b569ad3b..953eddc2fa 100644 --- a/src/slic3r/GUI/DesignPanel.cpp +++ b/src/slic3r/GUI/DesignPanel.cpp @@ -8792,7 +8792,9 @@ void DesignPanel::on_add_variable() m_doc.checkpoint(); m_doc.variables[name_str] = expr_str; bool ok = m_doc.recompute(); - if (!ok) { m_doc.undo(); } + // undo() recomputes, which SUCCEEDS and clears doc.error — so the reason the edit was + // rejected is gone before anything can display it. Carry it across the rollback. + if (!ok) { const std::string why = m_doc.error; m_doc.undo(); m_doc.error = why; } after_tree_edit(ok); refresh_variables(); } @@ -8817,7 +8819,9 @@ void DesignPanel::on_edit_variable() m_doc.checkpoint(); m_doc.variables[name_str] = expr_str; bool ok = m_doc.recompute(); - if (!ok) { m_doc.undo(); } + // undo() recomputes, which SUCCEEDS and clears doc.error — so the reason the edit was + // rejected is gone before anything can display it. Carry it across the rollback. + if (!ok) { const std::string why = m_doc.error; m_doc.undo(); m_doc.error = why; } after_tree_edit(ok); refresh_variables(); } @@ -8837,11 +8841,16 @@ void DesignPanel::on_remove_variable() m_doc.variables.erase(name_str); bool ok = m_doc.recompute(); if (!ok) { + // Capture before undo(): its recompute succeeds and clears doc.error. + const std::string why = m_doc.error; m_doc.undo(); - // A failed recompute on remove means a feature expr still references it + // Almost always a feature expr still referencing the variable — but say so as the + // likely cause and keep the real message, rather than asserting a diagnosis that + // would be wrong for any other failure. m_doc.error = wxString::Format( - _L("Variable '%s' is referenced by a feature expression and cannot be removed"), - m_var_list->GetItemText(sel, 0)).ToUTF8().data(); + _L("Variable '%s' could not be removed (likely still referenced by a feature " + "expression): %s"), + m_var_list->GetItemText(sel, 0), wxString::FromUTF8(why)).ToUTF8().data(); } after_tree_edit(ok); refresh_variables(); @@ -8916,7 +8925,9 @@ void DesignPanel::on_set_expr() m_doc.checkpoint(); m_doc.features[m_edit_index].expr[field] = expr; bool ok = m_doc.recompute(); - if (!ok) { m_doc.undo(); } + // undo() recomputes, which SUCCEEDS and clears doc.error — so the reason the edit was + // rejected is gone before anything can display it. Carry it across the rollback. + if (!ok) { const std::string why = m_doc.error; m_doc.undo(); m_doc.error = why; } after_tree_edit(ok); if (ok) m_expr_text->Clear(); @@ -8958,7 +8969,9 @@ void DesignPanel::on_clear_expr() m_doc.checkpoint(); feat_expr.erase(field); bool ok = m_doc.recompute(); - if (!ok) { m_doc.undo(); } + // undo() recomputes, which SUCCEEDS and clears doc.error — so the reason the edit was + // rejected is gone before anything can display it. Carry it across the rollback. + if (!ok) { const std::string why = m_doc.error; m_doc.undo(); m_doc.error = why; } after_tree_edit(ok); if (m_edit_index >= 0 && m_edit_index < int(m_doc.features.size())) { diff --git a/src/slic3r/GUI/McpControl.cpp b/src/slic3r/GUI/McpControl.cpp index 7c1feecb09..98cd4cc638 100644 --- a/src/slic3r/GUI/McpControl.cpp +++ b/src/slic3r/GUI/McpControl.cpp @@ -1372,7 +1372,9 @@ json action_set_variable(DesignPanel* panel, const json& params) doc.checkpoint(); doc.variables[name] = expr; bool ok = doc.recompute(); - if (!ok) { doc.undo(); } + // undo() recomputes, which succeeds and clears doc.error; carry the reason across so + // the JSON reply below reports why the edit was rejected instead of an empty string. + if (!ok) { const std::string why = doc.error; doc.undo(); doc.error = why; } panel->mcp_after_change(); return json{{"ok", ok}, {"name", name}, {"error", doc.error}}; } @@ -1392,7 +1394,9 @@ json action_set_feature_expr(DesignPanel* panel, const json& params) doc.checkpoint(); doc.features[fi].expr[field] = expr; bool ok = doc.recompute(); - if (!ok) { doc.undo(); } + // undo() recomputes, which succeeds and clears doc.error; carry the reason across so + // the JSON reply below reports why the edit was rejected instead of an empty string. + if (!ok) { const std::string why = doc.error; doc.undo(); doc.error = why; } panel->mcp_after_change(); return json{{"ok", ok}, {"feature", fi}, {"field", field}, {"error", doc.error}}; } diff --git a/tests/libslic3r/test_caddocument.cpp b/tests/libslic3r/test_caddocument.cpp index 820333d0db..ca4ad15dc1 100644 --- a/tests/libslic3r/test_caddocument.cpp +++ b/tests/libslic3r/test_caddocument.cpp @@ -4503,6 +4503,51 @@ TEST_CASE("unknown identifier fails recompute", "[CadDocument][variables]") REQUIRE_CONTAINS(doc.error, "unknown identifier"); } +// The checkpoint -> mutate -> recompute -> undo-on-failure pattern is what both the GUI and +// McpControl use to keep a bad edit out of the document. It only works if the snapshot covers +// `variables` as well as `features`: undo() used to restore features alone, so a bad variable +// survived the rollback and every later recompute failed — the document was left unusable. +TEST_CASE("undo rolls back a bad variable, not just features", "[CadDocument][variables]") +{ + CadDocument doc; + int sk = doc.add_sketch(SketchShape::Rectangle, SketchPlane::XY(), 20, 20, 10, "Sketch"); + doc.add_extrude(sk, 10.0, false, BooleanMode::New, "Extrude"); + doc.features[sk].expr = {{"width", "w"}}; + doc.variables = {{"w", "20"}}; + REQUIRE(doc.recompute()); + + // Caller sets a variable to something unevaluable, exactly as action_set_variable does. + doc.checkpoint(); + doc.variables["w"] = "nosuchvar + 1"; + REQUIRE_FALSE(doc.recompute()); + REQUIRE(doc.undo()); + + // The good value must be back... + REQUIRE(doc.variables.at("w") == "20"); + // ...and, the part that actually bit, the document must still be usable. + REQUIRE(doc.recompute()); + REQUIRE(doc.error.empty()); +} + +// A variable ADDED under a checkpoint must disappear entirely on undo, not linger with a +// stale value: the pre-mutation snapshot simply did not contain the key. +TEST_CASE("undo removes a variable that did not exist before the checkpoint", "[CadDocument][variables]") +{ + CadDocument doc; + int sk = doc.add_sketch(SketchShape::Rectangle, SketchPlane::XY(), 20, 20, 10, "Sketch"); + doc.add_extrude(sk, 10.0, false, BooleanMode::New, "Extrude"); + REQUIRE(doc.recompute()); + REQUIRE(doc.variables.empty()); + + doc.checkpoint(); + doc.variables["bogus"] = "1/0 +"; // syntactically broken + REQUIRE_FALSE(doc.recompute()); + REQUIRE(doc.undo()); + + REQUIRE(doc.variables.count("bogus") == 0); + REQUIRE(doc.recompute()); +} + TEST_CASE("parametric recipe round-trips through serialize/deserialize", "[CadDocument][variables]") { using Catch::Matchers::WithinAbs;