From 565564c061b9337b4ae6890225ba7904325a9ad6 Mon Sep 17 00:00:00 2001 From: omigamedev Date: Mon, 15 Jun 2026 19:26:11 +0200 Subject: [PATCH] Narrow retained UI overlay lifetime debt --- docs/modernization/debt.md | 2 +- src/legacy_ui_overlay_services.cpp | 160 ++++++++++++++ src/legacy_ui_overlay_services.h | 58 +++++ src/node_dialog_browse.cpp | 21 +- src/node_dialog_open.cpp | 23 +- src/node_panel_brush.cpp | 30 ++- src/node_panel_color.cpp | 10 +- src/node_panel_grid.cpp | 10 +- src/node_panel_layer.cpp | 266 +++++++++++++++++++---- src/node_panel_layer.h | 4 +- src/node_popup_menu.cpp | 7 +- src/node_popup_menu.h | 1 + src/ui_core/node_lifetime.h | 6 + tests/ui_core/node_lifetime_tests.cpp | 41 ++++ tests/ui_core/overlay_lifetime_tests.cpp | 31 +++ 15 files changed, 603 insertions(+), 67 deletions(-) diff --git a/docs/modernization/debt.md b/docs/modernization/debt.md index 70276153..d4cd147e 100644 --- a/docs/modernization/debt.md +++ b/docs/modernization/debt.md @@ -1817,7 +1817,7 @@ agent or engineer to remove them without reconstructing context from chat. | DEBT-0055 | Open | Modernization | `src/app.h` now forward-declares retained iOS/macOS/Android/Linux/Web platform handles instead of including platform SDK headers, and full SDK includes are isolated in `src/platform_legacy/legacy_platform_services.cpp`, but the `App` singleton still stores those platform handles directly | Reduce central header platform coupling incrementally without rewriting non-Windows platform entrypoints before Phase 6 | Windows app build; Apple/Android/Linux/Web package smoke once platform root builds are active | Platform handles are owned by injected `pp_platform_*` shell state or services, and `App` has no platform SDK handle fields or platform conditional members | | DEBT-0056 | Open | Modernization | `src/asset.h` is now Android-SDK-free and uses opaque Android asset handles behind `Asset::set_android_asset_manager`, but retained `Asset` still owns a static Android asset-manager bridge and `src/asset.cpp` still performs Android `AAssetManager` reads directly; the current `android-arm64` root preset is headless and does not expose `pp_legacy_assets_io`, though the retained Android standard package `native-lib` now builds through its refreshed C++23 CMake path | Reduce legacy asset I/O header coupling without rewriting Android asset loading before the asset manager/storage boundary exists | Windows app build; `powershell -ExecutionPolicy Bypass -File scripts\automation\platform-build.ps1 -Presets android-arm64 -Targets pp_assets`; `powershell -ExecutionPolicy Bypass -File scripts\automation\android-legacy-package-build.ps1 -Packages standard` | Android asset loading is owned by injected asset storage/platform services or `pp_assets` file providers, with no static Android asset manager on `Asset` | | DEBT-0061 | Open | Modernization | Desktop XR runtime selection now lives in tested `pp_platform_api` policy and prefers OpenXR, but `WindowsPlatformServices` still reports OpenXR unavailable and reaches the retained OpenVR SDK bridge as a legacy fallback; Windows runtime deployment copies `openvr_api.dll` beside `PanoPainter.exe` until that fallback is removed | Preserve current desktop VR behavior while replacing OpenVR with OpenXR behind the platform/renderer boundary | `pp_platform_api_tests`; `ctest --preset desktop-fast --build-config Debug -R pp_platform_api_tests --output-on-failure`; `cmake --build --preset windows-msvc-default --config Debug --target PanoPainter` | Add an OpenXR SDK/package target, implement desktop OpenXR startup/shutdown/pose/controller submission behind `pp_platform_vr` or `PlatformServices`, validate parity with mocked/runtime smoke coverage, and remove `libs/openvr` plus the OpenVR link/include paths from root CMake | -| DEBT-0063 | Open | Modernization | `pp_ui_core` now owns tested `NodeLifetimeTree` and `UiOverlayLifetime` models for checked handles, scoped callback connections, subtree destruction, capture release, and mutation-safe dispatch. `src/node_panel_layer.h/.cpp`, `src/node_combobox.cpp`, `src/node_dialog_open.cpp`, `src/node_dialog_browse.cpp`, `src/node_panel_stroke.cpp`, `src/node_panel_brush.cpp`, `src/node_dialog_picker.cpp`, `src/legacy_quick_ui_services.cpp`, and `src/node_popup_menu.h/.cpp` now route popup/dialog/menu lifetime through checked overlay handles and handle-based close; migration remains open in other legacy panel/dialog families | Preserve current UI behavior while completing safe panel/dialog lifetime migration incrementally | `pp_ui_core_layout_xml_tests`; `pp_ui_core_node_lifetime_tests`; `pp_ui_core_overlay_lifetime_tests`; `tests/ui_core/node_lifetime_tests.cpp:destroy_subtree_clears_child_connections`; `tests/ui_core/overlay_lifetime_tests.cpp:double_close_overlay_returns_invalid_argument`; `ctest --preset desktop-fast --build-config Debug`; `cmake --build --preset windows-msvc-default --config Debug --target panopainter_app pp_ui_core_node_lifetime_tests pp_ui_core_overlay_lifetime_tests` | Remaining legacy popup/dialog families still use non-handle ownership and open lifetimes; migration stays open until their surfaces are converted, including lifecycle safety parity checks | +| DEBT-0063 | Open | Modernization | `pp_ui_core` now owns tested `NodeLifetimeTree` and `UiOverlayLifetime` models for checked handles, scoped callback connections, subtree destruction, capture release, and mutation-safe dispatch. `src/legacy_ui_overlay_services.*` now keeps a root-scoped checked-overlay registry for retained nodes, and `src/node_panel_layer.h/.cpp`, `src/node_combobox.cpp`, `src/node_dialog_open.cpp`, `src/node_dialog_browse.cpp`, `src/node_panel_stroke.cpp`, `src/node_panel_brush.cpp`, `src/node_panel_color.cpp`, `src/node_panel_grid.cpp`, `src/node_dialog_picker.cpp`, `src/legacy_quick_ui_services.cpp`, and `src/node_popup_menu.h/.cpp` now route popup/dialog/menu lifetime through checked overlay handles or handle-based close instead of raw attach-and-destroy callbacks. Layer-panel selection/order state also stays on shared ownership rather than raw child pointers while mutation-safe close remains open in other legacy panel/dialog families | Preserve current UI behavior while completing safe panel/dialog lifetime migration incrementally | `pp_ui_core_layout_xml_tests`; `pp_ui_core_node_lifetime_tests`; `pp_ui_core_overlay_lifetime_tests`; `tests/ui_core/node_lifetime_tests.cpp:destroy_subtree_clears_child_connections`; `tests/ui_core/overlay_lifetime_tests.cpp:double_close_overlay_returns_invalid_argument`; `ctest --preset desktop-fast --build-config Debug`; `cmake --build --preset windows-msvc-default --config Debug --target panopainter_app pp_ui_core_node_lifetime_tests pp_ui_core_overlay_lifetime_tests` | Remaining legacy popup/dialog families still use non-handle ownership and open lifetimes; migration stays open until their surfaces are converted, including lifecycle safety parity checks | | DEBT-0057 | Open | Modernization | Default canvas allocation size now dispatches through `PlatformServices::default_canvas_resolution`, removing the `CANVAS_RES` platform macro from `src/canvas.h`; WebGL's retained 512 default now lives in tested `pp_platform_api` policy behind injectable `pp::platform::WebPlatformServices`, but the Web shell still reaches the default implementation through the retained fallback until a dedicated Web service is injected directly | Preserve WebGL memory behavior while moving canvas creation policy out of shared canvas headers and into the platform boundary | `pp_platform_api_tests`; `ctest --preset desktop-fast --build-config Debug -R pp_platform_api_tests`; Windows app build; WebGL package smoke once root Web build exists | Default canvas resolution is owned by injected `pp_platform_*` services for every supported platform, with no WebGL branch in the legacy fallback | | DEBT-0058 | Open | Modernization | App-level progress/message/input dialog metadata, including message-dialog OK/cancel captions, now consumes pure `pp_app_core` through `App::show_progress`, `App::message_box`, `App::input_box`, `pano_cli plan-app-dialog`, and `pp_app_core_app_dialog_tests`; live execution is centralized in `src/legacy_app_dialog_services.*`, retained root insertion now routes through `src/legacy_ui_overlay_services.*`, and whats-new dialog state persistence routes through `src/legacy_preference_storage.*`, but the bridge still creates retained `NodeProgressBar`, `NodeMessageBox`, and `NodeInputBox` instances with raw callback/lifetime ownership | Preserve current app-shell dialog behavior while moving shared dialog policy toward UI/app services | `pp_app_core_app_dialog_tests`; `pano_cli plan-app-dialog --kind progress --total -4`; `pano_cli plan-app-dialog --kind message --cancel`; `pano_cli plan-app-dialog --kind input --ok-caption Save`; `ctest --preset desktop-fast --build-config Debug`; Windows app build | Progress/message/input dialog creation, callback wiring, layout insertion, lifetime ownership, and headless automation are owned by injected app/UI services with `App` methods acting only as adapters | | DEBT-0059 | Open | Modernization | iOS root CMake headless builds assign generated bundle identifiers and disable code signing for executable test/tool targets | The current Apple gate is compile validation for shared component targets; signed iOS app/package validation is not migrated to root CMake yet | `powershell -ExecutionPolicy Bypass -File scripts\automation\apple-remote-build.ps1 -Presets macos,ios-simulator,ios-device`; `sh scripts/automation/platform-build.sh "ios-device"` on `panopainter-mac` | Root CMake owns the signed Apple app/package targets, package-smoke validates Apple bundles where signing material is available, and headless iOS test/tool targets are either excluded from signed package builds or use explicit test-runner signing policy | diff --git a/src/legacy_ui_overlay_services.cpp b/src/legacy_ui_overlay_services.cpp index 9154e022..e5cf5c02 100644 --- a/src/legacy_ui_overlay_services.cpp +++ b/src/legacy_ui_overlay_services.cpp @@ -8,8 +8,86 @@ #include "node_popup_menu.h" #include "node_progress_bar.h" +#include +#include + namespace pp::panopainter { +namespace { + +struct LegacyOverlayContext { + pp::ui::NodeHandle root_handle {}; + pp::ui::NodeLifetimeTree tree {}; + std::unique_ptr overlays {}; + std::unordered_map overlay_handles {}; +}; + +using LegacyOverlayContextMap = std::unordered_map; + +LegacyOverlayContextMap& overlay_contexts() +{ + static LegacyOverlayContextMap contexts {}; + return contexts; +} + +pp::foundation::Result get_overlay_context(Node& anchor, bool create_if_missing) noexcept +{ + auto* root = anchor.root(); + if (!root) { + return pp::foundation::Result::failure( + pp::foundation::Status::invalid_argument("legacy overlay root is missing")); + } + + auto& contexts = overlay_contexts(); + auto it = contexts.find(root); + if (it == contexts.end()) { + if (!create_if_missing) { + return pp::foundation::Result::failure( + pp::foundation::Status::invalid_argument("legacy overlay registry is missing")); + } + it = contexts.try_emplace(root, LegacyOverlayContext {}).first; + } + + auto& context = it->second; + if (!context.overlays) { + const auto root_handle = context.tree.create_root(); + if (!root_handle) { + return pp::foundation::Result::failure(root_handle.status()); + } + + context.root_handle = root_handle.value(); + context.overlays = std::make_unique(context.tree, context.root_handle); + } + + return pp::foundation::Result::success(&context); +} + +Node* overlay_node_for_handle( + const LegacyOverlayContext& context, + pp::ui::NodeHandle overlay) noexcept +{ + for (auto& entry : context.overlay_handles) { + if (entry.second == overlay) { + return entry.first; + } + } + return nullptr; +} + +void forget_overlay_by_handle(LegacyOverlayContext& context, pp::ui::NodeHandle overlay) noexcept +{ + for (auto it = context.overlay_handles.begin(); it != context.overlay_handles.end();) { + if (it->second == overlay) { + it = context.overlay_handles.erase(it); + break; + } else { + ++it; + } + } +} + +} // namespace + void initialize_legacy_overlay_node(App& app, Node& node) { node.set_manager(&app.layout); @@ -30,6 +108,17 @@ void detach_legacy_node_from_parent(Node& node) void close_legacy_dialog_node(Node& node) { + const auto context = get_overlay_context(node, false); + if (context) { + auto it = context.value()->overlay_handles.find(&node); + if (it != context.value()->overlay_handles.end()) { + const auto status = close_legacy_overlay_node(node, it->second); + if (status.ok()) { + return; + } + context.value()->overlay_handles.erase(it); + } + } destroy_legacy_node(node); } @@ -57,6 +146,26 @@ void close_legacy_popup_overlay(Node& node) noexcept destroy_legacy_node(node); } +void close_legacy_overlay_handle_ignoring_status( + Node& anchor, + pp::ui::NodeHandle overlay) noexcept +{ + (void)close_legacy_overlay_node(anchor, overlay); +} + +void close_legacy_overlay_handles_if_open( + Node& anchor, + const pp::foundation::Result& popup_overlay, + const pp::foundation::Result& tick_overlay) noexcept +{ + if (popup_overlay) { + close_legacy_overlay_handle_ignoring_status(anchor, popup_overlay.value()); + } + if (tick_overlay) { + close_legacy_overlay_handle_ignoring_status(anchor, tick_overlay.value()); + } +} + void close_legacy_dialog_and_hide_keyboard(App& app, Node& node) { close_legacy_dialog_node(node); @@ -110,6 +219,57 @@ pp::foundation::Status attach_legacy_overlay_node_to_root( return pp::foundation::Status::success(); } +pp::foundation::Result open_legacy_overlay_node_with_handle( + Node& anchor, + const std::shared_ptr& node, + bool modal) noexcept +{ + if (!node) { + return pp::foundation::Result::failure( + pp::foundation::Status::invalid_argument("legacy overlay node is null")); + } + + const auto context = get_overlay_context(anchor, true); + if (!context) { + return pp::foundation::Result::failure( + context.status()); + } + + auto overlay = context.value()->overlays->open_dialog(modal); + if (!overlay) { + return pp::foundation::Result::failure(overlay.status()); + } + + const auto status = attach_legacy_overlay_node_to_root(anchor, node); + if (!status.ok()) { + (void)context.value()->overlays->close(overlay.value()); + return pp::foundation::Result::failure(status); + } + + context.value()->overlay_handles[node.get()] = overlay.value(); + return pp::foundation::Result::success(overlay.value()); +} + +pp::foundation::Status close_legacy_overlay_node(Node& anchor, pp::ui::NodeHandle overlay) noexcept +{ + const auto context = get_overlay_context(anchor, false); + if (!context) { + return pp::foundation::Status::invalid_argument("legacy overlay registry is missing"); + } + + const auto status = context.value()->overlays->close(overlay); + if (!status.ok()) { + forget_overlay_by_handle(*context.value(), overlay); + return status; + } + + if (auto* raw_overlay = overlay_node_for_handle(*context.value(), overlay)) { + destroy_legacy_node(*raw_overlay); + } + forget_overlay_by_handle(*context.value(), overlay); + return status; +} + pp::foundation::Result> add_legacy_popup_menu( App& app, const char* template_id, diff --git a/src/legacy_ui_overlay_services.h b/src/legacy_ui_overlay_services.h index ba87d47c..6886b2d9 100644 --- a/src/legacy_ui_overlay_services.h +++ b/src/legacy_ui_overlay_services.h @@ -2,7 +2,9 @@ #include "app_core/app_dialog.h" #include "foundation/result.h" +#include "ui_core/overlay_lifetime.h" #include "node.h" +#include "node_image.h" #include "shader.h" #include @@ -104,6 +106,13 @@ void release_legacy_mouse_capture(Node& node) noexcept; void configure_legacy_popup_overlay(Node& node) noexcept; void activate_legacy_popup_overlay(Node& node) noexcept; void close_legacy_popup_overlay(Node& node) noexcept; +void close_legacy_overlay_handle_ignoring_status( + Node& anchor, + pp::ui::NodeHandle overlay) noexcept; +void close_legacy_overlay_handles_if_open( + Node& anchor, + const pp::foundation::Result& popup_overlay, + const pp::foundation::Result& tick_overlay) noexcept; void close_legacy_dialog_and_hide_keyboard(App& app, Node& node); void close_legacy_popup_panel( Node& node, @@ -117,6 +126,15 @@ void close_legacy_popup_panel( Node& anchor, const std::shared_ptr& node) noexcept; +[[nodiscard]] pp::foundation::Result open_legacy_overlay_node_with_handle( + Node& anchor, + const std::shared_ptr& node, + bool modal = true) noexcept; + +[[nodiscard]] pp::foundation::Status close_legacy_overlay_node( + Node& anchor, + pp::ui::NodeHandle overlay) noexcept; + [[nodiscard]] pp::foundation::Result> add_legacy_popup_menu( App& app, const char* template_id, @@ -163,6 +181,46 @@ std::shared_ptr add_legacy_overlay_node(App& app) return node; } +template +concept LegacyPopupOverlay = requires(PopupT& popup, const std::function& close_cb) { + popup.on_popup_close = close_cb; +}; + +template +bool open_popup_and_tick_overlay( + Node& anchor, + const std::shared_ptr& popup, + const glm::vec2& tick_pos, + float button_height) noexcept +{ + if (!popup) { + return false; + } + + auto tick = make_legacy_overlay_node_for_anchor(anchor); + tick->SetPositioning(YGPositionTypeAbsolute); + tick->SetSize(16, 32); + tick->SetPosition(tick_pos.x, tick_pos.y + (button_height - 32) * 0.5f); + tick->set_image("data/ui/popup-tick.png"); + tick->m_scale = { 1, 1 }; + + const auto popup_overlay = open_legacy_overlay_node_with_handle(anchor, popup); + const auto tick_overlay = open_legacy_overlay_node_with_handle(anchor, tick); + if (!popup_overlay || !tick_overlay) { + close_legacy_overlay_handles_if_open(anchor, popup_overlay, tick_overlay); + return false; + } + + const auto popup_handle = popup_overlay.value(); + const auto tick_handle = tick_overlay.value(); + popup->on_popup_close = [&anchor, popup_handle, tick_handle](Node*) { + close_legacy_overlay_handle_ignoring_status(anchor, popup_handle); + close_legacy_overlay_handle_ignoring_status(anchor, tick_handle); + }; + + return true; +} + template void bind_legacy_popup_close_destroys_overlay( PopupT& popup, diff --git a/src/node_dialog_browse.cpp b/src/node_dialog_browse.cpp index 90f80b0d..b217c061 100644 --- a/src/node_dialog_browse.cpp +++ b/src/node_dialog_browse.cpp @@ -30,7 +30,6 @@ void NodeDialogBrowse::init_controls() { btn_ok = find("btn-ok"); btn_cancel = find("btn-cancel"); - pp::panopainter::bind_legacy_click_destroys_node(*btn_cancel, *this); btn_delete = find("btn-delete"); btn_delete->on_click = [this](Node*) { if (!current) @@ -39,7 +38,13 @@ void NodeDialogBrowse::init_controls() auto msgbox = pp::panopainter::make_legacy_overlay_node(*App::I); msgbox->m_title->set_text("Delete Project"); msgbox->m_message->set_text(("Are you sure you want to delete " + current->m_file_name + "?").c_str()); - msgbox->btn_ok->on_click = [this,msgbox](Node*){ + const auto overlay = pp::panopainter::open_legacy_overlay_node_with_handle(*this, msgbox); + if (!overlay) + { + return; + } + const auto overlay_handle = overlay.value(); + const auto on_confirm = [this, overlay_handle](Node*){ auto path = current->m_path; int idx = container->get_child_index(current); container->remove_child(current); @@ -59,9 +64,17 @@ void NodeDialogBrowse::init_controls() selected_name = ""; } Asset::delete_file(path); - pp::panopainter::close_legacy_dialog_node(*msgbox); + const auto close_status = + pp::panopainter::close_legacy_overlay_node(*this, overlay_handle); + (void)close_status; + }; + msgbox->btn_ok->on_click = on_confirm; + msgbox->on_submit = on_confirm; + msgbox->btn_cancel->on_click = [this, overlay_handle](Node*) { + const auto close_status = + pp::panopainter::close_legacy_overlay_node(*this, overlay_handle); + (void)close_status; }; - (void)pp::panopainter::attach_legacy_overlay_node_to_root(*this, msgbox); root()->update(); }; container = find("files-list"); diff --git a/src/node_dialog_open.cpp b/src/node_dialog_open.cpp index 6a378d1a..c0e66674 100644 --- a/src/node_dialog_open.cpp +++ b/src/node_dialog_open.cpp @@ -32,7 +32,6 @@ void NodeDialogOpen::init_controls() { btn_ok = find("btn-ok"); btn_cancel = find("btn-cancel"); - pp::panopainter::bind_legacy_click_destroys_node(*btn_cancel, *this); btn_delete = find("btn-delete"); btn_delete->on_click = [this](Node*) { if (!current) @@ -41,7 +40,13 @@ void NodeDialogOpen::init_controls() auto msgbox = pp::panopainter::make_legacy_overlay_node(*App::I); msgbox->m_title->set_text("Delete Project"); msgbox->m_message->set_text(("Are you sure you want to delete " + current->m_file_name + "?").c_str()); - msgbox->btn_ok->on_click = [this,msgbox](Node*){ + const auto overlay = pp::panopainter::open_legacy_overlay_node_with_handle(*this, msgbox); + if (!overlay) + { + return; + } + const auto overlay_handle = overlay.value(); + const auto on_confirm = [this, overlay_handle](Node*){ auto path = current->m_path; int idx = container->get_child_index(current); container->remove_child(current); @@ -60,9 +65,17 @@ void NodeDialogOpen::init_controls() image_tex->tex.reset(); } Asset::delete_file(path); - pp::panopainter::close_legacy_dialog_node(*msgbox); + const auto close_status = + pp::panopainter::close_legacy_overlay_node(*this, overlay_handle); + (void)close_status; + }; + msgbox->btn_ok->on_click = on_confirm; + msgbox->on_submit = on_confirm; + msgbox->btn_cancel->on_click = [this, overlay_handle](Node*) { + const auto close_status = + pp::panopainter::close_legacy_overlay_node(*this, overlay_handle); + (void)close_status; }; - (void)pp::panopainter::attach_legacy_overlay_node_to_root(*this, msgbox); root()->update(); }; container = find("files-list"); @@ -177,7 +190,6 @@ void NodeDialogSave::init_controls() { btn_ok = find("btn-ok"); btn_cancel = find("btn-cancel"); - pp::panopainter::bind_legacy_click_destroys_node(*btn_cancel, *this); input = find("txt-input"); input->on_return = [&](NodeTextInput* target){ if (btn_ok->on_click) @@ -236,7 +248,6 @@ void NodeDialogNewDoc::init_controls() btn_ok = find("btn-ok"); m_resolution = find("resolution"); btn_cancel = find("btn-cancel"); - pp::panopainter::bind_legacy_click_destroys_node(*btn_cancel, *this); input = find("txt-input"); input->on_return = [&](NodeTextInput* target){ if (btn_ok->on_click) diff --git a/src/node_panel_brush.cpp b/src/node_panel_brush.cpp index e4eb6a20..3017f48e 100644 --- a/src/node_panel_brush.cpp +++ b/src/node_panel_brush.cpp @@ -174,7 +174,15 @@ kEventResult NodePanelBrush::handle_event(Event* e) case kEventType::MouseUpL: if (!m_mouse_inside) { - pp::panopainter::close_legacy_popup_panel(*this, on_popup_close); + pp::panopainter::release_legacy_mouse_capture(*this); + if (m_parent) + { + pp::panopainter::detach_legacy_node_from_parent(*this); + } + if (on_popup_close) + { + on_popup_close(this); + } } break; default: @@ -580,11 +588,15 @@ void NodePanelBrushPreset::init() if (!popup) return; popup->SetPosition(b->m_pos.x + b->m_size.x, b->m_pos.y); - (void)pp::panopainter::attach_legacy_overlay_node_to_root(*this, popup); + const auto popup_overlay = pp::panopainter::open_legacy_overlay_node_with_handle(*this, popup); + if (!popup_overlay) { + return; + } + const auto popup_handle = popup_overlay.value(); root()->update(); auto bounds = root()->GetSize() - zw(popup->get_children_rect()); popup->SetPosition(glm::clamp(popup->m_pos, { 0, 0 }, bounds)); - popup->on_select = [this, popup] (Node* target, int index) { + popup->on_select = [this, popup_handle](Node* target, int index) { switch (index) { case 0: // import file @@ -620,7 +632,7 @@ void NodePanelBrushPreset::init() break; } } - pp::panopainter::close_legacy_popup_overlay(*popup); + (void)pp::panopainter::close_legacy_overlay_node(*this, popup_handle); }; }; m_btn_import = find("import"); @@ -665,7 +677,15 @@ kEventResult NodePanelBrushPreset::handle_event(Event* e) case kEventType::MouseUpL: if (!m_mouse_inside) { - pp::panopainter::close_legacy_popup_panel(*this, on_popup_close); + pp::panopainter::release_legacy_mouse_capture(*this); + if (m_parent) + { + pp::panopainter::detach_legacy_node_from_parent(*this); + } + if (on_popup_close) + { + on_popup_close(this); + } } break; default: diff --git a/src/node_panel_color.cpp b/src/node_panel_color.cpp index c4aadfd2..87a33fda 100644 --- a/src/node_panel_color.cpp +++ b/src/node_panel_color.cpp @@ -67,7 +67,15 @@ kEventResult NodePanelColor::handle_event(Event* e) case kEventType::MouseUpL: if (!m_mouse_inside) { - pp::panopainter::close_legacy_popup_panel(*this, on_popup_close); + pp::panopainter::release_legacy_mouse_capture(*this); + if (m_parent) + { + pp::panopainter::detach_legacy_node_from_parent(*this); + } + if (on_popup_close) + { + on_popup_close(this); + } } break; default: diff --git a/src/node_panel_grid.cpp b/src/node_panel_grid.cpp index 91a8fdd6..872fddfb 100644 --- a/src/node_panel_grid.cpp +++ b/src/node_panel_grid.cpp @@ -365,7 +365,15 @@ kEventResult NodePanelGrid::handle_event(Event* e) case kEventType::MouseUpL: if (!m_mouse_inside) { - pp::panopainter::close_legacy_popup_panel(*this, on_popup_close); + pp::panopainter::release_legacy_mouse_capture(*this); + if (m_parent) + { + pp::panopainter::detach_legacy_node_from_parent(*this); + } + if (on_popup_close) + { + on_popup_close(this); + } } break; default: diff --git a/src/node_panel_layer.cpp b/src/node_panel_layer.cpp index 9be89652..2491ce90 100644 --- a/src/node_panel_layer.cpp +++ b/src/node_panel_layer.cpp @@ -7,6 +7,40 @@ #include "node_combobox.h" #include "app.h" +namespace { + +void move_layer_entry(std::vector>& layers, int old_index, int new_index) +{ + if (old_index < 0 || new_index < 0 || old_index == new_index) { + return; + } + + const auto size = static_cast(layers.size()); + if (old_index >= size || new_index >= size) { + return; + } + + auto moved = layers[old_index]; + layers.erase(layers.begin() + old_index); + layers.insert(layers.begin() + new_index, std::move(moved)); +} + +int find_layer_index(const std::vector>& layers, const NodeLayer* layer) +{ + if (!layer) { + return -1; + } + const auto it = std::find_if( + layers.begin(), + layers.end(), + [layer](const auto& item) { + return item.get() == layer; + }); + return it == layers.end() ? -1 : static_cast(std::distance(layers.begin(), it)); +} + +} // namespace + Node* NodeLayer::clone_instantiate() const { return new NodeLayer(); @@ -121,6 +155,14 @@ void NodePanelLayer::init() btn_down = find("btn-down"); btn_duplicate = find("btn-duplicate"); btn_duplicate->on_click = [this](Node*) { + if (!m_current_layer) { + return; + } + const auto source_index = find_layer_index(m_layers, m_current_layer.get()); + if (source_index < 0) { + m_current_layer.reset(); + return; + } std::string next = m_current_layer->m_label_text + "01"; std::regex r(R"(([^\d]*)(\d+)$)"); std::smatch m; @@ -132,35 +174,65 @@ void NodePanelLayer::init() sprintf(tmp, "%s%0*d", m[1].str().c_str(), (int)num.length(), count); next = tmp; } - int source_index = m_layers_container->get_child_index(m_current_layer); auto l = add_layer(next.c_str(), false, false, nullptr, nullptr, source_index + 1); if (on_layer_duplicate) on_layer_duplicate(this, source_index); if (on_layer_change) - on_layer_change(this, -1, m_layers_container->get_child_index(m_current_layer)); + { + const auto selected_index = find_layer_index(m_layers, m_current_layer.get()); + if (selected_index >= 0) { + on_layer_change(this, -1, selected_index); + } + } update_attributes(); auto a = new ActionLayerAdd; a->m_panel = this; a->m_layer_node = l->shared_from_this(); - a->m_layer_order = m_layers_container->get_child_index(l); - a->m_layer_id = Canvas::I->m_layers[a->m_layer_order]->id; + const auto new_layer_order = find_layer_index(m_layers, l); + if (new_layer_order < 0) { + delete a; + return; + } + a->m_layer_order = new_layer_order; + a->m_layer_id = Canvas::I->m_layers[new_layer_order]->id; ActionManager::add(a); }; btn_add->on_click = [this](Node*) { - add_layer(true, true, m_layers_container->get_child_index(m_current_layer) + 1); + if (!m_current_layer) { + return; + } + const auto insert_index = find_layer_index(m_layers, m_current_layer.get()); + if (insert_index < 0) { + return; + } + add_layer(true, true, insert_index + 1); }; btn_remove->on_click = [this](Node*) { if (m_layers.size() == 1) return; // don't delete the last layer - remove_layer(m_current_layer); + if (m_current_layer) { + remove_layer(m_current_layer.get()); + } }; btn_up->on_click = [this](Node*) { - int old_idx = m_layers_container->get_child_index(m_current_layer); - m_layers_container->move_child_offset(m_current_layer, +1); - int new_idx = m_layers_container->get_child_index(m_current_layer); + if (!m_current_layer) { + return; + } + const int old_idx = find_layer_index(m_layers, m_current_layer.get()); + if (old_idx < 0) { + m_current_layer.reset(); + return; + } + m_layers_container->move_child_offset(m_current_layer.get(), +1); + const int new_idx = m_layers_container->get_child_index(m_current_layer.get()); + if (new_idx < 0) { + m_current_layer->m_selected = true; + return; + } if (on_layer_order && old_idx != new_idx) { + move_layer_entry(m_layers, old_idx, new_idx); on_layer_order(this, old_idx, new_idx); } auto a = new ActionLayerMove; @@ -170,11 +242,23 @@ void NodePanelLayer::init() ActionManager::add(a); }; btn_down->on_click = [this](Node*) { - int old_idx = m_layers_container->get_child_index(m_current_layer); - m_layers_container->move_child_offset(m_current_layer, -1); - int new_idx = m_layers_container->get_child_index(m_current_layer); + if (!m_current_layer) { + return; + } + const int old_idx = find_layer_index(m_layers, m_current_layer.get()); + if (old_idx < 0) { + m_current_layer.reset(); + return; + } + m_layers_container->move_child_offset(m_current_layer.get(), -1); + const int new_idx = m_layers_container->get_child_index(m_current_layer.get()); + if (new_idx < 0) { + m_current_layer->m_selected = true; + return; + } if (on_layer_order && old_idx != new_idx) { + move_layer_entry(m_layers, old_idx, new_idx); on_layer_order(this, old_idx, new_idx); } auto a = new ActionLayerMove; @@ -185,15 +269,21 @@ void NodePanelLayer::init() }; m_opacity = find("opacity"); m_opacity->on_value_changed = [this](Node*, float value) { - handle_layer_opacity(m_current_layer, value); + if (m_current_layer && find_layer_index(m_layers, m_current_layer.get()) >= 0) { + handle_layer_opacity(m_current_layer.get(), value); + } }; m_alpha_lock = find("alpha-lock"); m_alpha_lock->on_value_changed = [this](Node*, bool locked) { - handle_layer_alpha_lock(m_current_layer, locked); + if (m_current_layer && find_layer_index(m_layers, m_current_layer.get()) >= 0) { + handle_layer_alpha_lock(m_current_layer.get(), locked); + } }; m_blend_mode = find("blend-mode"); m_blend_mode->on_select = [this](Node*, int index) { - handle_layer_blend_mode(m_current_layer, index); + if (m_current_layer && find_layer_index(m_layers, m_current_layer.get()) >= 0) { + handle_layer_blend_mode(m_current_layer.get(), index); + } }; } @@ -215,30 +305,45 @@ NodeLayer* NodePanelLayer::add_layer(const char* name, bool add_history /*= true // reset selected state for (const auto& c : m_layers_container->m_children) - ((NodeLayer*)c.get())->m_selected = false; + static_cast(c.get())->m_selected = false; if (m_current_layer) m_current_layer->m_selected = false; - m_current_layer = l.get(); + m_current_layer = l; m_current_layer->m_selected = true; - m_layers.push_back(l.get()); + const auto insert_index = std::clamp(index, 0, static_cast(m_layers.size())); + m_layers.insert(m_layers.begin() + insert_index, l); if (add_history) { if (create_events) { if (on_layer_add) - on_layer_add(this, nullptr, m_layers_container->get_child_index(m_current_layer)); + { + const auto current_index = find_layer_index(m_layers, m_current_layer.get()); + if (current_index >= 0) { + on_layer_add(this, nullptr, current_index); + } + } if (on_layer_change) - on_layer_change(this, -1, m_layers_container->get_child_index(m_current_layer)); + { + if (const auto current_index = find_layer_index(m_layers, m_current_layer.get()); current_index >= 0) { + on_layer_change(this, -1, current_index); + } + } update_attributes(); } auto a = new ActionLayerAdd; a->m_panel = this; a->m_layer_node = l->shared_from_this(); - a->m_layer_order = m_layers_container->get_child_index(l.get()); - a->m_layer_id = Canvas::I->m_layers[a->m_layer_order]->id; + const auto layer_order = find_layer_index(m_layers, l.get()); + if (layer_order < 0) { + delete a; + return l.get(); + } + a->m_layer_order = layer_order; + a->m_layer_id = Canvas::I->m_layers[layer_order]->id; ActionManager::add(a); } else if (create_events) @@ -246,7 +351,12 @@ NodeLayer* NodePanelLayer::add_layer(const char* name, bool add_history /*= true if (on_layer_add) on_layer_add(this, layer, index); if (on_layer_change) - on_layer_change(this, -1, m_layers_container->get_child_index(m_current_layer)); + { + const auto current_index = find_layer_index(m_layers, m_current_layer.get()); + if (current_index >= 0) { + on_layer_change(this, -1, current_index); + } + } update_attributes(); } @@ -267,21 +377,38 @@ NodeLayer* NodePanelLayer::get_layer_at(int index) void NodePanelLayer::remove_layer(NodeLayer* layer, bool add_history /*= true*/) { - auto it = std::find(m_layers.begin(), m_layers.end(), layer); - auto i = m_layers_container->get_child_index(layer); - int old_idx = i;// (int)std::distance(m_layers.begin(), it); + if (!layer) { + return; + } + const auto it = std::find_if( + m_layers.begin(), + m_layers.end(), + [layer](const std::shared_ptr& l) { + return l.get() == layer; + }); + if (it == m_layers.end()) { + return; + } + const int old_idx = static_cast(std::distance(m_layers.begin(), it)); (*it)->m_selected = false; auto copy = (*it)->shared_from_this(); m_layers_container->remove_child(layer); m_layers.erase(it); - i = std::min(i, (int)m_layers.size() - 1); + const int i = m_layers.empty() ? -1 : std::min(old_idx, static_cast(m_layers.size()) - 1); // reset selected state for (const auto& c : m_layers_container->m_children) - ((NodeLayer*)c.get())->m_selected = false; + static_cast(c.get())->m_selected = false; - m_current_layer = (NodeLayer*)m_layers_container->get_child_at(i); - m_current_layer->m_selected = true; + const auto next_layer = i < 0 ? nullptr : m_layers_container->get_child_at(i); + if (next_layer) { + m_current_layer = std::static_pointer_cast(next_layer->shared_from_this()); + } else { + m_current_layer.reset(); + } + if (m_current_layer) { + m_current_layer->m_selected = true; + } if (add_history) { @@ -302,45 +429,70 @@ void NodePanelLayer::remove_layer(NodeLayer* layer, bool add_history /*= true*/) void NodePanelLayer::handle_layer_opacity(NodeLayer* target, float value) { - if (on_layer_opacity_changed) - on_layer_opacity_changed(this, m_layers_container->get_child_index(target), value); + const auto idx = find_layer_index(m_layers, target); + if (idx < 0 || !on_layer_opacity_changed) { + return; + } + on_layer_opacity_changed(this, idx, value); } void NodePanelLayer::handle_layer_highlight(NodeLayer* target, bool highlight) { - if (on_layer_highlight_changed) - on_layer_highlight_changed(this, m_layers_container->get_child_index(target), highlight); + const auto idx = find_layer_index(m_layers, target); + if (idx < 0 || !on_layer_highlight_changed) { + return; + } + on_layer_highlight_changed(this, idx, highlight); } void NodePanelLayer::handle_layer_visibility(NodeLayer* target, bool visible) { + const auto idx = find_layer_index(m_layers, target); + if (idx < 0) { + return; + } save_history(); - if (on_layer_visibility_changed) - on_layer_visibility_changed(this, m_layers_container->get_child_index(target), visible); + if (on_layer_visibility_changed) { + on_layer_visibility_changed(this, idx, visible); + } } void NodePanelLayer::handle_layer_alpha_lock(NodeLayer* target, bool locked) { + const auto idx = find_layer_index(m_layers, target); + if (idx < 0) { + return; + } save_history(); - if (on_layer_alpha_lock_changed) - on_layer_alpha_lock_changed(this, m_layers_container->get_child_index(target), locked); + if (on_layer_alpha_lock_changed) { + on_layer_alpha_lock_changed(this, idx, locked); + } } void NodePanelLayer::handle_layer_blend_mode(NodeLayer* target, int mode) { + const auto idx = find_layer_index(m_layers, target); + if (idx < 0) { + return; + } save_history(); - if (on_layer_blend_mode_changed) - on_layer_blend_mode_changed(this, m_layers_container->get_child_index(target), mode); + if (on_layer_blend_mode_changed) { + on_layer_blend_mode_changed(this, idx, mode); + } } void NodePanelLayer::handle_layer_selected(NodeLayer* target) { + const auto idx = find_layer_index(m_layers, target); + if (idx < 0) { + return; + } if (m_current_layer) m_current_layer->m_selected = false; - m_current_layer = target; + m_current_layer = std::static_pointer_cast(target->shared_from_this()); m_current_layer->m_selected = true; if (on_layer_change) - on_layer_change(this, -1, m_layers_container->get_child_index(m_current_layer)); + on_layer_change(this, -1, idx); update_attributes(); } @@ -411,7 +563,15 @@ kEventResult NodePanelLayer::handle_event(Event* e) case kEventType::MouseUpL: if (!m_mouse_inside) { - pp::panopainter::close_legacy_popup_panel(*this, on_popup_close); + pp::panopainter::release_legacy_mouse_capture(*this); + if (m_parent) + { + pp::panopainter::detach_legacy_node_from_parent(*this); + } + if (on_popup_close) + { + on_popup_close(this); + } } break; default: @@ -500,11 +660,25 @@ Action* ActionLayerMove::get_redo() void ActionLayerMove::undo() { - int old_idx = m_panel->m_layers_container->get_child_index(m_layer_node.get()); - m_panel->m_layers_container->move_child_offset(m_layer_node.get(), -m_offset); - int new_idx = m_panel->m_layers_container->get_child_index(m_layer_node.get()); + if (!m_panel || !m_layer_node) { + return; + } + auto* layer = dynamic_cast(m_layer_node.get()); + if (!layer) { + return; + } + const int old_idx = find_layer_index(m_panel->m_layers, layer); + if (old_idx < 0) { + return; + } + m_panel->m_layers_container->move_child_offset(layer, -m_offset); + const int new_idx = m_panel->m_layers_container->get_child_index(layer); + if (new_idx < 0) { + return; + } if (m_panel->on_layer_order && old_idx != new_idx) { + move_layer_entry(m_panel->m_layers, old_idx, new_idx); m_panel->on_layer_order(m_panel, old_idx, new_idx); } } diff --git a/src/node_panel_layer.h b/src/node_panel_layer.h index 560fc376..ce19df1e 100644 --- a/src/node_panel_layer.h +++ b/src/node_panel_layer.h @@ -53,8 +53,8 @@ public: std::function on_layer_duplicate; std::function layer, int index)> on_layer_add; std::function on_layer_order; - NodeLayer* m_current_layer = nullptr; - std::vector m_layers; + std::shared_ptr m_current_layer; + std::vector> m_layers; NodeScroll* m_layers_container; NodeSliderH* m_opacity; NodeCheckBox* m_alpha_lock; diff --git a/src/node_popup_menu.cpp b/src/node_popup_menu.cpp index a6b7b9f0..7b9d9d93 100644 --- a/src/node_popup_menu.cpp +++ b/src/node_popup_menu.cpp @@ -10,6 +10,11 @@ Node* NodePopupMenu::clone_instantiate() const return new NodePopupMenu(); } +void NodePopupMenu::close_popup() noexcept +{ + pp::panopainter::close_legacy_dialog_node(*this); +} + void NodePopupMenu::init() { SetPosition(0, 0); @@ -40,7 +45,7 @@ kEventResult NodePopupMenu::handle_event(Event* e) } } } - pp::panopainter::close_legacy_popup_overlay(*this); + close_popup(); break; default: return kEventResult::Available; diff --git a/src/node_popup_menu.h b/src/node_popup_menu.h index fe73f100..2e2379e8 100644 --- a/src/node_popup_menu.h +++ b/src/node_popup_menu.h @@ -5,6 +5,7 @@ class NodePopupMenu : public Node { public: std::function on_select; + void close_popup() noexcept; virtual Node* clone_instantiate() const override; virtual void init() override; virtual kEventResult handle_event(Event* e) override; diff --git a/src/ui_core/node_lifetime.h b/src/ui_core/node_lifetime.h index b9505ff2..ee46c9fc 100644 --- a/src/ui_core/node_lifetime.h +++ b/src/ui_core/node_lifetime.h @@ -82,6 +82,12 @@ private: class NodeLifetimeTree { public: + // Thread-affinity and mutation contract: + // - Node and callback handles are generation-validated on each lookup. + // - Destroyed, disconnected, or stale handles are rejected with explicit status. + // - Mutations may occur during callback dispatch. Callbacks execute from a + // captured snapshot, so create/remove connections and destroy subtrees are + // safely observed by the current dispatch loop. using Callback = std::function; [[nodiscard]] pp::foundation::Result create_root(); diff --git a/tests/ui_core/node_lifetime_tests.cpp b/tests/ui_core/node_lifetime_tests.cpp index abb1dbe8..cef39ed2 100644 --- a/tests/ui_core/node_lifetime_tests.cpp +++ b/tests/ui_core/node_lifetime_tests.cpp @@ -210,6 +210,46 @@ void dispatch_uses_stable_connection_snapshot(pp::tests::Harness& h) PP_EXPECT(h, second_count == 1); } +void destroy_subtree_clears_child_connections(pp::tests::Harness& h) +{ + NodeLifetimeTree tree; + const auto root = tree.create_root(); + PP_EXPECT(h, root); + if (!root) { + return; + } + + const auto child = tree.create_child(root.value()); + PP_EXPECT(h, child); + if (!child) { + return; + } + + int parent_count = 0; + int child_count = 0; + const auto root_connection = tree.connect(root.value(), [&parent_count](pp::ui::NodeHandle) { + ++parent_count; + }); + const auto child_connection = tree.connect(child.value(), [&child_count](pp::ui::NodeHandle) { + ++child_count; + }); + PP_EXPECT(h, root_connection); + PP_EXPECT(h, child_connection); + if (!root_connection || !child_connection) { + return; + } + + PP_EXPECT(h, tree.dispatch(child.value()).ok()); + PP_EXPECT(h, parent_count == 0); + PP_EXPECT(h, child_count == 1); + + PP_EXPECT(h, tree.destroy_subtree(root.value()).ok()); + PP_EXPECT(h, !tree.disconnect(root_connection.value()).ok()); + PP_EXPECT(h, !tree.disconnect(child_connection.value()).ok()); + PP_EXPECT(h, !tree.contains(root.value())); + PP_EXPECT(h, !tree.contains(child.value())); +} + void captures_and_releases_pointer_and_keyboard_nodes(pp::tests::Harness& h) { NodeLifetimeTree tree; @@ -320,6 +360,7 @@ int main() harness.run("destroying_node_disconnects_callbacks", destroying_node_disconnects_callbacks); harness.run("dispatch_survives_destroy_during_callback", dispatch_survives_destroy_during_callback); harness.run("dispatch_uses_stable_connection_snapshot", dispatch_uses_stable_connection_snapshot); + harness.run("destroy_subtree_clears_child_connections", destroy_subtree_clears_child_connections); harness.run("captures_and_releases_pointer_and_keyboard_nodes", captures_and_releases_pointer_and_keyboard_nodes); harness.run("destroying_captured_node_releases_capture", destroying_captured_node_releases_capture); harness.run("clear_models_layout_reload", clear_models_layout_reload); diff --git a/tests/ui_core/overlay_lifetime_tests.cpp b/tests/ui_core/overlay_lifetime_tests.cpp index 602ea77b..ceda9c8e 100644 --- a/tests/ui_core/overlay_lifetime_tests.cpp +++ b/tests/ui_core/overlay_lifetime_tests.cpp @@ -210,6 +210,36 @@ void clear_for_layout_reload_invalidates_overlays(pp::tests::Harness& h) PP_EXPECT(h, !tree.captured_node(UiCaptureKind::keyboard).ok()); } +void double_close_overlay_returns_invalid_argument(pp::tests::Harness& h) +{ + NodeLifetimeTree tree; + const auto root = tree.create_root(); + PP_EXPECT(h, root); + if (!root) { + return; + } + + UiOverlayLifetime overlays(tree, root.value()); + const auto popup = overlays.open_popup(); + PP_EXPECT(h, popup); + if (!popup) { + return; + } + + const auto child = overlays.open_child_popup(popup.value()); + PP_EXPECT(h, child); + if (!child) { + return; + } + + PP_EXPECT(h, overlays.close(child.value()).ok()); + PP_EXPECT(h, !overlays.close(child.value()).ok()); + PP_EXPECT(h, overlays.close(popup.value()).ok()); + PP_EXPECT(h, !overlays.close(popup.value()).ok()); + PP_EXPECT(h, overlays.overlay_count() == 0U); + PP_EXPECT(h, !overlays.tracks(child.value())); +} + } int main() @@ -222,6 +252,7 @@ int main() harness.run("modal_dialog_captures_pointer_and_keyboard", modal_dialog_captures_pointer_and_keyboard); harness.run("modeless_dialog_does_not_steal_capture", modeless_dialog_does_not_steal_capture); harness.run("rejects_untracked_or_dead_overlay_closes", rejects_untracked_or_dead_overlay_closes); + harness.run("double_close_overlay_returns_invalid_argument", double_close_overlay_returns_invalid_argument); harness.run("clear_for_layout_reload_invalidates_overlays", clear_for_layout_reload_invalidates_overlays); return harness.finish(); }