Dispatch layer rename through app core

2026-06-03 20:06:11 +02:00
parent 921fc8f00b
commit 10c995f1da
6 changed files with 185 additions and 51 deletions
--- a/docs/modernization/capability-map.md
+++ b/docs/modernization/capability-map.md
@@ -45,7 +45,7 @@ and validation command.

 | Capability | Current Area | Target Owner | Required Tests |
 | --- | --- | --- | --- |
-| Layer add/remove/move/merge | `Canvas`, `Layer`, actions | `pp_document`, `pp_app_core` | Operation planning, service-dispatch, no-op preservation, undo/redo invariant tests |
+| Layer rename/add/remove/move/merge | `Canvas`, `Layer`, actions | `pp_document`, `pp_app_core` | Rename and operation planning, service-dispatch, no-op preservation, undo/redo invariant tests |
 | Blend/opacity/visibility/alpha lock | `Layer`, UI panels, shaders | `pp_document`, `pp_app_core`, `pp_paint_renderer` | Metadata planning, service-dispatch, CPU model and render golden |
 | Selection mask | `Canvas` mask layer | `pp_document`, `pp_paint_renderer` | Mask apply/clear edge cases |
 | Animation frames | `LayerFrame`, animation panel | `pp_document`, `pp_panopainter_ui` | Duration, duplicate, remove, seek |
--- a/docs/modernization/debt.md
+++ b/docs/modernization/debt.md
@@ -38,7 +38,7 @@ agent or engineer to remove them without reconstructing context from chat.
 | DEBT-0017 | Open | Modernization | Startup storage path preparation, `App::clipboard_get_text`, `App::clipboard_set_text`, `App::show_cursor`, `App::hide_cursor`, `App::showKeyboard`, `App::hideKeyboard`, `App::display_file`, `App::share_file`, native app/window close, UI-thread lifecycle hooks, render-context acquire/release/present hooks, render-target binding hooks, render platform hint hooks, render debug callback hooks, render-capture frame hooks, recording cleanup, live asset/layout reload policy, diagnostic stacktrace/crash hooks, per-frame platform hooks, `App::pick_image`, `App::pick_file`, the non-writer `App::pick_file_save`, `App::pick_dir`, and prepared-file save/download handoff now call the SDK-free `pp::platform::PlatformServices` interface, and Windows injects `WindowsPlatformServices` from `src/platform_windows/windows_platform_services.*`; non-Windows live implementations still use `src/platform_legacy/legacy_platform_services.*`, a named fallback adapter that forwards to retained Apple/Android/Linux/Web bridge functions and retained no-op branches | Preserve behavior while moving platform execution behind a testable service boundary before platform shell implementations are injected | `pp_platform_api_tests`; `pp_app_core_document_platform_io_tests`; `ctest --preset desktop-fast --build-config Debug`; `powershell -ExecutionPolicy Bypass -File scripts\automation\package-smoke.ps1 -Preset windows-msvc-default -Configuration Debug` | Replace `src/platform_legacy/legacy_platform_services.*` with injected `pp_platform_*` service implementations owned by each non-Windows platform shell |
 | DEBT-0019 | Open | Modernization | Unreferenced-parameter warnings are muted globally through `pp_project_warnings` with MSVC `/wd4100` and Clang/GCC `-Wno-unused-parameter` | Legacy callbacks, virtual hooks, serializer methods, and platform/API compatibility functions carry many intentionally unused parameters during the component split; muting this keeps stricter warning builds focused on higher-signal migration issues | `cmake --build --preset windows-msvc-default --config Debug --target PanoPainter`; `ctest --preset desktop-fast --build-config Debug`; `cmake --build --preset linux-clang --target pp_foundation` | Remove `/wd4100` and `-Wno-unused-parameter`, mark intentionally unused parameters with names/comments or `[[maybe_unused]]`, and make the Windows app plus headless Clang/GCC tests pass without unreferenced-parameter warnings |
 | DEBT-0020 | Open | Modernization | Document resize dialog state, selected-resolution planning, and execution dispatch now consume pure `pp_app_core` through `NodeDialogResize`, `App::dialog_resize`, `pano_cli plan-document-resize`, and the `DocumentResizeServices` boundary, but the live adapter still calls legacy `Canvas::resize`, updates the legacy app title, and clears legacy `ActionManager` history | Preserve existing layer/frame GPU resize behavior while the document model and canvas execution boundary are extracted incrementally | `pp_app_core_document_resize_tests`; `pano_cli plan-document-resize --current-resolution 2048 --selected-resolution-index 4`; `ctest --preset desktop-fast --build-config Debug` | Document resize execution is owned by injected document/app services with no legacy resize adapter, title shim, or direct `ActionManager` history clearing |
