From f3834827b11d404b6bbbb7822c1e0e4a5dae9986 Mon Sep 17 00:00:00 2001 From: omigamedev Date: Sat, 6 Jun 2026 12:09:36 +0200 Subject: [PATCH] Move project save commit planning to app core --- docs/modernization/build-inventory.md | 11 +++- docs/modernization/debt.md | 10 ++++ docs/modernization/roadmap.md | 8 ++- src/app_core/document_canvas.h | 47 +++++++++++++++ src/canvas.cpp | 46 +++++++------- tests/CMakeLists.txt | 16 ++++- tests/app_core/document_canvas_tests.cpp | 76 ++++++++++++++++++++++++ tools/pano_cli/main.cpp | 28 ++++++++- 8 files changed, 214 insertions(+), 28 deletions(-) diff --git a/docs/modernization/build-inventory.md b/docs/modernization/build-inventory.md index f70d1fa..36e5a13 100644 --- a/docs/modernization/build-inventory.md +++ b/docs/modernization/build-inventory.md @@ -304,8 +304,10 @@ powershell -ExecutionPolicy Bypass -File scripts\automation\apple-remote-build.p serialization. The command also reports the app-core write-mode plan for direct first saves versus existing-target temporary writes, including the retained direct-write fallback when the temporary file cannot be opened, and - it is covered by forward-slash, Windows backslash, existing-target, and - invalid-path smokes. + the app-core commit plan for direct saves, successful temporary swaps, + target-remove failures, and rename-after-remove failures. It is covered by + forward-slash, Windows backslash, existing-target, remove-failure, + rename-failure, and invalid-path smokes. - Live equirectangular, layer, animation-frame, and cube-face export adapters now prepare and log the same payload-bearing canvas document snapshot plus shared paint-renderer export-readiness report. @@ -1186,7 +1188,10 @@ powershell -ExecutionPolicy Bypass -File scripts\automation\apple-remote-build.p app-core pure PPI save-writer route for payload-complete snapshots, log generated byte counts, and derive project-save target/tmp/timelapse paths plus direct-vs-temporary write-mode decisions through `pp_app_core` before - delegating to retained `Canvas::project_save`. + delegating to retained `Canvas::project_save`. Direct save, successful + temporary swap, remove-failure, and rename-after-remove commit outcomes are + now classified by the same app-core planner before retained success metadata + mutation. Retained legacy UI/canvas execution and actual live save serialization remain tracked by `DEBT-0040`, `DEBT-0041`, and `DEBT-0042`; the pure snapshot-to-PPI export handoff is diff --git a/docs/modernization/debt.md b/docs/modernization/debt.md index c8e42a8..4bad80d 100644 --- a/docs/modernization/debt.md +++ b/docs/modernization/debt.md @@ -549,6 +549,16 @@ agent or engineer to remove them without reconstructing context from chat. serialization, and `pano_cli plan-canvas-project-save-target` reports it in JSON. Actual PPI bytes, temporary-file swap execution, progress/threading, timelapse sidecar serialization, and app metadata mutation remain retained. +- 2026-06-06: DEBT-0040/DEBT-0042 were narrowed again. `pp_app_core` now owns + the retained project-save commit outcome policy for direct writes, successful + temporary swaps, target-remove failures, and rename-after-remove failures, + including an explicit `targetMayBeMissing` flag for failed swaps after the + original target was removed. Live `Canvas::project_save_thread` consumes that + result before retained success metadata mutation, and + `pano_cli plan-canvas-project-save-target` reports the same commit plan. + Actual PPI serialization, filesystem remove/rename execution, + progress/threading, timelapse sidecar serialization, and app metadata + mutation remain retained. - 2026-06-05: DEBT-0010/DEBT-0013 were narrowed again. `pp_app_core` now exports payload-complete or metadata-only canvas document snapshots through the pure `pp_document` PPI writer and rejects snapshots that still require diff --git a/docs/modernization/roadmap.md b/docs/modernization/roadmap.md index a8ca4f0..88f77ee 100644 --- a/docs/modernization/roadmap.md +++ b/docs/modernization/roadmap.md @@ -712,7 +712,10 @@ path planner for target, temporary PPI, and timelapse sidecar paths; live app-core boundary now also plans the retained save write mode, distinguishing direct first saves from existing-target temporary writes that swap into place and preserving the legacy direct-write fallback when the temporary file cannot -be opened. The same automation now feeds payload-complete snapshots through the +be opened. The same app-core boundary now also classifies the post-write +commit result for direct writes, successful temporary swaps, remove failures, +and rename-after-remove failures before retained save metadata mutation +continues. The same automation now feeds payload-complete snapshots through the shared `pp_paint_renderer::prepare_document_frame_export_readiness` report, which records renderer-neutral six-face texture upload commands and encodes the @@ -2320,6 +2323,9 @@ Results: It also reports the app-core write-mode plan for direct first saves versus existing-target temporary writes, including the retained fallback to direct target writes when the temporary file cannot be opened. + The command now reports the app-core commit plan for direct saves, successful + temporary swaps, target-remove failures, and rename-after-remove failures, + including whether the target may be missing after a failed swap. - The same payload-complete snapshot automation now uploads the active document frame through `pp_paint_renderer::upload_document_frame_faces` and the `RecordingRenderDevice`, emitting `rendererUpload` JSON with texture, diff --git a/src/app_core/document_canvas.h b/src/app_core/document_canvas.h index c8e4d12..2a1fda8 100644 --- a/src/app_core/document_canvas.h +++ b/src/app_core/document_canvas.h @@ -118,6 +118,23 @@ struct DocumentCanvasProjectSaveWritePlan { bool falls_back_to_direct_on_temporary_open_failure = false; }; +struct DocumentCanvasProjectSaveCommitInput { + bool used_temporary = false; + bool target_remove_attempted = false; + bool target_remove_succeeded = false; + bool temporary_rename_attempted = false; + bool temporary_rename_succeeded = false; +}; + +struct DocumentCanvasProjectSaveCommitPlan { + bool saved = false; + bool used_temporary = false; + bool target_removed = false; + bool temporary_renamed = false; + bool target_may_be_missing = false; + std::string_view log_message; +}; + class DocumentCanvasClearServices { public: virtual ~DocumentCanvasClearServices() = default; @@ -397,6 +414,36 @@ plan_document_canvas_project_save_write( return pp::foundation::Result::success(std::move(plan)); } +[[nodiscard]] constexpr DocumentCanvasProjectSaveCommitPlan plan_document_canvas_project_save_commit( + DocumentCanvasProjectSaveCommitInput input) noexcept +{ + DocumentCanvasProjectSaveCommitPlan plan; + plan.used_temporary = input.used_temporary; + + if (!input.used_temporary) { + plan.saved = true; + plan.log_message = "project saved to target"; + return plan; + } + + if (!input.target_remove_attempted || !input.target_remove_succeeded) { + plan.log_message = "could not remove target project before temporary swap"; + return plan; + } + + plan.target_removed = true; + if (!input.temporary_rename_attempted || !input.temporary_rename_succeeded) { + plan.target_may_be_missing = true; + plan.log_message = "temporary project not swapped after original removal"; + return plan; + } + + plan.saved = true; + plan.temporary_renamed = true; + plan.log_message = "temporary project swapped successfully"; + return plan; +} + [[nodiscard]] inline pp::foundation::Result plan_document_canvas_clear( bool has_canvas, float r = 0.0F, diff --git a/src/canvas.cpp b/src/canvas.cpp index e32ff68..e77bea2 100644 --- a/src/canvas.cpp +++ b/src/canvas.cpp @@ -2541,33 +2541,39 @@ bool Canvas::project_save_thread(std::string file_path, bool show_progress) fclose(fp); - bool success = false; + bool target_remove_attempted = false; + bool target_remove_succeeded = false; + bool temporary_rename_attempted = false; + bool temporary_rename_succeeded = false; if (use_tmp) { LOG("project saved tmp to %s", tmp_path.c_str()); LOG("swapping to %s", file_path.c_str()); - if (std::remove(file_path.c_str()) == 0) + target_remove_attempted = true; + target_remove_succeeded = std::remove(file_path.c_str()) == 0; + if (target_remove_succeeded) { - if (std::rename(tmp_path.c_str(), file_path.c_str()) == 0) - { - success = true; - LOG("tmp file swapped succesfully"); - } - else - { - success = false; - LOG("tmp file NOT swapped, original removed"); - } - } - else - { - success = false; - LOG("could not remove %s", file_path.c_str()); + temporary_rename_attempted = true; + temporary_rename_succeeded = std::rename(tmp_path.c_str(), file_path.c_str()) == 0; } } - else - { - success = true; + + const auto commit_plan = pp::app::plan_document_canvas_project_save_commit( + pp::app::DocumentCanvasProjectSaveCommitInput { + .used_temporary = use_tmp, + .target_remove_attempted = target_remove_attempted, + .target_remove_succeeded = target_remove_succeeded, + .temporary_rename_attempted = temporary_rename_attempted, + .temporary_rename_succeeded = temporary_rename_succeeded, + }); + const bool success = commit_plan.saved; + if (commit_plan.saved && commit_plan.temporary_renamed) { + LOG("tmp file swapped succesfully"); + } else if (!commit_plan.saved && commit_plan.target_may_be_missing) { + LOG("tmp file NOT swapped, original removed"); + } else if (!commit_plan.saved && commit_plan.used_temporary) { + LOG("could not remove %s", file_path.c_str()); + } else if (commit_plan.saved) { LOG("project saved to %s", file_path.c_str()); } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b18371c..ddd3c07 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -1530,7 +1530,7 @@ if(TARGET pano_cli) COMMAND pano_cli plan-canvas-project-save-target --data-dir D:/Paint/data --path D:/Paint/projects/demo.ppi) set_tests_properties(pano_cli_plan_canvas_project_save_target_smoke PROPERTIES LABELS "app;document;integration;desktop-fast" - PASS_REGULAR_EXPRESSION "\"command\":\"plan-canvas-project-save-target\".*\"dataDirectory\":\"D:/Paint/data\".*\"targetPath\":\"D:/Paint/projects/demo.ppi\".*\"fileName\":\"demo\".*\"temporaryPath\":\"D:/Paint/data/demo.tmp.ppi\".*\"timelapsePath\":\"D:/Paint/data/demo.pptl\".*\"writePlan\":\\{\"action\":\"write-direct-to-target\",\"targetExists\":false,\"usesTemporary\":false,\"writePath\":\"D:/Paint/projects/demo.ppi\",\"fallbackDirectOnTemporaryOpenFailure\":false\\}") + PASS_REGULAR_EXPRESSION "\"command\":\"plan-canvas-project-save-target\".*\"dataDirectory\":\"D:/Paint/data\".*\"targetPath\":\"D:/Paint/projects/demo.ppi\".*\"fileName\":\"demo\".*\"temporaryPath\":\"D:/Paint/data/demo.tmp.ppi\".*\"timelapsePath\":\"D:/Paint/data/demo.pptl\".*\"writePlan\":\\{\"action\":\"write-direct-to-target\",\"targetExists\":false,\"usesTemporary\":false,\"writePath\":\"D:/Paint/projects/demo.ppi\",\"fallbackDirectOnTemporaryOpenFailure\":false\\}.*\"commitPlan\":\\{\"saved\":true,\"usedTemporary\":false,\"targetRemoved\":false,\"temporaryRenamed\":false,\"targetMayBeMissing\":false,\"message\":\"project saved to target\"\\}") add_test(NAME pano_cli_plan_canvas_project_save_target_backslash_smoke COMMAND pano_cli plan-canvas-project-save-target --data-dir D:/Paint/data --path "D:\\Paint\\projects\\demo.ppi") @@ -1542,7 +1542,19 @@ if(TARGET pano_cli) COMMAND pano_cli plan-canvas-project-save-target --data-dir D:/Paint/data --path D:/Paint/projects/demo.ppi --target-exists) set_tests_properties(pano_cli_plan_canvas_project_save_target_existing_smoke PROPERTIES LABELS "app;document;integration;desktop-fast" - PASS_REGULAR_EXPRESSION "\"command\":\"plan-canvas-project-save-target\".*\"targetPath\":\"D:/Paint/projects/demo.ppi\".*\"writePlan\":\\{\"action\":\"write-temporary-then-swap\",\"targetExists\":true,\"usesTemporary\":true,\"writePath\":\"D:/Paint/data/demo.tmp.ppi\",\"fallbackDirectOnTemporaryOpenFailure\":true\\}") + PASS_REGULAR_EXPRESSION "\"command\":\"plan-canvas-project-save-target\".*\"targetPath\":\"D:/Paint/projects/demo.ppi\".*\"writePlan\":\\{\"action\":\"write-temporary-then-swap\",\"targetExists\":true,\"usesTemporary\":true,\"writePath\":\"D:/Paint/data/demo.tmp.ppi\",\"fallbackDirectOnTemporaryOpenFailure\":true\\}.*\"commitPlan\":\\{\"saved\":true,\"usedTemporary\":true,\"targetRemoved\":true,\"temporaryRenamed\":true,\"targetMayBeMissing\":false,\"message\":\"temporary project swapped successfully\"\\}") + + add_test(NAME pano_cli_plan_canvas_project_save_target_remove_failure_smoke + COMMAND pano_cli plan-canvas-project-save-target --data-dir D:/Paint/data --path D:/Paint/projects/demo.ppi --target-exists --remove-fails) + set_tests_properties(pano_cli_plan_canvas_project_save_target_remove_failure_smoke PROPERTIES + LABELS "app;document;integration;desktop-fast;fuzz" + PASS_REGULAR_EXPRESSION "\"command\":\"plan-canvas-project-save-target\".*\"commitPlan\":\\{\"saved\":false,\"usedTemporary\":true,\"targetRemoved\":false,\"temporaryRenamed\":false,\"targetMayBeMissing\":false,\"message\":\"could not remove target project before temporary swap\"\\}") + + add_test(NAME pano_cli_plan_canvas_project_save_target_rename_failure_smoke + COMMAND pano_cli plan-canvas-project-save-target --data-dir D:/Paint/data --path D:/Paint/projects/demo.ppi --target-exists --rename-fails) + set_tests_properties(pano_cli_plan_canvas_project_save_target_rename_failure_smoke PROPERTIES + LABELS "app;document;integration;desktop-fast;fuzz" + PASS_REGULAR_EXPRESSION "\"command\":\"plan-canvas-project-save-target\".*\"commitPlan\":\\{\"saved\":false,\"usedTemporary\":true,\"targetRemoved\":true,\"temporaryRenamed\":false,\"targetMayBeMissing\":true,\"message\":\"temporary project not swapped after original removal\"\\}") add_test(NAME pano_cli_plan_canvas_project_save_target_rejects_empty_path COMMAND pano_cli plan-canvas-project-save-target --path "") diff --git a/tests/app_core/document_canvas_tests.cpp b/tests/app_core/document_canvas_tests.cpp index ce42e85..4cd9585 100644 --- a/tests/app_core/document_canvas_tests.cpp +++ b/tests/app_core/document_canvas_tests.cpp @@ -353,6 +353,78 @@ void project_save_write_plan_rejects_missing_paths(pp::tests::Harness& harness) PP_EXPECT(harness, missing_temporary.status().code == pp::foundation::StatusCode::invalid_argument); } +void project_save_commit_plan_succeeds_for_direct_write(pp::tests::Harness& harness) +{ + const auto plan = pp::app::plan_document_canvas_project_save_commit( + pp::app::DocumentCanvasProjectSaveCommitInput { + .used_temporary = false, + }); + + PP_EXPECT(harness, plan.saved); + PP_EXPECT(harness, !plan.used_temporary); + PP_EXPECT(harness, !plan.target_removed); + PP_EXPECT(harness, !plan.temporary_renamed); + PP_EXPECT(harness, !plan.target_may_be_missing); + PP_EXPECT(harness, plan.log_message == "project saved to target"); +} + +void project_save_commit_plan_succeeds_for_temporary_swap(pp::tests::Harness& harness) +{ + const auto plan = pp::app::plan_document_canvas_project_save_commit( + pp::app::DocumentCanvasProjectSaveCommitInput { + .used_temporary = true, + .target_remove_attempted = true, + .target_remove_succeeded = true, + .temporary_rename_attempted = true, + .temporary_rename_succeeded = true, + }); + + PP_EXPECT(harness, plan.saved); + PP_EXPECT(harness, plan.used_temporary); + PP_EXPECT(harness, plan.target_removed); + PP_EXPECT(harness, plan.temporary_renamed); + PP_EXPECT(harness, !plan.target_may_be_missing); + PP_EXPECT(harness, plan.log_message == "temporary project swapped successfully"); +} + +void project_save_commit_plan_fails_when_target_remove_fails(pp::tests::Harness& harness) +{ + const auto plan = pp::app::plan_document_canvas_project_save_commit( + pp::app::DocumentCanvasProjectSaveCommitInput { + .used_temporary = true, + .target_remove_attempted = true, + .target_remove_succeeded = false, + .temporary_rename_attempted = false, + .temporary_rename_succeeded = false, + }); + + PP_EXPECT(harness, !plan.saved); + PP_EXPECT(harness, plan.used_temporary); + PP_EXPECT(harness, !plan.target_removed); + PP_EXPECT(harness, !plan.temporary_renamed); + PP_EXPECT(harness, !plan.target_may_be_missing); + PP_EXPECT(harness, plan.log_message == "could not remove target project before temporary swap"); +} + +void project_save_commit_plan_flags_missing_target_after_rename_failure(pp::tests::Harness& harness) +{ + const auto plan = pp::app::plan_document_canvas_project_save_commit( + pp::app::DocumentCanvasProjectSaveCommitInput { + .used_temporary = true, + .target_remove_attempted = true, + .target_remove_succeeded = true, + .temporary_rename_attempted = true, + .temporary_rename_succeeded = false, + }); + + PP_EXPECT(harness, !plan.saved); + PP_EXPECT(harness, plan.used_temporary); + PP_EXPECT(harness, plan.target_removed); + PP_EXPECT(harness, !plan.temporary_renamed); + PP_EXPECT(harness, plan.target_may_be_missing); + PP_EXPECT(harness, plan.log_message == "temporary project not swapped after original removal"); +} + void snapshot_plan_rejects_invalid_canvas_state(pp::tests::Harness& harness) { const std::uint32_t frames[] { 100U }; @@ -540,6 +612,10 @@ int main() harness.run("project save write plan writes direct for new targets", project_save_write_plan_writes_direct_for_new_targets); harness.run("project save write plan prefers temporary for existing targets", project_save_write_plan_prefers_temporary_for_existing_targets); harness.run("project save write plan rejects missing paths", project_save_write_plan_rejects_missing_paths); + harness.run("project save commit plan succeeds for direct write", project_save_commit_plan_succeeds_for_direct_write); + harness.run("project save commit plan succeeds for temporary swap", project_save_commit_plan_succeeds_for_temporary_swap); + harness.run("project save commit plan fails when target remove fails", project_save_commit_plan_fails_when_target_remove_fails); + harness.run("project save commit plan flags missing target after rename failure", project_save_commit_plan_flags_missing_target_after_rename_failure); harness.run("snapshot plan rejects invalid canvas state", snapshot_plan_rejects_invalid_canvas_state); harness.run("clear plan records legacy canvas effects", clear_plan_records_legacy_canvas_effects); harness.run("clear plan noops without canvas", clear_plan_noops_without_canvas); diff --git a/tools/pano_cli/main.cpp b/tools/pano_cli/main.cpp index 2a40524..3967d4b 100644 --- a/tools/pano_cli/main.cpp +++ b/tools/pano_cli/main.cpp @@ -414,6 +414,8 @@ struct PlanCanvasProjectSaveTargetArgs { std::string data_directory = "D:/Paint/data"; std::string target_path = "D:/Paint/projects/demo.ppi"; bool target_exists = false; + bool remove_fails = false; + bool rename_fails = false; }; struct PlanCanvasDocumentSnapshotArgs { @@ -2560,7 +2562,7 @@ void print_help() << " plan-canvas-view-density [--density N] [--bad-float]\n" << " plan-canvas-view-cursor-mode [--mode N]\n" << " plan-canvas-cursor [--mode draw|erase|line|camera|grid|copy|cut|fill|mask-free|mask-line|bucket] [--visibility never|small-brush|not-painting|always] [--brush-size N] [--no-brush] [--drawing] [--alt] [--resizing] [--picking] [--bad-size]\n" - << " plan-canvas-project-save-target [--data-dir DIR] [--path FILE] [--target-exists]\n" + << " plan-canvas-project-save-target [--data-dir DIR] [--path FILE] [--target-exists] [--remove-fails] [--rename-fails]\n" << " plan-grid-operation --kind pick|load|reload|clear|render|commit [--path FILE] [--no-heightmap] [--no-canvas] [--float32] [--float16] [--texture-resolution N] [--samples N]\n" << " plan-history-operation --kind undo|redo|clear [--undo-count N] [--redo-count N] [--memory-bytes N]\n" << " plan-main-toolbar --command open|save|undo|redo|clear-history|clear-canvas|message-box|settings [--undo-count N] [--redo-count N] [--memory-bytes N] [--no-canvas]\n" @@ -6091,6 +6093,10 @@ pp::foundation::Status parse_plan_canvas_project_save_target_args( } } else if (key == "--target-exists") { args.target_exists = true; + } else if (key == "--remove-fails") { + args.remove_fails = true; + } else if (key == "--rename-fails") { + args.rename_fails = true; } else { return pp::foundation::Status::invalid_argument("unknown option"); } @@ -6126,6 +6132,18 @@ int plan_canvas_project_save_target(int argc, char** argv) const auto& value = plan.value(); const auto& write_value = write_plan.value(); + const bool target_remove_attempted = write_value.uses_temporary; + const bool target_remove_succeeded = target_remove_attempted && !args.remove_fails; + const bool temporary_rename_attempted = target_remove_succeeded; + const bool temporary_rename_succeeded = temporary_rename_attempted && !args.rename_fails; + const auto commit_plan = pp::app::plan_document_canvas_project_save_commit( + pp::app::DocumentCanvasProjectSaveCommitInput { + .used_temporary = write_value.uses_temporary, + .target_remove_attempted = target_remove_attempted, + .target_remove_succeeded = target_remove_succeeded, + .temporary_rename_attempted = temporary_rename_attempted, + .temporary_rename_succeeded = temporary_rename_succeeded, + }); std::cout << "{\"ok\":true,\"command\":\"plan-canvas-project-save-target\"" << ",\"dataDirectory\":\"" << json_escape(args.data_directory) << "\",\"targetPath\":\"" << json_escape(value.target_path) @@ -6139,7 +6157,13 @@ int plan_canvas_project_save_target(int argc, char** argv) << ",\"writePath\":\"" << json_escape(write_value.write_path) << "\",\"fallbackDirectOnTemporaryOpenFailure\":" << json_bool(write_value.falls_back_to_direct_on_temporary_open_failure) - << "}" + << "},\"commitPlan\":{\"saved\":" << json_bool(commit_plan.saved) + << ",\"usedTemporary\":" << json_bool(commit_plan.used_temporary) + << ",\"targetRemoved\":" << json_bool(commit_plan.target_removed) + << ",\"temporaryRenamed\":" << json_bool(commit_plan.temporary_renamed) + << ",\"targetMayBeMissing\":" << json_bool(commit_plan.target_may_be_missing) + << ",\"message\":\"" << json_escape(std::string(commit_plan.log_message)) + << "\"}" << "}\n"; return 0; }