From 87c9ab138c534ab98b38d6f515cbbd5cf7fc1396 Mon Sep 17 00:00:00 2001 From: Andrew <159703254+andrewsoonqn@users.noreply.github.com> Date: Mon, 13 Jul 2026 17:32:16 +0800 Subject: [PATCH] Protect action identity from mutation Keep action identity and display metadata constructor-set so registry keys cannot drift from the objects they index. Require sources to publish unique ownership while the registry retains shared keepalive for runs. Preserve opaque IDs and persisted state keys. --- src/slic3r/GUI/ActionRegistry.cpp | 27 +++++++++--------- src/slic3r/GUI/ActionRegistry.hpp | 24 +++++++++------- src/slic3r/GUI/ScriptActionSource.cpp | 12 ++++---- src/slic3r/GUI/ScriptActionSource.hpp | 2 +- src/slic3r/GUI/SpeedDialDialog.cpp | 2 +- tests/slic3rutils/test_action_source.cpp | 35 ++++++++++++++++++++++++ 6 files changed, 71 insertions(+), 31 deletions(-) diff --git a/src/slic3r/GUI/ActionRegistry.cpp b/src/slic3r/GUI/ActionRegistry.cpp index 270f1b03e5..326bbbf6c5 100644 --- a/src/slic3r/GUI/ActionRegistry.cpp +++ b/src/slic3r/GUI/ActionRegistry.cpp @@ -68,15 +68,16 @@ void ActionRegistry::add_source(std::unique_ptr source) m_sources.push_back(std::move(source)); } -void ActionRegistry::upsert(std::shared_ptr action) +void ActionRegistry::upsert(std::unique_ptr action) { assert(wxThread::IsMain()); if (!action) return; seed_state(*action); - std::string id = action->id; - m_actions.insert_or_assign(std::move(id), std::move(action)); + std::string id = action->id(); + std::shared_ptr stored = std::move(action); + m_actions.insert_or_assign(std::move(id), std::move(stored)); } void ActionRegistry::remove(const std::string& id) @@ -88,10 +89,10 @@ void ActionRegistry::remove(const std::string& id) void ActionRegistry::seed_state(AppAction& a) const { auto favs = read_string_array("favourite_actions"); - a.favourite = std::find(favs.begin(), favs.end(), a.id) != favs.end(); + a.favourite = std::find(favs.begin(), favs.end(), a.id()) != favs.end(); nlohmann::json stats = read_section("stats", nlohmann::json::object()); - auto it = stats.find(a.id); + auto it = stats.find(a.id()); if (it != stats.end() && it->is_object()) { a.count = it->value("count", 0); a.last = it->value("last", 0LL); @@ -208,18 +209,18 @@ nlohmann::json ActionRegistry::snapshot() const double sb = frecency_score(b->count, b->last, now); if (sa != sb) return sa > sb; - if (a->title != b->title) - return a->title < b->title; - if (a->source != b->source) - return a->source < b->source; - return a->id < b->id; + if (a->title() != b->title()) + return a->title() < b->title(); + if (a->source() != b->source()) + return a->source() < b->source(); + return a->id() < b->id(); }); nlohmann::json actions = nlohmann::json::array(); for (const AppAction* a : sorted) - actions.push_back({{"id", a->id}, - {"title", a->title}, - {"source", a->source}, + actions.push_back({{"id", a->id()}, + {"title", a->title()}, + {"source", a->source()}, {"shortcut", ""}}); // why: favourites is the ORDERED pin list - it must come from favourite_actions // as stored, not be re-derived from the frecency-sorted actions (that would diff --git a/src/slic3r/GUI/ActionRegistry.hpp b/src/slic3r/GUI/ActionRegistry.hpp index c76f6bee44..eb85d09737 100644 --- a/src/slic3r/GUI/ActionRegistry.hpp +++ b/src/slic3r/GUI/ActionRegistry.hpp @@ -33,11 +33,9 @@ struct AppActionRunResult // UnsavedChangesDialog's exit-action enum, and this header reaches most GUI TUs. struct AppAction { - std::string id; // speed_dial_action_id(...) - stable identity + AppConfig key - std::string title; // display name - // Where the action comes from. Each subclass sets it in its constructor, so the - // base cannot be built without one. - std::string source; + const std::string& id() const { return m_id; } + const std::string& title() const { return m_title; } + const std::string& source() const { return m_source; } // seeded from AppConfig for the snapshot / sort: bool favourite = false; @@ -48,9 +46,15 @@ struct AppAction virtual AppActionRunResult run() const = 0; // re-resolves + runs (UI thread) protected: - // why: protected + no default forces every subclass to supply id/title/source. + // The definition is constructor-set and immutable. Refreshes replace an action + // instead of mutating identity after the registry has indexed it by id. AppAction(std::string id, std::string title, std::string source) - : id(std::move(id)), title(std::move(title)), source(std::move(source)) {} + : m_id(std::move(id)), m_title(std::move(title)), m_source(std::move(source)) {} + +private: + std::string m_id; // speed_dial_action_id(...) - stable identity + AppConfig key + std::string m_title; // display name + std::string m_source; // display name of the action's source }; // Generic sink and single owner of runnable actions for the app session. @@ -74,9 +78,9 @@ public: // until init() starts it. void add_source(std::unique_ptr source); - // Seeds persisted state, then inserts the action or replaces the action with the - // same id. A null action is ignored. - void upsert(std::shared_ptr action); + // Takes ownership, seeds persisted state, then inserts the action or replaces + // the action with the same id. A null action is ignored. + void upsert(std::unique_ptr action); // Removes the action with this id. Missing ids are a harmless no-op. void remove(const std::string& id); diff --git a/src/slic3r/GUI/ScriptActionSource.cpp b/src/slic3r/GUI/ScriptActionSource.cpp index 64c33f7044..de70c96288 100644 --- a/src/slic3r/GUI/ScriptActionSource.cpp +++ b/src/slic3r/GUI/ScriptActionSource.cpp @@ -103,15 +103,15 @@ void ScriptActionSource::start(ActionRegistry& sink) auto source = source_names.find(capability->plugin_key); const std::string& source_name = source == source_names.end() ? capability->plugin_key : source->second; if (auto action = make_action(capability->plugin_key, capability->name, source_name)) { - track(capability->plugin_key, action->id); + track(capability->plugin_key, action->id()); m_sink->upsert(std::move(action)); } } } -std::shared_ptr ScriptActionSource::make_action(const std::string& plugin_key, - const std::string& capability, - const std::string& source) const +std::unique_ptr ScriptActionSource::make_action(const std::string& plugin_key, + const std::string& capability, + const std::string& source) const { PluginLoader& loader = PluginManager::instance().get_loader(); if (!loader.is_plugin_loaded(plugin_key)) @@ -121,7 +121,7 @@ std::shared_ptr ScriptActionSource::make_action(const std::string& pl if (!loaded || !loaded->enabled) return nullptr; - return std::make_shared(plugin_key, capability, source); + return std::make_unique(plugin_key, capability, source); } void ScriptActionSource::refresh_capability(const std::string& plugin_key, const std::string& capability, @@ -169,7 +169,7 @@ void ScriptActionSource::refresh_source(const std::string& plugin_key, ActionCha if (!capability || !capability->enabled) continue; if (auto action = make_action(plugin_key, capability->name, source_name)) { - track(plugin_key, action->id); + track(plugin_key, action->id()); m_sink->upsert(std::move(action)); } } diff --git a/src/slic3r/GUI/ScriptActionSource.hpp b/src/slic3r/GUI/ScriptActionSource.hpp index 06e5f09d66..a15e906e15 100644 --- a/src/slic3r/GUI/ScriptActionSource.hpp +++ b/src/slic3r/GUI/ScriptActionSource.hpp @@ -28,7 +28,7 @@ public: private: void refresh_source(const std::string& plugin_key, ActionChange change); void refresh_capability(const std::string& plugin_key, const std::string& capability, ActionChange change); - std::shared_ptr make_action(const std::string& plugin_key, const std::string& capability, + std::unique_ptr make_action(const std::string& plugin_key, const std::string& capability, const std::string& source) const; void track(const std::string& plugin_key, const std::string& id); void untrack(const std::string& plugin_key, const std::string& id); diff --git a/src/slic3r/GUI/SpeedDialDialog.cpp b/src/slic3r/GUI/SpeedDialDialog.cpp index 514859b566..447d6a8dca 100644 --- a/src/slic3r/GUI/SpeedDialDialog.cpp +++ b/src/slic3r/GUI/SpeedDialDialog.cpp @@ -255,7 +255,7 @@ private: if (!a) return; const bool ask = reg.should_ask(id); - const std::string atitle = a->title; // capture before Dismiss; `a` may not outlive it + const std::string atitle = a->title(); // capture before Dismiss; `a` may not outlive it Dismiss(); // launcher gone before the modal / the script's own UI if (ask) { diff --git a/tests/slic3rutils/test_action_source.cpp b/tests/slic3rutils/test_action_source.cpp index 8a90d2ccb1..f85d1b42d9 100644 --- a/tests/slic3rutils/test_action_source.cpp +++ b/tests/slic3rutils/test_action_source.cpp @@ -4,7 +4,11 @@ #include "slic3r/GUI/IActionSource.hpp" #include +#include +#include +using Slic3r::GUI::AppAction; +using Slic3r::GUI::AppActionRunResult; using Slic3r::GUI::ActionRegistry; using Slic3r::GUI::IActionSource; @@ -21,8 +25,39 @@ private: bool& m_started; }; +// AppAction is abstract; this minimal concrete action lets the tests exercise +// its constructor-set definition without involving a plugin runner. +class TestAppAction final : public AppAction +{ +public: + TestAppAction() : AppAction("action-id", "Action title", "Action source") {} + + AppActionRunResult run() const override { return {}; } +}; + } // namespace +TEST_CASE("AppAction definitions are immutable after construction", "[speeddial][actions]") +{ + using StringAccessor = const std::string& (AppAction::*)() const; + + STATIC_CHECK(std::is_same_v); + STATIC_CHECK(std::is_same_v); + STATIC_CHECK(std::is_same_v); + + const TestAppAction action; + CHECK(action.id() == "action-id"); + CHECK(action.title() == "Action title"); + CHECK(action.source() == "Action source"); +} + +TEST_CASE("ActionRegistry takes exclusive ownership of published actions", "[speeddial][actions]") +{ + using ExpectedUpsert = void (ActionRegistry::*)(std::unique_ptr); + + STATIC_CHECK(std::is_same_v); +} + TEST_CASE("ActionRegistry starts registered action sources", "[speeddial][actions]") { bool started = false;