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;