-| DEBT-0021 | Open | Modernization | Layer rename planning and layer panel operation planning/execution dispatch now consume pure `pp_app_core` through `App::dialog_layer_rename`, `App::init_sidebar` layer callbacks, `pano_cli plan-layer-rename`, `pano_cli plan-layer-operation`, and the `DocumentLayerOperationServices` boundary, but the live operation adapter still mutates legacy `Canvas` layer state and `NodeLayer`/`NodePanelLayer`, while rename still reaches `ActionManager` undo entries directly | Preserve existing UI/canvas behavior while document layer commands and undo history are extracted incrementally | `pp_app_core_document_layer_tests`; `pano_cli plan-layer-rename --old-name Base --new-name Paint`; `pano_cli plan-layer-operation --kind add --layer-count 2 --index 1 --name Paint`; `ctest --preset desktop-fast --build-config Debug` | Layer command execution is owned by the document/app command boundary with legacy `Canvas`/UI nodes acting only as adapters or removed entirely |
+| DEBT-0021 | Open | Modernization | Layer rename planning/execution dispatch and layer panel operation planning/execution dispatch now consume pure `pp_app_core` through `App::dialog_layer_rename`, `App::init_sidebar` layer callbacks, `pano_cli plan-layer-rename`, `pano_cli plan-layer-operation`, `DocumentLayerRenameServices`, and `DocumentLayerOperationServices`, but the live adapters still mutate legacy `Canvas` layer state, `NodeLayer`/`NodePanelLayer`, and `ActionManager` undo entries | Preserve existing UI/canvas behavior while document layer commands and undo history are extracted incrementally | `pp_app_core_document_layer_tests`; `pano_cli plan-layer-rename --old-name Base --new-name Paint`; `pano_cli plan-layer-operation --kind add --layer-count 2 --index 1 --name Paint`; `ctest --preset desktop-fast --build-config Debug` | Layer command execution is owned by the document/app command boundary with legacy `Canvas`/UI nodes acting only as adapters or removed entirely |
 | DEBT-0022 | Open | Modernization | Animation panel frame command planning, panel action planning, panel-control/timeline execution dispatch, selected-frame click dispatch, playback tick stepping, and play-mode toggles now consume pure `pp_app_core` through `NodePanelAnimation`, `pano_cli plan-animation-operation`, `pano_cli plan-animation-panel-action`, and `DocumentAnimationServices`, and `pp_legacy_ui_core` temporarily links `pp_app_core`, but the live adapter still mutates or reads legacy `Canvas`/`Layer` frame state and canvas mode directly | Preserve existing animation panel behavior while timeline/frame commands move toward the document/app command boundary | `pp_app_core_document_animation_tests`; `pano_cli plan-animation-operation --kind add --frame-count 2 --current-frame 0`; `pano_cli plan-animation-operation --kind select --frame-count 3 --selected-frame 1 --layer-index 2 --layer-id 42`; `pano_cli plan-animation-operation --kind playback --total-duration 5 --current-frame 4 --offset 1`; `pano_cli plan-animation-operation --kind toggle-playback --playing`; `pano_cli plan-animation-panel-action --action next --total-duration 5 --current-frame 4`; `ctest --preset desktop-fast --build-config Debug` | Animation frame/timeline/playback execution is owned by injected document/app timeline services with no legacy `Canvas`/`Layer`/canvas-mode adapter and UI nodes acting only as adapters or removed entirely |
 | DEBT-0023 | Open | Modernization | Brush/color/preset/stroke-settings UI planning, texture-list add/remove/reorder planning, stroke-panel slider/toggle/blend/reset planning, and execution dispatch now consume pure `pp_app_core` through `App::init_sidebar`, `NodePanelBrush`, `NodePanelStroke`, restored/docked floating-panel callbacks, `pano_cli plan-brush-operation`, `pano_cli plan-brush-texture-list`, `pano_cli plan-brush-stroke-control`, `BrushUiServices`, `BrushTextureListServices`, and `BrushStrokeControlServices`, but the live adapter still mutates legacy `Brush`/`Canvas::I`, loads/saves legacy brush texture images, and refreshes legacy quick/stroke/color widgets | Preserve existing brush UI behavior while brush commands move toward a brush/app/asset command boundary and asset-managed texture selection | `pp_app_core_brush_ui_tests`; `pano_cli plan-brush-operation --kind color --r 0.25 --g 0.5 --b 0.75 --a 1`; `pano_cli plan-brush-operation --kind pattern --path data/patterns/noise.png --thumb data/patterns/thumbs/noise.png`; `pano_cli plan-brush-texture-list --kind add --dir brushes --data-path data --source C:/Temp/soft.png`; `pano_cli plan-brush-stroke-control --kind float --setting tip-size --value 42.5`; `pano_cli plan-brush-stroke-control --kind blend --setting pattern --blend-mode 3`; `ctest --preset desktop-fast --build-config Debug` | Brush color/texture/preset/stroke-settings, texture-list, and stroke-control execution are owned by injected brush/app/asset/UI services with no legacy brush/canvas adapter |
 | DEBT-0024 | Open | Modernization | Grid/heightmap/lightmap UI planning now consumes pure `pp_app_core` through `NodePanelGrid` and `pano_cli plan-grid-operation`, but live execution still performs legacy image loading, OpenGL texture updates, nanort lightmap baking, progress UI, and `Canvas::draw_objects` commit directly | Preserve grid/lightmap behavior while moving renderable grid commands toward app/renderer/document boundaries | `pp_app_core_grid_ui_tests`; `pano_cli plan-grid-operation --kind render --float32 --texture-resolution 1024 --samples 32`; `ctest --preset desktop-fast --build-config Debug` | Grid heightmap/lightmap execution is owned by app/renderer/document services with `NodePanelGrid` acting only as UI adapter |
