From 2cef101ffe738554ff9041d45a8353d5d0a2d55b Mon Sep 17 00:00:00 2001 From: Andrew <159703254+andrewsoonqn@users.noreply.github.com> Date: Fri, 10 Jul 2026 20:46:50 +0800 Subject: [PATCH] Make speed dial actions polymorphic Decouple registry dispatch from plugin-specific result types so future action types can provide their own run implementations. Store actions behind shared pointers to keep them alive while plugin UI pumps a nested event loop and queued refreshes mutate the registry. Scope "don't ask again" to an individual action instead of its whole plugin. --- src/slic3r/GUI/ActionRegistry.cpp | 102 +++++++++++++++++------------ src/slic3r/GUI/ActionRegistry.hpp | 79 ++++++++++++++++------ src/slic3r/GUI/SpeedDialDialog.cpp | 17 +++-- 3 files changed, 127 insertions(+), 71 deletions(-) diff --git a/src/slic3r/GUI/ActionRegistry.cpp b/src/slic3r/GUI/ActionRegistry.cpp index a2b30ae677..f37023979b 100644 --- a/src/slic3r/GUI/ActionRegistry.cpp +++ b/src/slic3r/GUI/ActionRegistry.cpp @@ -68,31 +68,41 @@ std::string find_loaded_package_name(PluginLoader& loader, const std::string& pl } // namespace -ScriptRunOutcome AppAction::run() const +PluginScriptAction::PluginScriptAction(std::string plugin_key, std::string capability, std::string package_name) + : AppAction(speed_dial_action_id(plugin_key, capability), // id + capability.empty() ? plugin_key : capability, // title + std::move(package_name)), // pkg = source (package name) + plugin_key(std::move(plugin_key)), capability(std::move(capability)) +{} + +AppActionRunResult PluginScriptAction::run() const { - return run_script_plugin_capability(plugin_key, capability); + ScriptRunOutcome o = run_script_plugin_capability(plugin_key, capability); + AppActionRunResult::Level level = AppActionRunResult::Level::Info; + switch (o.level) { + case ScriptRunOutcome::Level::Success: level = AppActionRunResult::Level::Success; break; + case ScriptRunOutcome::Level::Error: level = AppActionRunResult::Level::Error; break; + case ScriptRunOutcome::Level::Busy: level = AppActionRunResult::Level::Busy; break; + case ScriptRunOutcome::Level::Info: break; + } + return { level, o.message }; } // ---- build / enumeration ---------------------------------------------------- -bool ActionRegistry::make_action(const std::string& plugin_key, const std::string& capability, - const std::function& package_name_for, - AppAction& out) const +std::shared_ptr ActionRegistry::make_action(const std::string& plugin_key, const std::string& capability, + const std::function& package_name_for) const { PluginLoader& loader = PluginManager::instance().get_loader(); if (!loader.is_plugin_loaded(plugin_key)) - return false; + return nullptr; auto cap = loader.get_plugin_capability_by_name(plugin_key, PluginCapabilityType::Script, capability); if (!cap || !cap->enabled) - return false; + return nullptr; - out.id = speed_dial_action_id(plugin_key, capability); - out.title = capability.empty() ? plugin_key : capability; - out.pkg = package_name_for(plugin_key); - out.plugin_key = plugin_key; - out.capability = capability; - seed_state(out); - return true; + auto a = std::make_shared(plugin_key, capability, package_name_for(plugin_key)); + seed_state(*a); + return a; } void ActionRegistry::seed_state(AppAction& a) const @@ -132,8 +142,7 @@ void ActionRegistry::build() for (const auto& cap : loader.get_plugin_capabilities_by_type(PluginCapabilityType::Script)) { if (!cap || !cap->enabled) continue; - AppAction a; - if (make_action(cap->plugin_key, cap->name, package_name_for, a)) + if (auto a = make_action(cap->plugin_key, cap->name, package_name_for)) m_actions.push_back(std::move(a)); } } @@ -143,8 +152,8 @@ void ActionRegistry::build() const AppAction* ActionRegistry::by_id(const std::string& id) const { assert(wxThread::IsMain()); - auto it = std::find_if(m_actions.begin(), m_actions.end(), [&](const AppAction& a) { return a.id == id; }); - return it == m_actions.end() ? nullptr : &*it; + auto it = std::find_if(m_actions.begin(), m_actions.end(), [&](const auto& a) { return a->id == id; }); + return it == m_actions.end() ? nullptr : it->get(); } AppAction* ActionRegistry::find(const std::string& id) @@ -155,7 +164,7 @@ AppAction* ActionRegistry::find(const std::string& id) void ActionRegistry::erase_id(const std::string& id) { m_actions.erase(std::remove_if(m_actions.begin(), m_actions.end(), - [&](const AppAction& a) { return a.id == id; }), + [&](const auto& a) { return a->id == id; }), m_actions.end()); } @@ -175,13 +184,14 @@ void ActionRegistry::refresh_capability(const std::string& plugin_key, const std // multi-plugin lookup, so the callback can just return the already-resolved name. auto package_name_for = [&package_name](const std::string&) { return package_name; }; - AppAction a; - if (!make_action(plugin_key, capability, package_name_for, a)) { // why: disabled/unloaded capabilities leave the dial. + auto a = make_action(plugin_key, capability, package_name_for); + if (!a) { // why: disabled/unloaded capabilities leave the dial. erase_id(id); return; } - if (AppAction* existing = find(id)) - *existing = std::move(a); + auto it = std::find_if(m_actions.begin(), m_actions.end(), [&](const auto& e) { return e->id == id; }); + if (it != m_actions.end()) + *it = std::move(a); else m_actions.push_back(std::move(a)); } @@ -190,8 +200,14 @@ void ActionRegistry::refresh_package(const std::string& plugin_key, ActionChange { assert(wxThread::IsMain()); // Drop every action of this package, then (on add/update) re-add its current script caps. + // why: this is the producer path (loader-driven), so it legitimately knows it deals with + // PluginScriptAction - the one type it builds; the cast is not the dispatch-time switch + // that run() polymorphism removed. m_actions.erase(std::remove_if(m_actions.begin(), m_actions.end(), - [&](const AppAction& a) { return a.plugin_key == plugin_key; }), + [&](const auto& a) { + auto* p = dynamic_cast(a.get()); + return p && p->plugin_key == plugin_key; + }), m_actions.end()); if (change == ActionChange::Removed) return; @@ -204,26 +220,26 @@ void ActionRegistry::refresh_package(const std::string& plugin_key, ActionChange for (const auto& cap : loader.get_plugin_capabilities_by_type(plugin_key, PluginCapabilityType::Script)) { if (!cap || !cap->enabled) continue; - AppAction a; - if (make_action(plugin_key, cap->name, package_name_for, a)) + if (auto a = make_action(plugin_key, cap->name, package_name_for)) m_actions.push_back(std::move(a)); } } // ---- dispatch + write-through ---------------------------------------------- -ScriptRunOutcome ActionRegistry::run(const std::string& id) +AppActionRunResult ActionRegistry::run(const std::string& id) { assert(wxThread::IsMain()); - const AppAction* a = by_id(id); - if (!a) - return ScriptRunOutcome{ScriptRunOutcome::Level::Info, {}}; - // why: run a stack COPY, never the vector element. The script runner pumps a nested - // event loop (plugin modal UI) and holds the action's strings by reference; a queued - // refresh_* landing during that loop may reallocate m_actions and dangle them. - const AppAction action = *a; - ScriptRunOutcome o = action.run(); - if (o.level == ScriptRunOutcome::Level::Busy) + auto it = std::find_if(m_actions.begin(), m_actions.end(), [&](const auto& e) { return e->id == id; }); + if (it == m_actions.end()) + return {}; // default Info, empty message + // why: hold a shared_ptr keep-alive, never a bare vector element. The script runner pumps + // a nested event loop (plugin modal UI); a queued refresh_* landing during that loop may + // erase this action from m_actions - the keep-alive keeps the heap object valid until run + // returns. (Replaces the old stack-copy; an abstract AppAction cannot be sliced.) + std::shared_ptr keep = *it; + AppActionRunResult o = keep->run(); + if (o.level == AppActionRunResult::Level::Busy) return o; // Bump stats (write-through). Re-read to avoid clobbering a concurrent field. @@ -271,19 +287,19 @@ void ActionRegistry::reorder_favourites(const std::vector& ids) write_section("favourite_actions", nlohmann::json(next)); } -bool ActionRegistry::should_ask(const std::string& plugin_key) const +bool ActionRegistry::should_ask(const std::string& id) const { assert(wxThread::IsMain()); auto arr = read_string_array("ask_suppressed"); - return std::find(arr.begin(), arr.end(), plugin_key) == arr.end(); + return std::find(arr.begin(), arr.end(), id) == arr.end(); } -void ActionRegistry::suppress_ask(const std::string& plugin_key) +void ActionRegistry::suppress_ask(const std::string& id) { assert(wxThread::IsMain()); auto arr = read_string_array("ask_suppressed"); - if (std::find(arr.begin(), arr.end(), plugin_key) == arr.end()) - arr.push_back(plugin_key); + if (std::find(arr.begin(), arr.end(), id) == arr.end()) + arr.push_back(id); write_section("ask_suppressed", nlohmann::json(arr)); } @@ -294,8 +310,8 @@ nlohmann::json ActionRegistry::snapshot() const assert(wxThread::IsMain()); std::vector sorted; sorted.reserve(m_actions.size()); - for (const AppAction& a : m_actions) - sorted.push_back(&a); + for (const auto& a : m_actions) + sorted.push_back(a.get()); const long long now = (long long) std::time(nullptr); std::stable_sort(sorted.begin(), sorted.end(), [&](const AppAction* a, const AppAction* b) { diff --git a/src/slic3r/GUI/ActionRegistry.hpp b/src/slic3r/GUI/ActionRegistry.hpp index 022194e139..42bcf26b45 100644 --- a/src/slic3r/GUI/ActionRegistry.hpp +++ b/src/slic3r/GUI/ActionRegistry.hpp @@ -1,6 +1,6 @@ #pragma once -#include "PluginScriptRunner.hpp" // ScriptRunOutcome +#include "PluginScriptRunner.hpp" // wxString + ScriptRunOutcome (mapped in the .cpp, not exposed here) #include @@ -8,6 +8,7 @@ #include #include +#include #include #include @@ -15,24 +16,62 @@ namespace Slic3r { namespace GUI { enum class ActionChange { Added, Removed, Updated }; +// Result of running an AppAction, in the ACTION layer's own vocabulary. +// why: run() must not leak plugin-specific types through the action abstraction, so +// this is deliberately NOT ScriptRunOutcome (the script/plugin layer's type) and is +// neither derived from nor composed with it. Each subclass POPULATES this from +// whatever its underlying runner returns (PluginScriptAction maps a ScriptRunOutcome +// into it). The shape mirrors ScriptRunOutcome for now; keeping them separate lets the +// two diverge freely as non-plugin action sources land. +struct AppActionRunResult +{ + enum class Level { Success, Info, Error, Busy }; + + Level level = Level::Info; + wxString message; // empty = "nothing worth showing" +}; + // A speed-dial action: identity + user-state seeded from config + how to run itself. -// Holds a weak (plugin_key, capability) handle, never a live capability pointer. +// Abstract base - the only virtual is run(); a concrete subclass (e.g. +// PluginScriptAction) knows how to run and what its pkg/source is. // note: named AppAction, not Action - Slic3r::GUI::Action is already taken by // UnsavedChangesDialog's exit-action enum, and this header reaches most GUI TUs. struct AppAction { - std::string id; // speed_dial_action_id(plugin_key, capability) - std::string title; // capability display name - std::string pkg; // owning package display name (row eyebrow) - std::string plugin_key; // weak handle: owning package - std::string capability; // weak handle: capability name + std::string id; // speed_dial_action_id(...) - stable identity + AppConfig key + std::string title; // display name + // pkg is the action's SOURCE - where it comes from. For a plugin action it is + // the owning package's display name (row eyebrow); a future app-command action + // would set its own label. Each subclass sets pkg in its constructor (below), + // so the base cannot be built without one. + std::string pkg; // seeded from AppConfig for the snapshot / sort: bool favourite = false; int count = 0; long long last = 0; // epoch seconds - ScriptRunOutcome run() const; // re-resolves the capability and runs it (UI thread) + virtual ~AppAction() = default; + virtual AppActionRunResult run() const = 0; // re-resolves + runs (UI thread) + +protected: + // why: protected + no default forces every subclass to supply id/title/pkg. A + // virtual compute_pkg() can't be used here - virtual dispatch from a base ctor + // resolves to the base, so this constructor-passing is the enforced substitute. + AppAction(std::string id, std::string title, std::string pkg) + : id(std::move(id)), title(std::move(title)), pkg(std::move(pkg)) {} +}; + +// The one action source today: a runnable SCRIPT plugin capability. Holds a weak +// (plugin_key, capability) handle, never a live capability pointer - run() +// re-resolves through the loader. +struct PluginScriptAction : AppAction +{ + std::string plugin_key; // weak handle: owning package + std::string capability; // weak handle: capability name + + PluginScriptAction(std::string plugin_key, std::string capability, std::string package_name); + AppActionRunResult run() const override; }; // Single source of runnable actions for the app session, owned by GUI_App. @@ -46,8 +85,8 @@ public: void build(); // Always-clean read surface. UI thread only. - const std::vector& all() const { assert(wxThread::IsMain()); return m_actions; } - const AppAction* by_id(const std::string& id) const; + const std::vector>& all() const { assert(wxThread::IsMain()); return m_actions; } + const AppAction* by_id(const std::string& id) const; // Targeted incremental maintenance, reached via CallAfter from loader callbacks. // refresh_package syncs every script capability of one package (load/unload). @@ -56,12 +95,13 @@ public: void refresh_capability(const std::string& plugin_key, const std::string& capability, ActionChange change); // Dispatch + write-through (registry is the only thing that touches AppConfig). - ScriptRunOutcome run(const std::string& id); // runs + bumps stats + AppActionRunResult run(const std::string& id); // runs + bumps stats void set_favourite(const std::string& id, bool on); void reorder_favourites(const std::vector& ids); // persist a new bar order - bool should_ask(const std::string& plugin_key) const; // run-confirm gate - void suppress_ask(const std::string& plugin_key); + // Run-confirm gate, keyed by action id (per-action "don't ask again"). + bool should_ask(const std::string& id) const; + void suppress_ask(const std::string& id); // Flat, frecency-sorted snapshot for the webview: {actions:[...], favourites:[...]}. nlohmann::json snapshot() const; @@ -70,16 +110,17 @@ private: void seed_state(AppAction& a) const; // favourite/stats from config AppAction* find(const std::string& id); void erase_id(const std::string& id); - // Builds the AppAction for one loaded+enabled script capability; returns false - // (and builds nothing) when the capability is unloaded, missing, or disabled. - bool make_action(const std::string& plugin_key, const std::string& capability, - const std::function& package_name_for, - AppAction& out) const; + // Builds the action for one loaded+enabled script capability; returns nullptr + // when the capability is unloaded, missing, or disabled. + std::shared_ptr make_action(const std::string& plugin_key, const std::string& capability, + const std::function& package_name_for) const; // why: vector storage favors the popup path, which snapshots by iterating, sorting, and // serializing small action sets. Id lookups are event-driven and build is startup-only, so // a persistent map would add hashing/allocation cost without helping render. - std::vector m_actions; // UI-thread confined; no lock + // shared_ptr: AppAction is abstract (stored by pointer), and run() takes a keep-alive + // copy so a queued refresh can't erase the object mid-run (nested plugin event loop). + std::vector> m_actions; // UI-thread confined; no lock }; }} // namespace Slic3r::GUI diff --git a/src/slic3r/GUI/SpeedDialDialog.cpp b/src/slic3r/GUI/SpeedDialDialog.cpp index c0f392fed8..514859b566 100644 --- a/src/slic3r/GUI/SpeedDialDialog.cpp +++ b/src/slic3r/GUI/SpeedDialDialog.cpp @@ -254,30 +254,29 @@ private: const AppAction* a = reg.by_id(id); if (!a) return; - const std::string key = a->plugin_key; - const std::string cap = a->capability; - const bool ask = reg.should_ask(key); + const bool ask = reg.should_ask(id); + 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) { - const wxString label = title.empty() ? from_u8(cap) : from_u8(title); + const wxString label = title.empty() ? from_u8(atitle) : from_u8(title); RichMessageDialog dlg(wxGetApp().mainframe, wxString::Format(_L("Run \"%s\"?"), label), _L("Run plugin"), wxOK | wxCANCEL); - dlg.ShowCheckBox(_L("Don't ask again for this plugin")); + dlg.ShowCheckBox(_L("Don't ask again for this action")); if (dlg.ShowModal() != wxID_OK) return; if (dlg.IsCheckBoxChecked()) - wxGetApp().action_registry().suppress_ask(key); + wxGetApp().action_registry().suppress_ask(id); } // why: value-capture only. Script execution may outlive this popup if the app is closing. wxGetApp().CallAfter([id] { - ScriptRunOutcome o = wxGetApp().action_registry().run(id); - if (o.level == ScriptRunOutcome::Level::Busy) + AppActionRunResult o = wxGetApp().action_registry().run(id); + if (o.level == AppActionRunResult::Level::Busy) return; if (!o.message.IsEmpty()) wxGetApp().plater()->get_notification_manager()->push_notification( NotificationType::CustomNotification, - o.level == ScriptRunOutcome::Level::Error ? NotificationManager::NotificationLevel::ErrorNotificationLevel : + o.level == AppActionRunResult::Level::Error ? NotificationManager::NotificationLevel::ErrorNotificationLevel : NotificationManager::NotificationLevel::RegularNotificationLevel, into_u8(o.message)); });