diff --git a/docs/modernization/capability-map.md b/docs/modernization/capability-map.md index 7ce5d54..6652767 100644 --- 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 | diff --git a/docs/modernization/debt.md b/docs/modernization/debt.md index 67d55f3..9ef9e18 100644 --- 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 | diff --git a/docs/modernization/roadmap.md b/docs/modernization/roadmap.md index 9b28bdd..d305444 100644 --- 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 diff --git a/src/app_core/document_layer.h b/src/app_core/document_layer.h index 4c0dd64..9765bcf 100644 --- 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::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) diff --git a/src/app_dialogs.cpp b/src/app_dialogs.cpp index 5708aef..08d25f4 100644 --- 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 m_layer_node; + + LegacyActionLayerRename( + std::string old_name, + std::string new_name, + std::shared_ptr 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& 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(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 dialog_; +}; + } std::shared_ptr 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 m_layer_node; - ActionLayerRename(std::string old_name, std::string new_name, std::shared_ptr 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(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*) { diff --git a/tests/app_core/document_layer_tests.cpp b/tests/app_core/document_layer_tests.cpp index 6677f83..a4c4142 100644 --- 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);