--- a/docs/modernization/roadmap.md
+++ b/docs/modernization/roadmap.md
@@ -488,8 +488,9 @@ selected-resolution commit plan used by the live document resize dialog, and
 resize execution now dispatches through `DocumentResizeServices` before the
 legacy `Canvas` resize adapter and `ActionManager` history clearing continue.
 `pano_cli plan-layer-rename` exposes the app-core layer rename decision used by
-the live layer rename dialog before legacy `Canvas` layer mutation and
-`ActionManager` undo wiring continue.
+the live layer rename dialog, and rename execution now dispatches through
+`DocumentLayerRenameServices` before legacy `Canvas`, `NodeLayer`, and
+`ActionManager` undo adapters continue.
 `pano_cli plan-layer-operation` exposes app-core planning for layer add,
 duplicate, select, reorder, remove, opacity, visibility, alpha-lock, blend-mode,
 and highlight actions used by the live layer panel. Direct layer-panel
@@ -1238,14 +1239,16 @@ Results:
  `pano_cli_plan_document_resize_rejects_invalid_selection` passed and expose
  live document-resize planning as JSON automation.
 - `pp_app_core_document_layer_tests` passed, covering changed layer rename,
-  unchanged no-op rename, empty-name rejection, overlong-name rejection, layer
-  add/duplicate/select/reorder/remove planning, metadata planning, bad-index
-  rejection, bad-opacity rejection, bad-blend-mode rejection, transient
-  highlight behavior, layer operation executor dispatch, layer operation
-  side-effect dispatch, no-op operation preservation, malformed operation
-  rejection, Layer menu labels/actions, merge-down routing, animated merge
-  blocking, missing selection handling, bad Layer menu state rejection, Layer
-  menu executor dispatch, and no-op menu execution preservation.
+  unchanged no-op rename, empty-name rejection, overlong-name rejection, rename
+  executor dispatch, no-op rename dialog finish, malformed rename-plan
+  rejection, layer add/duplicate/select/reorder/remove planning, metadata
+  planning, bad-index rejection, bad-opacity rejection, bad-blend-mode
+  rejection, transient highlight behavior, layer operation executor dispatch,
+  layer operation side-effect dispatch, no-op operation preservation,
+  malformed operation rejection, Layer menu labels/actions, merge-down routing,
+  animated merge blocking, missing selection handling, bad Layer menu state
+  rejection, Layer menu executor dispatch, and no-op menu execution
+  preservation.
 - `pano_cli_plan_layer_rename_smoke`,
  `pano_cli_plan_layer_rename_no_op_smoke`, and
  `pano_cli_plan_layer_rename_rejects_empty_name` passed and expose live
