From 651be0fdcaf634bc83e0534b8284a1668862e00a Mon Sep 17 00:00:00 2001 From: scawful Date: Wed, 15 Oct 2025 13:29:21 -0400 Subject: [PATCH] refactor(editor): enhance popup management and initialization order - Updated EditorManager to initialize PopupManager in the constructor, ensuring safe access to popups during menu operations. - Refactored MenuOrchestrator to utilize PopupID constants for popup management, improving clarity and maintainability. - Enhanced PopupManager to register popups with structured definitions, allowing for better organization and future expansion. Benefits: - Streamlines the initialization process, preventing potential crashes due to uninitialized components. - Improves the organization of popup management, leading to a more intuitive user experience and easier maintenance. --- src/app/editor/editor_manager.cc | 29 +++- src/app/editor/system/menu_orchestrator.cc | 44 +++--- src/app/editor/system/menu_orchestrator.h | 2 +- src/app/editor/system/popup_manager.cc | 161 ++++++++++++++++----- src/app/editor/system/popup_manager.h | 72 +++++++++ src/app/editor/ui/ui_coordinator.cc | 37 +++-- 6 files changed, 267 insertions(+), 78 deletions(-) diff --git a/src/app/editor/editor_manager.cc b/src/app/editor/editor_manager.cc index b3ec6b8a..6a764832 100644 --- a/src/app/editor/editor_manager.cc +++ b/src/app/editor/editor_manager.cc @@ -144,28 +144,46 @@ EditorManager::EditorManager() // - EditorCardRegistry: Card-based editor UI management // - ShortcutConfigurator: Keyboard shortcut registration // - WindowDelegate: Window layout operations + // - PopupManager: Modal popup/dialog management // // EditorManager retains: // - Session storage (sessions_) and current pointers (current_rom_, etc.) // - Main update loop (iterates sessions, calls editor updates) // - Asset loading (Initialize/Load on all editors) // - Session ID tracking (current_session_id_) + // + // INITIALIZATION ORDER (CRITICAL): + // 1. PopupManager - MUST be first, MenuOrchestrator/UICoordinator take ref to it + // 2. SessionCoordinator - Independent, can be early + // 3. MenuOrchestrator - Depends on PopupManager, SessionCoordinator + // 4. UICoordinator - Depends on PopupManager, SessionCoordinator + // 5. ShortcutConfigurator - Created in Initialize(), depends on all above + // + // If this order is violated, you will get SIGSEGV crashes when menu callbacks + // try to call popup_manager_.Show() with an uninitialized PopupManager! // ============================================================================ - // SessionCoordinator: Manages session lifecycle and session-specific UI + // STEP 1: Initialize PopupManager FIRST + popup_manager_ = std::make_unique(this); + popup_manager_->Initialize(); // Registers all popups with PopupID constants + + // STEP 2: Initialize SessionCoordinator (independent of popups) session_coordinator_ = std::make_unique( static_cast(&sessions_), &card_registry_, &toast_manager_); - // MenuOrchestrator: Builds all menus and routes actions to appropriate managers + // STEP 3: Initialize MenuOrchestrator (depends on popup_manager_, session_coordinator_) menu_orchestrator_ = std::make_unique( this, menu_builder_, rom_file_manager_, project_manager_, editor_registry_, *session_coordinator_, toast_manager_, *popup_manager_); - // UICoordinator: Coordinates all UI drawing (dialogs, popups, welcome screen) + // STEP 4: Initialize UICoordinator (depends on popup_manager_, session_coordinator_) ui_coordinator_ = std::make_unique( this, rom_file_manager_, project_manager_, editor_registry_, *session_coordinator_, window_delegate_, toast_manager_, *popup_manager_, shortcut_manager_); + + // STEP 5: ShortcutConfigurator created later in Initialize() method + // It depends on all above coordinators being available } EditorManager::~EditorManager() = default; @@ -227,9 +245,8 @@ void EditorManager::Initialize(gfx::IRenderer* renderer, PRINT_IF_ERROR(OpenRomOrProject(filename)); } - // Initialize popup manager - popup_manager_ = std::make_unique(this); - popup_manager_->Initialize(); + // Note: PopupManager is now initialized in constructor before MenuOrchestrator + // This ensures all menu callbacks can safely call popup_manager_.Show() // Register emulator cards early (emulator Initialize might not be called) // Using EditorCardRegistry directly diff --git a/src/app/editor/system/menu_orchestrator.cc b/src/app/editor/system/menu_orchestrator.cc index 6b896635..12257b41 100644 --- a/src/app/editor/system/menu_orchestrator.cc +++ b/src/app/editor/system/menu_orchestrator.cc @@ -199,18 +199,18 @@ void MenuOrchestrator::AddViewMenuItems() { [this]() { OnShowEmulator(); }, "Ctrl+Shift+E") .Separator(); + // Settings and UI + menu_builder_ + .Item("Display Settings", ICON_MD_DISPLAY_SETTINGS, + [this]() { OnShowDisplaySettings(); }) + .Separator(); + // Additional UI Elements menu_builder_ .Item("Card Browser", ICON_MD_DASHBOARD, [this]() { OnShowCardBrowser(); }, "Ctrl+Shift+B") .Item("Welcome Screen", ICON_MD_HOME, - [this]() { OnShowWelcomeScreen(); }) - .Separator(); - - // Display Settings - menu_builder_ - .Item("Display Settings", ICON_MD_DISPLAY_SETTINGS, - [this]() { OnShowDisplaySettings(); }); + [this]() { OnShowWelcomeScreen(); }); } void MenuOrchestrator::BuildToolsMenu() { @@ -401,8 +401,7 @@ void MenuOrchestrator::OnSaveRom() { } void MenuOrchestrator::OnSaveRomAs() { - // TODO: Show save dialog and delegate to rom_manager_ - toast_manager_.Show("Save ROM As", ToastType::kInfo); + popup_manager_.Show(PopupID::kSaveAs); } void MenuOrchestrator::OnCreateProject() { @@ -552,12 +551,7 @@ void MenuOrchestrator::OnShowEditorSelection() { } void MenuOrchestrator::OnShowDisplaySettings() { - // Delegate to UICoordinator for display settings popup - if (editor_manager_) { - if (auto* ui = editor_manager_->ui_coordinator()) { - ui->ShowDisplaySettings(); - } - } + popup_manager_.Show(PopupID::kDisplaySettings); } void MenuOrchestrator::OnShowHexEditor() { @@ -785,44 +779,44 @@ void MenuOrchestrator::OnShowNetworkStatus() { // Help menu actions void MenuOrchestrator::OnShowAbout() { - popup_manager_.Show("About"); + popup_manager_.Show(PopupID::kAbout); } void MenuOrchestrator::OnShowGettingStarted() { - popup_manager_.Show("Getting Started"); + popup_manager_.Show(PopupID::kGettingStarted); } void MenuOrchestrator::OnShowAsarIntegration() { - popup_manager_.Show("Asar Integration"); + popup_manager_.Show(PopupID::kAsarIntegration); } void MenuOrchestrator::OnShowBuildInstructions() { - popup_manager_.Show("Build Instructions"); + popup_manager_.Show(PopupID::kBuildInstructions); } void MenuOrchestrator::OnShowCLIUsage() { - popup_manager_.Show("CLI Usage"); + popup_manager_.Show(PopupID::kCLIUsage); } void MenuOrchestrator::OnShowTroubleshooting() { - popup_manager_.Show("Troubleshooting"); + popup_manager_.Show(PopupID::kTroubleshooting); } void MenuOrchestrator::OnShowContributing() { - popup_manager_.Show("Contributing"); + popup_manager_.Show(PopupID::kContributing); } void MenuOrchestrator::OnShowWhatsNew() { - popup_manager_.Show("Whats New v03"); + popup_manager_.Show(PopupID::kWhatsNew); } void MenuOrchestrator::OnShowSupportedFeatures() { - popup_manager_.Show("Supported Features"); + popup_manager_.Show(PopupID::kSupportedFeatures); } // Additional File menu actions void MenuOrchestrator::OnShowRomInfo() { - popup_manager_.Show("ROM Information"); + popup_manager_.Show(PopupID::kRomInfo); } void MenuOrchestrator::OnCreateBackup() { diff --git a/src/app/editor/system/menu_orchestrator.h b/src/app/editor/system/menu_orchestrator.h index 83def148..dbe25312 100644 --- a/src/app/editor/system/menu_orchestrator.h +++ b/src/app/editor/system/menu_orchestrator.h @@ -86,7 +86,7 @@ class MenuOrchestrator { // Editor-specific menu actions void OnSwitchToEditor(EditorType editor_type); void OnShowEditorSelection(); - void OnShowDisplaySettings(); + void OnShowDisplaySettings(); // Display settings popup void OnShowHexEditor(); void OnShowEmulator(); void OnShowCardBrowser(); diff --git a/src/app/editor/system/popup_manager.cc b/src/app/editor/system/popup_manager.cc index 51c11ba0..0f5cc458 100644 --- a/src/app/editor/system/popup_manager.cc +++ b/src/app/editor/system/popup_manager.cc @@ -16,31 +16,104 @@ PopupManager::PopupManager(EditorManager* editor_manager) : editor_manager_(editor_manager), status_(absl::OkStatus()) {} void PopupManager::Initialize() { - // Register all popups - popups_["About"] = {"About", false, [this]() { DrawAboutPopup(); }}; - popups_["ROM Information"] = {"ROM Information", false, [this]() { DrawRomInfoPopup(); }}; - popups_["Save As.."] = {"Save As..", false, [this]() { DrawSaveAsPopup(); }}; - popups_["New Project"] = {"New Project", false, [this]() { DrawNewProjectPopup(); }}; - popups_["Supported Features"] = {"Supported Features", false, [this]() { DrawSupportedFeaturesPopup(); }}; - popups_["Open a ROM"] = {"Open a ROM", false, [this]() { DrawOpenRomHelpPopup(); }}; - popups_["Manage Project"] = {"Manage Project", false, [this]() { DrawManageProjectPopup(); }}; + // ============================================================================ + // POPUP REGISTRATION + // ============================================================================ + // All popups must be registered here BEFORE any menu callbacks can trigger them. + // This method is called in EditorManager constructor BEFORE MenuOrchestrator + // and UICoordinator are created, ensuring safe initialization order. + // + // Popup Registration Format: + // popups_[PopupID::kConstant] = { + // .name = PopupID::kConstant, + // .type = PopupType::kXxx, + // .is_visible = false, + // .allow_resize = false/true, + // .draw_function = [this]() { DrawXxxPopup(); } + // }; + // ============================================================================ - // v0.3 Help Documentation popups - popups_["Getting Started"] = {"Getting Started", false, [this]() { DrawGettingStartedPopup(); }}; - popups_["Asar Integration"] = {"Asar Integration", false, [this]() { DrawAsarIntegrationPopup(); }}; - popups_["Build Instructions"] = {"Build Instructions", false, [this]() { DrawBuildInstructionsPopup(); }}; - popups_["CLI Usage"] = {"CLI Usage", false, [this]() { DrawCLIUsagePopup(); }}; - popups_["Troubleshooting"] = {"Troubleshooting", false, [this]() { DrawTroubleshootingPopup(); }}; - popups_["Contributing"] = {"Contributing", false, [this]() { DrawContributingPopup(); }}; - popups_["Whats New v03"] = {"What's New in v0.3", false, [this]() { DrawWhatsNewPopup(); }}; + // File Operations + popups_[PopupID::kSaveAs] = { + PopupID::kSaveAs, PopupType::kFileOperation, false, false, + [this]() { DrawSaveAsPopup(); } + }; + popups_[PopupID::kNewProject] = { + PopupID::kNewProject, PopupType::kFileOperation, false, false, + [this]() { DrawNewProjectPopup(); } + }; + popups_[PopupID::kManageProject] = { + PopupID::kManageProject, PopupType::kFileOperation, false, false, + [this]() { DrawManageProjectPopup(); } + }; - // Workspace-related popups - popups_["Workspace Help"] = {"Workspace Help", false, [this]() { DrawWorkspaceHelpPopup(); }}; - popups_["Session Limit Warning"] = {"Session Limit Warning", false, [this]() { DrawSessionLimitWarningPopup(); }}; - popups_["Layout Reset Confirm"] = {"Reset Layout Confirmation", false, [this]() { DrawLayoutResetConfirmPopup(); }}; + // Information + popups_[PopupID::kAbout] = { + PopupID::kAbout, PopupType::kInfo, false, false, + [this]() { DrawAboutPopup(); } + }; + popups_[PopupID::kRomInfo] = { + PopupID::kRomInfo, PopupType::kInfo, false, false, + [this]() { DrawRomInfoPopup(); } + }; + popups_[PopupID::kSupportedFeatures] = { + PopupID::kSupportedFeatures, PopupType::kInfo, false, false, + [this]() { DrawSupportedFeaturesPopup(); } + }; + popups_[PopupID::kOpenRomHelp] = { + PopupID::kOpenRomHelp, PopupType::kHelp, false, false, + [this]() { DrawOpenRomHelpPopup(); } + }; - // Settings popups (accessible without ROM) - popups_["Display Settings"] = {"Display Settings", false, [this]() { DrawDisplaySettingsPopup(); }}; + // Help Documentation + popups_[PopupID::kGettingStarted] = { + PopupID::kGettingStarted, PopupType::kHelp, false, false, + [this]() { DrawGettingStartedPopup(); } + }; + popups_[PopupID::kAsarIntegration] = { + PopupID::kAsarIntegration, PopupType::kHelp, false, false, + [this]() { DrawAsarIntegrationPopup(); } + }; + popups_[PopupID::kBuildInstructions] = { + PopupID::kBuildInstructions, PopupType::kHelp, false, false, + [this]() { DrawBuildInstructionsPopup(); } + }; + popups_[PopupID::kCLIUsage] = { + PopupID::kCLIUsage, PopupType::kHelp, false, false, + [this]() { DrawCLIUsagePopup(); } + }; + popups_[PopupID::kTroubleshooting] = { + PopupID::kTroubleshooting, PopupType::kHelp, false, false, + [this]() { DrawTroubleshootingPopup(); } + }; + popups_[PopupID::kContributing] = { + PopupID::kContributing, PopupType::kHelp, false, false, + [this]() { DrawContributingPopup(); } + }; + popups_[PopupID::kWhatsNew] = { + PopupID::kWhatsNew, PopupType::kHelp, false, false, + [this]() { DrawWhatsNewPopup(); } + }; + + // Settings + popups_[PopupID::kDisplaySettings] = { + PopupID::kDisplaySettings, PopupType::kSettings, false, true, // Resizable + [this]() { DrawDisplaySettingsPopup(); } + }; + + // Workspace + popups_[PopupID::kWorkspaceHelp] = { + PopupID::kWorkspaceHelp, PopupType::kHelp, false, false, + [this]() { DrawWorkspaceHelpPopup(); } + }; + popups_[PopupID::kSessionLimitWarning] = { + PopupID::kSessionLimitWarning, PopupType::kWarning, false, false, + [this]() { DrawSessionLimitWarningPopup(); } + }; + popups_[PopupID::kLayoutResetConfirm] = { + PopupID::kLayoutResetConfirm, PopupType::kConfirmation, false, false, + [this]() { DrawLayoutResetConfirmPopup(); } + }; } void PopupManager::DrawPopups() { @@ -52,11 +125,9 @@ void PopupManager::DrawPopups() { if (params.is_visible) { OpenPopup(name.c_str()); - // Special handling for Display Settings popup to make it resizable - ImGuiWindowFlags popup_flags = ImGuiWindowFlags_AlwaysAutoResize; - if (name == "Display Settings") { - popup_flags = ImGuiWindowFlags_None; // Allow resizing for display settings - } + // Use allow_resize flag from popup definition + ImGuiWindowFlags popup_flags = params.allow_resize ? + ImGuiWindowFlags_None : ImGuiWindowFlags_AlwaysAutoResize; if (BeginPopupModal(name.c_str(), nullptr, popup_flags)) { params.draw_function(); @@ -67,14 +138,31 @@ void PopupManager::DrawPopups() { } void PopupManager::Show(const char* name) { - auto it = popups_.find(name); + if (!name) { + return; // Safety check for null pointer + } + + std::string name_str(name); + auto it = popups_.find(name_str); if (it != popups_.end()) { it->second.is_visible = true; + } else { + // Log warning for unregistered popup + printf("[PopupManager] Warning: Popup '%s' not registered. Available popups: ", name); + for (const auto& [key, _] : popups_) { + printf("'%s' ", key.c_str()); + } + printf("\n"); } } void PopupManager::Hide(const char* name) { - auto it = popups_.find(name); + if (!name) { + return; // Safety check for null pointer + } + + std::string name_str(name); + auto it = popups_.find(name_str); if (it != popups_.end()) { it->second.is_visible = false; CloseCurrentPopup(); @@ -82,7 +170,12 @@ void PopupManager::Hide(const char* name) { } bool PopupManager::IsVisible(const char* name) const { - auto it = popups_.find(name); + if (!name) { + return false; // Safety check for null pointer + } + + std::string name_str(name); + auto it = popups_.find(name_str); if (it != popups_.end()) { return it->second.is_visible; } @@ -189,7 +282,7 @@ void PopupManager::DrawSaveAsPopup() { auto status = editor_manager_->SaveRomAs(final_filename); if (status.ok()) { save_as_filename = ""; - Hide("Save As.."); + Hide(PopupID::kSaveAs); } } } @@ -198,7 +291,7 @@ void PopupManager::DrawSaveAsPopup() { if (Button(absl::StrFormat("%s Cancel", ICON_MD_CANCEL).c_str(), gui::kDefaultModalSize)) { save_as_filename = ""; - Hide("Save As.."); + Hide(PopupID::kSaveAs); } } @@ -265,7 +358,7 @@ void PopupManager::DrawNewProjectPopup() { rom_filename = ""; labels_filename = ""; code_folder = ""; - Hide("New Project"); + Hide(PopupID::kNewProject); } } } @@ -278,7 +371,7 @@ void PopupManager::DrawNewProjectPopup() { rom_filename = ""; labels_filename = ""; code_folder = ""; - Hide("New Project"); + Hide(PopupID::kNewProject); } } diff --git a/src/app/editor/system/popup_manager.h b/src/app/editor/system/popup_manager.h index 5ea41b03..b45b90ac 100644 --- a/src/app/editor/system/popup_manager.h +++ b/src/app/editor/system/popup_manager.h @@ -13,12 +13,84 @@ namespace editor { // Forward declaration class EditorManager; +/** + * @enum PopupType + * @brief Type classification for popups to enable future filtering and organization + */ +enum class PopupType { + kInfo, // Information display (About, ROM Info, etc.) + kHelp, // Help documentation (Getting Started, etc.) + kSettings, // Settings dialogs (Display Settings, etc.) + kFileOperation, // File operations (Save As, New Project, etc.) + kConfirmation, // Confirmation dialogs (Layout Reset, etc.) + kWarning, // Warning messages (Session Limit, etc.) + kEditor // Editor-specific dialogs +}; + +/** + * @struct PopupDefinition + * @brief Complete definition of a popup including metadata + */ +struct PopupDefinition { + const char* id; // Unique constant identifier + const char* display_name; // Human-readable name for UI + PopupType type; // Type classification + bool allow_resize; // Whether popup can be resized + std::function draw_function; // Drawing callback (set at runtime) +}; + +/** + * @struct PopupParams + * @brief Runtime state for a registered popup + */ struct PopupParams { std::string name; + PopupType type; bool is_visible = false; + bool allow_resize = false; std::function draw_function; }; +/** + * @namespace PopupID + * @brief String constants for all popup identifiers to prevent typos + */ +namespace PopupID { + // File Operations + constexpr const char* kSaveAs = "Save As.."; + constexpr const char* kNewProject = "New Project"; + constexpr const char* kManageProject = "Manage Project"; + + // Information + constexpr const char* kAbout = "About"; + constexpr const char* kRomInfo = "ROM Information"; + constexpr const char* kSupportedFeatures = "Supported Features"; + constexpr const char* kStatus = "Status"; + + // Help Documentation + constexpr const char* kGettingStarted = "Getting Started"; + constexpr const char* kAsarIntegration = "Asar Integration"; + constexpr const char* kBuildInstructions = "Build Instructions"; + constexpr const char* kCLIUsage = "CLI Usage"; + constexpr const char* kTroubleshooting = "Troubleshooting"; + constexpr const char* kContributing = "Contributing"; + constexpr const char* kWhatsNew = "Whats New v03"; + constexpr const char* kOpenRomHelp = "Open a ROM"; + + // Settings + constexpr const char* kDisplaySettings = "Display Settings"; + + // Workspace + constexpr const char* kWorkspaceHelp = "Workspace Help"; + constexpr const char* kSessionLimitWarning = "Session Limit Warning"; + constexpr const char* kLayoutResetConfirm = "Reset Layout Confirmation"; + + // Future expansion + constexpr const char* kQuickExport = "Quick Export"; + constexpr const char* kAssetImport = "Asset Import"; + constexpr const char* kScriptGenerator = "Script Generator"; +} + // ImGui popup manager. class PopupManager { public: diff --git a/src/app/editor/ui/ui_coordinator.cc b/src/app/editor/ui/ui_coordinator.cc index 4ca038ab..260efefe 100644 --- a/src/app/editor/ui/ui_coordinator.cc +++ b/src/app/editor/ui/ui_coordinator.cc @@ -104,6 +104,7 @@ void UICoordinator::DrawAllUI() { // Draw all UI components in order DrawMenuBarExtras(); DrawContextSensitiveCardControl(); + DrawCommandPalette(); // NEW: Moved from EditorManager DrawSessionSwitcher(); DrawSessionManager(); DrawSessionRenameDialog(); @@ -119,15 +120,15 @@ void UICoordinator::DrawAllUI() { void UICoordinator::DrawMenuBarExtras() { // Get current ROM from EditorManager (RomFileManager doesn't track "current") auto* current_rom = editor_manager_->GetCurrentRom(); + + // Calculate version width for right alignment std::string version_text = absl::StrFormat("v%s", editor_manager_->version().c_str()); float version_width = ImGui::CalcTextSize(version_text.c_str()).x; - float session_rom_area_width = 280.0f; - - ImGui::SameLine(ImGui::GetWindowWidth() - version_width - 10 - session_rom_area_width); // Session indicator with Material Design styling if (session_coordinator_.HasMultipleSessions()) { - std::string session_button_text = absl::StrFormat("%s%zu", ICON_MD_TAB, + ImGui::SameLine(); + std::string session_button_text = absl::StrFormat("%s %zu", ICON_MD_TAB, session_coordinator_.GetActiveSessionCount()); // Material Design button styling @@ -142,21 +143,32 @@ void UICoordinator::DrawMenuBarExtras() { ImGui::PopStyleColor(3); if (ImGui::IsItemHovered()) { - ImGui::SetTooltip("Sessions: %zu active\nClick to switch", - session_coordinator_.GetActiveSessionCount()); + ImGui::SetTooltip("Switch Sessions (Ctrl+Tab)"); } - ImGui::SameLine(); } // ROM information display with Material Design card styling + ImGui::SameLine(); if (current_rom && current_rom->is_loaded()) { ImGui::PushStyleColor(ImGuiCol_Text, gui::GetTextSecondaryVec4()); - ImGui::Text("%s %s", ICON_MD_INSERT_DRIVE_FILE, current_rom->title().c_str()); + std::string rom_title = current_rom->title(); + if (current_rom->dirty()) { + ImGui::Text("%s %s*", ICON_MD_CIRCLE, rom_title.c_str()); + if (ImGui::IsItemHovered()) { + ImGui::SetTooltip("Unsaved changes"); + } + } else { + ImGui::Text("%s %s", ICON_MD_INSERT_DRIVE_FILE, rom_title.c_str()); + } + ImGui::PopStyleColor(); + } else { + ImGui::PushStyleColor(ImGuiCol_Text, gui::GetTextDisabledVec4()); + ImGui::Text("%s No ROM", ICON_MD_WARNING); ImGui::PopStyleColor(); - ImGui::SameLine(); } - // Version info with subtle styling + // Version info aligned to far right + ImGui::SameLine(ImGui::GetWindowWidth() - version_width - 15.0f); ImGui::PushStyleColor(ImGuiCol_Text, gui::GetTextDisabledVec4()); ImGui::Text("%s", version_text.c_str()); ImGui::PopStyleColor(); @@ -319,8 +331,9 @@ void UICoordinator::HidePopup(const std::string& popup_name) { } void UICoordinator::ShowDisplaySettings() { - show_display_settings_ = true; - ShowPopup("Display Settings"); + // Display Settings is now a popup managed by PopupManager + // Delegate directly to PopupManager instead of UICoordinator + popup_manager_.Show(PopupID::kDisplaySettings); } void UICoordinator::HideCurrentEditorCards() {