--- a/src/app_core/document_layer.h
+++ b/src/app_core/document_layer.h
@@ -88,6 +88,14 @@ public:
    virtual void show_merge_animated_not_supported() = 0;
 };

+class DocumentLayerRenameServices {
+public:
+    virtual ~DocumentLayerRenameServices() = default;
+
+    virtual void rename_layer(std::string_view old_name, std::string_view new_name) = 0;
+    virtual void finish_layer_rename() = 0;
+};
+
 class DocumentLayerOperationServices {
 public:
    virtual ~DocumentLayerOperationServices() = default;
@@ -439,6 +447,29 @@ public:
    return pp::foundation::Result<DocumentLayerMenuPlan>::success(std::move(plan));
 }

+[[nodiscard]] inline pp::foundation::Status execute_document_layer_rename_plan(
+    const DocumentLayerRenamePlan& plan,
+    DocumentLayerRenameServices& services)
+{
+    switch (plan.action) {
+    case DocumentLayerRenameAction::no_op_same_name:
+        services.finish_layer_rename();
+        return pp::foundation::Status::success();
+    case DocumentLayerRenameAction::rename_and_record_undo:
+        if (plan.new_name.empty()) {
+            return pp::foundation::Status::invalid_argument("layer rename plan must include a new name");
+        }
+        if (plan.old_name == plan.new_name) {
+            return pp::foundation::Status::invalid_argument("layer rename plan must change the name");
+        }
+        services.rename_layer(plan.old_name, plan.new_name);
+        services.finish_layer_rename();
+        return pp::foundation::Status::success();
+    }
+
+    return pp::foundation::Status::invalid_argument("unknown document layer rename action");
+}
+
 inline void execute_document_layer_operation_side_effects(
    const DocumentLayerOperationPlan& plan,
    DocumentLayerOperationServices& services)
--- a/src/app_dialogs.cpp
+++ b/src/app_dialogs.cpp
@@ -50,6 +50,78 @@ namespace {
    return false;
 }

+struct LegacyActionLayerRename : public Action
+{
+    std::string m_old_name;
+    std::string m_new_name;
+    Layer* m_layer = nullptr;
+    std::shared_ptr<NodeLayer> m_layer_node;
+
+    LegacyActionLayerRename(
+        std::string old_name,
+        std::string new_name,
+        std::shared_ptr<NodeLayer> layer_node,
+        Layer* layer)
+        : m_old_name(std::move(old_name))
+        , m_new_name(std::move(new_name))
+        , m_layer(layer)
+        , m_layer_node(std::move(layer_node))
+    {
+    }
+
+    void run() override { }
+    size_t memory() override { return 0; }
+    void undo() override
+    {
+        if (m_layer_node)
+            m_layer_node->set_name(m_old_name.c_str());
+        if (m_layer)
+            m_layer->m_name = m_old_name;
+    }
+    Action* get_redo() override
+    {
+        return new LegacyActionLayerRename(m_new_name, m_old_name, m_layer_node, m_layer);
+    }
+};
+
+class LegacyDocumentLayerRenameServices final : public pp::app::DocumentLayerRenameServices {
+public:
+    LegacyDocumentLayerRenameServices(App& app, const std::shared_ptr<NodeDialogLayerRename>& dialog) noexcept
+        : app_(app)
+        , dialog_(dialog)
+    {
+    }
+
+    void rename_layer(std::string_view old_name, std::string_view new_name) override
+    {
+        if (!app_.layers || !app_.layers->m_current_layer || !app_.canvas || !app_.canvas->m_canvas)
+            return;
+
+        auto layer_node = std::static_pointer_cast<NodeLayer>(app_.layers->m_current_layer->shared_from_this());
+        auto* layer = app_.canvas->m_canvas->m_layers[app_.canvas->m_canvas->m_current_layer_idx].get();
+        const std::string old_name_copy(old_name);
+        const std::string new_name_copy(new_name);
+        ActionManager::add(new LegacyActionLayerRename(
+            old_name_copy,
+            new_name_copy,
+            layer_node,
+            layer));
+        layer_node->set_name(new_name_copy.c_str());
+        layer->m_name = new_name_copy;
+    }
+
+    void finish_layer_rename() override
+    {
+        if (dialog_)
+            dialog_->destroy();
+        app_.hideKeyboard();
+    }
+
+private:
+    App& app_;
+    std::shared_ptr<NodeDialogLayerRename> dialog_;
+};
+
 }

 std::shared_ptr<NodeProgressBar> App::show_progress(const std::string& title, int total /*= 0*/)
@@ -637,45 +709,10 @@ void App::dialog_layer_rename()
        const auto plan = pp::app::plan_document_layer_rename(old_name, dialog->get_name());
        if (!plan)
            return;
-        if (plan.value().action == pp::app::DocumentLayerRenameAction::no_op_same_name)
-        {
-            dialog->destroy();
-            App::I->hideKeyboard();
-            return;
-        }
-
-        struct ActionLayerRename : public Action
-        {
-            std::string m_old_name;
-            std::string m_new_name;
-            bool m_unsaved;
-            Layer* m_layer;
-            std::shared_ptr<NodeLayer> m_layer_node;
-            ActionLayerRename(std::string old_name, std::string new_name, std::shared_ptr<NodeLayer> layer_node, Layer* layer) :
-                m_old_name(old_name), m_new_name(new_name), m_layer_node(layer_node), m_layer(layer) { }
-            virtual void run() override { }
-            virtual size_t memory() override { return 0; }
-            virtual void undo() override
-            {
-                m_layer_node->set_name(m_old_name.c_str());
-                m_layer->m_name = m_old_name;
-            }
-            virtual Action* get_redo() override
-            {
-                return new ActionLayerRename(m_new_name, m_old_name, m_layer_node, m_layer);
-            }
-        };
-        auto layer_node = std::static_pointer_cast<NodeLayer>(layers->m_current_layer->shared_from_this());
-        auto* layer = canvas->m_canvas->m_layers[canvas->m_canvas->m_current_layer_idx].get();
-        ActionManager::add(new ActionLayerRename(
-            plan.value().old_name,
-            plan.value().new_name,
-            layer_node,
-            layer));
-        layer_node->set_name(plan.value().new_name.c_str());
-        layer->m_name = plan.value().new_name;
-        dialog->destroy();
-        App::I->hideKeyboard();
+        LegacyDocumentLayerRenameServices services(*this, dialog);
+        const auto status = pp::app::execute_document_layer_rename_plan(plan.value(), services);
+        if (!status.ok())
+            LOG("Layer rename failed: %s", status.message);
    };
    dialog->btn_cancel->on_click = [this, dialog](Node*)
    {
--- a/tests/app_core/document_layer_tests.cpp
+++ b/tests/app_core/document_layer_tests.cpp
@@ -31,6 +31,23 @@ public:
    int last_to_index = -1;
 };

+class FakeDocumentLayerRenameServices final : public pp::app::DocumentLayerRenameServices {
+public:
+    void rename_layer(std::string_view old_name, std::string_view new_name) override
+    {
+        rename_calls += 1;
+        last_old_name = std::string(old_name);
+        last_new_name = std::string(new_name);
+    }
+
+    void finish_layer_rename() override { finish_calls += 1; }
+
+    int rename_calls = 0;
+    int finish_calls = 0;
+    std::string last_old_name;
+    std::string last_new_name;
+};
+
 class FakeDocumentLayerOperationServices final : public pp::app::DocumentLayerOperationServices {
 public:
    void add_layer(std::string_view name, int insert_index) override
@@ -180,6 +197,49 @@ void layer_rename_rejects_overlong_name(pp::tests::Harness& harness)
    }
 }

+void layer_rename_executor_dispatches_changed_name(pp::tests::Harness& harness)
+{
+    FakeDocumentLayerRenameServices services;
+
+    const auto plan = pp::app::plan_document_layer_rename("Base", "Paint");
+    PP_EXPECT(harness, plan);
+    if (plan) {
+        PP_EXPECT(harness, pp::app::execute_document_layer_rename_plan(plan.value(), services).ok());
+        PP_EXPECT(harness, services.rename_calls == 1);
+        PP_EXPECT(harness, services.finish_calls == 1);
+        PP_EXPECT(harness, services.last_old_name == "Base");
+        PP_EXPECT(harness, services.last_new_name == "Paint");
+    }
+}
+
+void layer_rename_executor_finishes_no_op(pp::tests::Harness& harness)
+{
+    FakeDocumentLayerRenameServices services;
+
+    const auto plan = pp::app::plan_document_layer_rename("Ink", "Ink");
+    PP_EXPECT(harness, plan);
+    if (plan) {
+        PP_EXPECT(harness, pp::app::execute_document_layer_rename_plan(plan.value(), services).ok());
+        PP_EXPECT(harness, services.rename_calls == 0);
+        PP_EXPECT(harness, services.finish_calls == 1);
+    }
+}
+
+void layer_rename_executor_rejects_malformed_changed_plan(pp::tests::Harness& harness)
+{
+    FakeDocumentLayerRenameServices services;
+
+    pp::app::DocumentLayerRenamePlan malformed;
+    malformed.old_name = "Ink";
+    malformed.action = pp::app::DocumentLayerRenameAction::rename_and_record_undo;
+
+    const auto status = pp::app::execute_document_layer_rename_plan(malformed, services);
+    PP_EXPECT(harness, !status.ok());
+    PP_EXPECT(harness, status.code == pp::foundation::StatusCode::invalid_argument);
+    PP_EXPECT(harness, services.rename_calls == 0);
+    PP_EXPECT(harness, services.finish_calls == 0);
+}
+
 void layer_add_validates_insert_index_and_name(pp::tests::Harness& harness)
 {
    const auto plan = pp::app::plan_document_layer_add(2, 1, "Paint");
@@ -673,6 +733,9 @@ int main()
    harness.run("layer rename ignores unchanged name", layer_rename_ignores_unchanged_name);
    harness.run("layer rename rejects empty name", layer_rename_rejects_empty_name);
    harness.run("layer rename rejects overlong name", layer_rename_rejects_overlong_name);
+    harness.run("layer rename executor dispatches changed name", layer_rename_executor_dispatches_changed_name);
+    harness.run("layer rename executor finishes no op", layer_rename_executor_finishes_no_op);
+    harness.run("layer rename executor rejects malformed changed plan", layer_rename_executor_rejects_malformed_changed_plan);
    harness.run("layer add validates insert index and name", layer_add_validates_insert_index_and_name);
    harness.run("layer duplicate select and reorder validate indices", layer_duplicate_select_and_reorder_validate_indices);
    harness.run("layer remove keeps at least one layer", layer_remove_keeps_at_least_one_layer);