refactor: Improve Editor Management and UI Consistency

- Removed PushID and PopID calls in EditorManager to prevent ID stack corruption, relying on window titles for uniqueness. - Updated ImGui window size and position settings to use FirstUseEver for maximizing on first open, enhancing user experience. - Replaced AgentUI::PopPanelStyle with ImGui::PopStyleColor in multiple locations for consistency in style management. - Ensured all EditorCard instances consistently call End after Begin, improving code clarity and preventing potential rendering issues.
2025-10-06 00:33:10 -04:00
parent 5dca8ecc79
commit dd56addd5e
9 changed files with 287 additions and 118 deletions
--- a/src/app/editor/agent/agent_chat_widget.cc
+++ b/src/app/editor/agent/agent_chat_widget.cc
@@ -555,7 +555,7 @@ void AgentChatWidget::RenderHistory() {
    }
  }
  ImGui::EndChild();
-  AgentUI::PopPanelStyle();
+  ImGui::PopStyleColor();  // Pop the color we pushed at line 531
  last_history_size_ = history.size();
 }

@@ -1342,8 +1342,8 @@ void AgentChatWidget::RenderCollaborationPanel() {
  }

  ImGui::EndChild();
-  AgentUI::PopPanelStyle();
-  ImGui::PopStyleVar(2);
+  ImGui::PopStyleColor();  // Pop the ChildBg color from line 1091
+  ImGui::PopStyleVar(2);   // Pop the 2 StyleVars from lines 1082-1083
  ImGui::PopID();  // CollabPanel
 }

@@ -1666,7 +1666,7 @@ void AgentChatWidget::RenderAgentConfigPanel() {
  }

  ImGui::EndChild();
-  AgentUI::PopPanelStyle();
+  ImGui::PopStyleColor();  // Pop the ChildBg color from line 1609
 }

 void AgentChatWidget::RenderZ3EDCommandPanel() {
@@ -1749,9 +1749,9 @@ void AgentChatWidget::RenderZ3EDCommandPanel() {
  }

  ImGui::EndChild();
-  AgentUI::PopPanelStyle();
+  ImGui::PopStyleColor();  // Pop the ChildBg color from line 1677

-  ImGui::PopID();  // FIX: Pop the Z3EDCmdPanel ID
+  ImGui::PopID();  // Pop the Z3EDCmdPanel ID
 }

 void AgentChatWidget::RenderRomSyncPanel() {
@@ -1848,7 +1848,7 @@ void AgentChatWidget::RenderRomSyncPanel() {
  }

  ImGui::EndChild();
-  AgentUI::PopPanelStyle();
+  ImGui::PopStyleColor();  // Pop the ChildBg color from line 1758
 }

 void AgentChatWidget::RenderSnapshotPreviewPanel() {
@@ -1898,7 +1898,7 @@ void AgentChatWidget::RenderSnapshotPreviewPanel() {
  }

  ImGui::EndChild();
-  AgentUI::PopPanelStyle();
+  ImGui::PopStyleColor();  // Pop the ChildBg color from line 1860
 }

 void AgentChatWidget::RenderProposalManagerPanel() {
@@ -2094,7 +2094,7 @@ void AgentChatWidget::RenderHarnessPanel() {
  }

  ImGui::EndChild();
-  AgentUI::PopPanelStyle();
+  ImGui::PopStyleColor();  // Pop the ChildBg color from line 1982
  ImGui::PopID();
 }

--- a/src/app/editor/code/assembly_editor.cc
+++ b/src/app/editor/code/assembly_editor.cc
@@ -228,8 +228,8 @@ void AssemblyEditor::UpdateCodeView() {
        current_folder_ = LoadFolder(FileDialogWrapper::ShowOpenFolderDialog());
      }
    }
-    file_browser_card.End();
  }
+  file_browser_card.End();  // ALWAYS call End after Begin

  // Draw open files as individual, dockable EditorCards
  for (int i = 0; i < active_files_.Size; i++) {
@@ -253,8 +253,8 @@ void AssemblyEditor::UpdateCodeView() {
            active_file_id_ = file_id;
        }
        open_files_[file_id].Render(absl::StrCat("##", card_name).c_str());
-        file_card.End();
    }
+    file_card.End();  // ALWAYS call End after Begin

    if (!open) {
      active_files_.erase(active_files_.Data + i);
--- a/src/app/editor/code/memory_editor.cc
+++ b/src/app/editor/code/memory_editor.cc
@@ -1,23 +1,53 @@
 #include "app/editor/code/memory_editor.h"
+
+#include "absl/strings/str_format.h"
 #include "app/gui/icons.h"
+#include "app/gui/style.h"
+#include "imgui/imgui.h"

 namespace yaze {
 namespace editor {

 void MemoryEditorWithDiffChecker::DrawToolbar() {
+  // Modern compact toolbar with icon-only buttons
+  ImGui::PushStyleVar(ImGuiStyleVar_FramePadding, ImVec2(6, 4));
+  ImGui::PushStyleVar(ImGuiStyleVar_ItemSpacing, ImVec2(4, 4));
+  
  if (ImGui::Button(ICON_MD_LOCATION_SEARCHING " Jump")) {
    ImGui::OpenPopup("JumpToAddress");
  }
+  if (ImGui::IsItemHovered()) {
+    ImGui::SetTooltip("Jump to specific address");
+  }
  
  ImGui::SameLine();
  if (ImGui::Button(ICON_MD_SEARCH " Search")) {
    ImGui::OpenPopup("SearchPattern");
  }
+  if (ImGui::IsItemHovered()) {
+    ImGui::SetTooltip("Search for hex pattern");
+  }
  
  ImGui::SameLine();
  if (ImGui::Button(ICON_MD_BOOKMARK " Bookmarks")) {
    ImGui::OpenPopup("Bookmarks");
  }
+  if (ImGui::IsItemHovered()) {
+    ImGui::SetTooltip("Manage address bookmarks");
+  }
+  
+  ImGui::SameLine();
+  ImGui::Text(ICON_MD_MORE_VERT);
+  ImGui::SameLine();
+  
+  // Show current address
+  if (current_address_ != 0) {
+    ImGui::TextColored(ImVec4(0.4f, 0.8f, 1.0f, 1.0f), 
+                      ICON_MD_LOCATION_ON " 0x%06X", current_address_);
+  }
+  
+  ImGui::PopStyleVar(2);
+  ImGui::Separator();
  
  DrawJumpToAddressPopup();
  DrawSearchPopup();
@@ -25,17 +55,37 @@ void MemoryEditorWithDiffChecker::DrawToolbar() {
 }

 void MemoryEditorWithDiffChecker::DrawJumpToAddressPopup() {
-  if (ImGui::BeginPopup("JumpToAddress")) {
-    ImGui::Text(ICON_MD_LOCATION_SEARCHING " Jump to Address");
+  if (ImGui::BeginPopupModal("JumpToAddress", nullptr, ImGuiWindowFlags_AlwaysAutoResize)) {
+    ImGui::TextColored(ImVec4(0.4f, 0.8f, 1.0f, 1.0f), 
+                      ICON_MD_LOCATION_SEARCHING " Jump to Address");
    ImGui::Separator();
-    ImGui::InputText("Address (hex)", jump_address_, IM_ARRAYSIZE(jump_address_));
+    ImGui::Spacing();
+    
+    ImGui::SetNextItemWidth(200);
+    if (ImGui::InputText("##jump_addr", jump_address_, IM_ARRAYSIZE(jump_address_), 
+                        ImGuiInputTextFlags_CharsHexadecimal | ImGuiInputTextFlags_EnterReturnsTrue)) {
+      // Parse and jump on Enter key
+      unsigned int addr;
+      if (sscanf(jump_address_, "%X", &addr) == 1) {
+        current_address_ = addr;
+        ImGui::CloseCurrentPopup();
+      }
+    }
    ImGui::TextDisabled("Format: 0x1C800 or 1C800");
-    if (ImGui::Button("Go", ImVec2(120, 0))) {
-      // TODO: Parse address and scroll to it
+    
+    ImGui::Spacing();
+    ImGui::Separator();
+    ImGui::Spacing();
+    
+    if (ImGui::Button(ICON_MD_CHECK " Go", ImVec2(120, 0))) {
+      unsigned int addr;
+      if (sscanf(jump_address_, "%X", &addr) == 1) {
+        current_address_ = addr;
+      }
      ImGui::CloseCurrentPopup();
    }
    ImGui::SameLine();
-    if (ImGui::Button("Cancel", ImVec2(120, 0))) {
+    if (ImGui::Button(ICON_MD_CANCEL " Cancel", ImVec2(120, 0))) {
      ImGui::CloseCurrentPopup();
    }
    ImGui::EndPopup();
@@ -43,17 +93,45 @@ void MemoryEditorWithDiffChecker::DrawJumpToAddressPopup() {
 }

 void MemoryEditorWithDiffChecker::DrawSearchPopup() {
-  if (ImGui::BeginPopup("SearchPattern")) {
-    ImGui::Text(ICON_MD_SEARCH " Search Hex Pattern");
+  if (ImGui::BeginPopupModal("SearchPattern", nullptr, ImGuiWindowFlags_AlwaysAutoResize)) {
+    ImGui::TextColored(ImVec4(0.4f, 0.8f, 0.4f, 1.0f), 
+                      ICON_MD_SEARCH " Search Hex Pattern");
    ImGui::Separator();
-    ImGui::InputText("Pattern", search_pattern_, IM_ARRAYSIZE(search_pattern_));
+    ImGui::Spacing();
+    
+    ImGui::SetNextItemWidth(300);
+    if (ImGui::InputText("##search_pattern", search_pattern_, IM_ARRAYSIZE(search_pattern_),
+                        ImGuiInputTextFlags_EnterReturnsTrue)) {
+      // TODO: Implement search
+      ImGui::CloseCurrentPopup();
+    }
    ImGui::TextDisabled("Use ?? for wildcard (e.g. FF 00 ?? 12)");
-    if (ImGui::Button("Search", ImVec2(120, 0))) {
+    ImGui::Spacing();
+    
+    // Quick preset patterns
+    ImGui::Text(ICON_MD_LIST " Quick Patterns:");
+    if (ImGui::SmallButton("LDA")) {
+      snprintf(search_pattern_, sizeof(search_pattern_), "A9 ??");
+    }
+    ImGui::SameLine();
+    if (ImGui::SmallButton("STA")) {
+      snprintf(search_pattern_, sizeof(search_pattern_), "8D ?? ??");
+    }
+    ImGui::SameLine();
+    if (ImGui::SmallButton("JSR")) {
+      snprintf(search_pattern_, sizeof(search_pattern_), "20 ?? ??");
+    }
+    
+    ImGui::Spacing();
+    ImGui::Separator();
+    ImGui::Spacing();
+    
+    if (ImGui::Button(ICON_MD_SEARCH " Search", ImVec2(120, 0))) {
      // TODO: Implement search using hex-search handler
      ImGui::CloseCurrentPopup();
    }
    ImGui::SameLine();
-    if (ImGui::Button("Cancel", ImVec2(120, 0))) {
+    if (ImGui::Button(ICON_MD_CANCEL " Cancel", ImVec2(120, 0))) {
      ImGui::CloseCurrentPopup();
    }
    ImGui::EndPopup();
@@ -61,27 +139,79 @@ void MemoryEditorWithDiffChecker::DrawSearchPopup() {
 }

 void MemoryEditorWithDiffChecker::DrawBookmarksPopup() {
-  if (ImGui::BeginPopup("Bookmarks")) {
-    ImGui::Text(ICON_MD_BOOKMARK " Memory Bookmarks");
+  if (ImGui::BeginPopupModal("Bookmarks", nullptr, ImGuiWindowFlags_AlwaysAutoResize)) {
+    ImGui::TextColored(ImVec4(1.0f, 0.843f, 0.0f, 1.0f), 
+                      ICON_MD_BOOKMARK " Memory Bookmarks");
    ImGui::Separator();
+    ImGui::Spacing();
    
    if (bookmarks_.empty()) {
-      ImGui::TextDisabled("No bookmarks yet");
+      ImGui::TextDisabled(ICON_MD_INFO " No bookmarks yet");
+      ImGui::Spacing();
      ImGui::Separator();
-      if (ImGui::Button("Add Current Address")) {
-        // TODO: Add bookmark at current address
+      ImGui::Spacing();
+      
+      if (ImGui::Button(ICON_MD_ADD " Add Current Address", ImVec2(250, 0))) {
+        Bookmark new_bookmark;
+        new_bookmark.address = current_address_;
+        new_bookmark.name = absl::StrFormat("Bookmark %zu", bookmarks_.size() + 1);
+        new_bookmark.description = "User-defined bookmark";
+        bookmarks_.push_back(new_bookmark);
      }
    } else {
-      for (size_t i = 0; i < bookmarks_.size(); ++i) {
-        const auto& bm = bookmarks_[i];
-        ImGui::PushID(static_cast<int>(i));
-        if (ImGui::Selectable(bm.name.c_str())) {
-          current_address_ = bm.address;
-          // TODO: Jump to this address
+      // Bookmarks table
+      ImGui::BeginChild("##bookmarks_list", ImVec2(500, 300), true);
+      if (ImGui::BeginTable("##bookmarks_table", 3, 
+                           ImGuiTableFlags_Borders | ImGuiTableFlags_RowBg | 
+                           ImGuiTableFlags_Resizable)) {
+        ImGui::TableSetupColumn("Name", ImGuiTableColumnFlags_WidthFixed, 150);
+        ImGui::TableSetupColumn("Address", ImGuiTableColumnFlags_WidthFixed, 100);
+        ImGui::TableSetupColumn("Description", ImGuiTableColumnFlags_WidthStretch);
+        ImGui::TableHeadersRow();
+        
+        for (size_t i = 0; i < bookmarks_.size(); ++i) {
+          const auto& bm = bookmarks_[i];
+          ImGui::PushID(static_cast<int>(i));
+          
+          ImGui::TableNextRow();
+          ImGui::TableNextColumn();
+          if (ImGui::Selectable(bm.name.c_str(), false, ImGuiSelectableFlags_SpanAllColumns)) {
+            current_address_ = bm.address;
+            ImGui::CloseCurrentPopup();
+          }
+          
+          ImGui::TableNextColumn();
+          ImGui::TextColored(ImVec4(0.4f, 0.8f, 1.0f, 1.0f), "0x%06X", bm.address);
+          
+          ImGui::TableNextColumn();
+          ImGui::TextDisabled("%s", bm.description.c_str());
+          
+          ImGui::PopID();
        }
-        ImGui::TextDisabled("  0x%06X - %s", bm.address, bm.description.c_str());
-        ImGui::PopID();
+        ImGui::EndTable();
      }
+      ImGui::EndChild();
+      
+      ImGui::Spacing();
+      if (ImGui::Button(ICON_MD_ADD " Add Bookmark", ImVec2(150, 0))) {
+        Bookmark new_bookmark;
+        new_bookmark.address = current_address_;
+        new_bookmark.name = absl::StrFormat("Bookmark %zu", bookmarks_.size() + 1);
+        new_bookmark.description = "User-defined bookmark";
+        bookmarks_.push_back(new_bookmark);
+      }
+      ImGui::SameLine();
+      if (ImGui::Button(ICON_MD_CLEAR_ALL " Clear All", ImVec2(150, 0))) {
+        bookmarks_.clear();
+      }
+    }
+    
+    ImGui::Spacing();
+    ImGui::Separator();
+    ImGui::Spacing();
+    
+    if (ImGui::Button(ICON_MD_CLOSE " Close", ImVec2(250, 0))) {
+      ImGui::CloseCurrentPopup();
    }
    
    ImGui::EndPopup();
--- a/src/app/editor/dungeon/dungeon_editor_v2.cc
+++ b/src/app/editor/dungeon/dungeon_editor_v2.cc
@@ -140,8 +140,8 @@ void DungeonEditorV2::DrawLayout() {
    gui::EditorCard room_card(card_name, ICON_MD_GRID_ON, &open);
    if (room_card.Begin()) {
      DrawRoomTab(room_id);
-      room_card.End();
    }
+    room_card.End();  // ALWAYS call End after Begin

    if (!open) {
      active_rooms_.erase(active_rooms_.Data + i);
--- a/src/app/editor/editor_manager.cc
+++ b/src/app/editor/editor_manager.cc
@@ -747,15 +747,12 @@ absl::Status EditorManager::Update() {
        std::string window_title =
            GenerateUniqueEditorTitle(editor->type(), session_idx);
        
-        // Create truly unique ImGui ID combining session index and editor type
-        // This ensures ImGui treats them as completely separate windows
-        ImGui::PushID(static_cast<int>(session_idx * 100 + static_cast<int>(editor->type())));
+        // Note: PushID removed - window title provides sufficient uniqueness
+        // and PushID was causing ID stack corruption issues

-        // Set window to maximize on first open
-        if (ImGui::IsWindowAppearing()) {
-          ImGui::SetNextWindowSize(ImGui::GetMainViewport()->WorkSize, ImGuiCond_Appearing);
-          ImGui::SetNextWindowPos(ImGui::GetMainViewport()->WorkPos, ImGuiCond_Appearing);
-        }
+        // Set window to maximize on first open (use FirstUseEver instead of IsWindowAppearing check)
+        ImGui::SetNextWindowSize(ImGui::GetMainViewport()->WorkSize, ImGuiCond_FirstUseEver);
+        ImGui::SetNextWindowPos(ImGui::GetMainViewport()->WorkPos, ImGuiCond_FirstUseEver);
        
        if (ImGui::Begin(window_title.c_str(), editor->active(), 
                        ImGuiWindowFlags_None)) {  // Allow full docking
@@ -801,7 +798,7 @@ absl::Status EditorManager::Update() {
          context_.session_id = prev_session_id;  // Restore previous session ID
        }
        ImGui::End();
-        ImGui::PopID();  // Pop the unique ID for this session+editor combination
+        // PopID removed to match PushID removal above
      }
    }
  }
--- a/src/app/editor/graphics/graphics_editor.cc
+++ b/src/app/editor/graphics/graphics_editor.cc
@@ -56,27 +56,35 @@ absl::Status GraphicsEditor::Update() {
  static gui::EditorCard player_anims_card("Player Animations", ICON_MD_PERSON);
  static gui::EditorCard prototype_card("Prototype Viewer", ICON_MD_CONSTRUCTION);

-  if (show_sheet_editor_ && sheet_editor_card.Begin(&show_sheet_editor_)) {
-    status_ = UpdateGfxEdit();
-    sheet_editor_card.End();
-  }
-
-  if (show_sheet_browser_ && sheet_browser_card.Begin(&show_sheet_browser_)) {
-    if (asset_browser_.Initialized == false) {
-      asset_browser_.Initialize(gfx::Arena::Get().gfx_sheets());
+  if (show_sheet_editor_) {
+    if (sheet_editor_card.Begin(&show_sheet_editor_)) {
+      status_ = UpdateGfxEdit();
    }
-    asset_browser_.Draw(gfx::Arena::Get().gfx_sheets());
-    sheet_browser_card.End();
+    sheet_editor_card.End();  // ALWAYS call End after Begin
  }

-  if (show_player_animations_ && player_anims_card.Begin(&show_player_animations_)) {
-    status_ = UpdateLinkGfxView();
-    player_anims_card.End();
+  if (show_sheet_browser_) {
+    if (sheet_browser_card.Begin(&show_sheet_browser_)) {
+      if (asset_browser_.Initialized == false) {
+        asset_browser_.Initialize(gfx::Arena::Get().gfx_sheets());
+      }
+      asset_browser_.Draw(gfx::Arena::Get().gfx_sheets());
+    }
+    sheet_browser_card.End();  // ALWAYS call End after Begin
  }

-  if (show_prototype_viewer_ && prototype_card.Begin(&show_prototype_viewer_)) {
-    status_ = UpdateScadView();
-    prototype_card.End();
+  if (show_player_animations_) {
+    if (player_anims_card.Begin(&show_player_animations_)) {
+      status_ = UpdateLinkGfxView();
+    }
+    player_anims_card.End();  // ALWAYS call End after Begin
+  }
+
+  if (show_prototype_viewer_) {
+    if (prototype_card.Begin(&show_prototype_viewer_)) {
+      status_ = UpdateScadView();
+    }
+    prototype_card.End();  // ALWAYS call End after Begin
  }

  CLEAR_AND_RETURN_STATUS(status_)
--- a/src/app/editor/graphics/screen_editor.cc
+++ b/src/app/editor/graphics/screen_editor.cc
@@ -74,25 +74,35 @@ absl::Status ScreenEditor::Update() {
  static gui::EditorCard title_screen_card("Title Screen", ICON_MD_TITLE);
  static gui::EditorCard naming_screen_card("Naming Screen", ICON_MD_EDIT_ATTRIBUTES);

-  if (show_dungeon_maps_ && dungeon_maps_card.Begin(&show_dungeon_maps_)) {
-    DrawDungeonMapsEditor();
-    dungeon_maps_card.End();
+  if (show_dungeon_maps_) {
+    if (dungeon_maps_card.Begin(&show_dungeon_maps_)) {
+      DrawDungeonMapsEditor();
+    }
+    dungeon_maps_card.End();  // ALWAYS call End after Begin
  }
-  if (show_inventory_menu_ && inventory_menu_card.Begin(&show_inventory_menu_)) {
-    DrawInventoryMenuEditor();
-    inventory_menu_card.End();
+  if (show_inventory_menu_) {
+    if (inventory_menu_card.Begin(&show_inventory_menu_)) {
+      DrawInventoryMenuEditor();
+    }
+    inventory_menu_card.End();  // ALWAYS call End after Begin
  }
-  if (show_overworld_map_ && overworld_map_card.Begin(&show_overworld_map_)) {
-    DrawOverworldMapEditor();
-    overworld_map_card.End();
+  if (show_overworld_map_) {
+    if (overworld_map_card.Begin(&show_overworld_map_)) {
+      DrawOverworldMapEditor();
+    }
+    overworld_map_card.End();  // ALWAYS call End after Begin
  }
-  if (show_title_screen_ && title_screen_card.Begin(&show_title_screen_)) {
-    DrawTitleScreenEditor();
-    title_screen_card.End();
+  if (show_title_screen_) {
+    if (title_screen_card.Begin(&show_title_screen_)) {
+      DrawTitleScreenEditor();
+    }
+    title_screen_card.End();  // ALWAYS call End after Begin
  }
-  if (show_naming_screen_ && naming_screen_card.Begin(&show_naming_screen_)) {
-    DrawNamingScreenEditor();
-    naming_screen_card.End();
+  if (show_naming_screen_) {
+    if (naming_screen_card.Begin(&show_naming_screen_)) {
+      DrawNamingScreenEditor();
+    }
+    naming_screen_card.End();  // ALWAYS call End after Begin
  }

  return status_;
--- a/src/app/editor/overworld/overworld_editor.cc
+++ b/src/app/editor/overworld/overworld_editor.cc
@@ -168,58 +168,72 @@ absl::Status OverworldEditor::Update() {
  DrawOverworldCanvas();
  
  // Floating tile selector cards (4 tabs converted to separate cards)
-  if (show_tile16_selector_ && tile16_card.Begin(&show_tile16_selector_)) {
-    status_ = DrawTile16Selector();
-    tile16_card.End();
+  if (show_tile16_selector_) {
+    if (tile16_card.Begin(&show_tile16_selector_)) {
+      status_ = DrawTile16Selector();
+    }
+    tile16_card.End();  // ALWAYS call End after Begin
  }
  
-  if (show_tile8_selector_ && tile8_card.Begin(&show_tile8_selector_)) {
-    gui::BeginPadding(3);
-    gui::BeginChildWithScrollbar("##Tile8SelectorScrollRegion");
-    DrawTile8Selector();
-    ImGui::EndChild();
-    gui::EndNoPadding();
-    tile8_card.End();
+  if (show_tile8_selector_) {
+    if (tile8_card.Begin(&show_tile8_selector_)) {
+      gui::BeginPadding(3);
+      gui::BeginChildWithScrollbar("##Tile8SelectorScrollRegion");
+      DrawTile8Selector();
+      ImGui::EndChild();
+      gui::EndNoPadding();
+    }
+    tile8_card.End();  // ALWAYS call End after Begin
  }
  
-  if (show_area_gfx_ && area_gfx_card.Begin(&show_area_gfx_)) {
-    status_ = DrawAreaGraphics();
-    area_gfx_card.End();
+  if (show_area_gfx_) {
+    if (area_gfx_card.Begin(&show_area_gfx_)) {
+      status_ = DrawAreaGraphics();
+    }
+    area_gfx_card.End();  // ALWAYS call End after Begin
  }
  
-  if (show_scratch_ && scratch_card.Begin(&show_scratch_)) {
-    status_ = DrawScratchSpace();
-    scratch_card.End();
+  if (show_scratch_) {
+    if (scratch_card.Begin(&show_scratch_)) {
+      status_ = DrawScratchSpace();
+    }
+    scratch_card.End();  // ALWAYS call End after Begin
  }
  
  // Tile16 Editor popup-only (no tab)
-  if (show_tile16_editor_ && tile16_editor_card.Begin(&show_tile16_editor_)) {
-    if (rom_->is_loaded()) {
-      status_ = tile16_editor_.Update();
-    } else {
-      gui::CenterText("No ROM loaded");
+  if (show_tile16_editor_) {
+    if (tile16_editor_card.Begin(&show_tile16_editor_)) {
+      if (rom_->is_loaded()) {
+        status_ = tile16_editor_.Update();
+      } else {
+        gui::CenterText("No ROM loaded");
+      }
    }
-    tile16_editor_card.End();
+    tile16_editor_card.End();  // ALWAYS call End after Begin
  }
  
  // Graphics Groups popup
-  if (show_gfx_groups_ && gfx_groups_card.Begin(&show_gfx_groups_)) {
-    if (rom_->is_loaded()) {
-      status_ = gfx_group_editor_.Update();
-    } else {
-      gui::CenterText("No ROM loaded");
+  if (show_gfx_groups_) {
+    if (gfx_groups_card.Begin(&show_gfx_groups_)) {
+      if (rom_->is_loaded()) {
+        status_ = gfx_group_editor_.Update();
+      } else {
+        gui::CenterText("No ROM loaded");
+      }
    }
-    gfx_groups_card.End();
+    gfx_groups_card.End();  // ALWAYS call End after Begin
  }
  
  // Usage Statistics popup
-  if (show_usage_stats_ && usage_stats_card.Begin(&show_usage_stats_)) {
-    if (rom_->is_loaded()) {
-      status_ = UpdateUsageStats();
-    } else {
-      gui::CenterText("No ROM loaded");
+  if (show_usage_stats_) {
+    if (usage_stats_card.Begin(&show_usage_stats_)) {
+      if (rom_->is_loaded()) {
+        status_ = UpdateUsageStats();
+      } else {
+        gui::CenterText("No ROM loaded");
+      }
    }
-    usage_stats_card.End();
+    usage_stats_card.End();  // ALWAYS call End after Begin
  }

  // Area Configuration Panel (detailed editing)
@@ -1254,7 +1268,6 @@ void OverworldEditor::DrawOverworldCanvas() {
  gui::BeginNoPadding();
  gui::BeginChildBothScrollbars(7);
  ow_map_canvas_.DrawBackground();
-  gui::EndNoPadding();

  // Setup dynamic context menu based on current map state (Phase 3B)
  if (rom_->is_loaded() && overworld_.is_loaded() && map_properties_system_) {
@@ -1314,6 +1327,7 @@ void OverworldEditor::DrawOverworldCanvas() {
  ow_map_canvas_.DrawGrid();
  ow_map_canvas_.DrawOverlay();
  EndChild();
+  gui::EndNoPadding();  // End the no-padding style that was started at line 1254
 }

 absl::Status OverworldEditor::DrawTile16Selector() {
@@ -1344,7 +1358,13 @@ absl::Status OverworldEditor::DrawTile16Selector() {

  if (result.selection_changed) {
    current_tile16_ = result.selected_tile;
-    RETURN_IF_ERROR(tile16_editor_.SetCurrentTile(current_tile16_));
+    auto status = tile16_editor_.SetCurrentTile(current_tile16_);
+    if (!status.ok()) {
+      // Store error but ensure we close the child before returning
+      EndChild();
+      ImGui::EndGroup();
+      return status;
+    }
    // Note: We do NOT auto-scroll here because it breaks user interaction.
    // The canvas should only scroll when explicitly requested (e.g., when
    // selecting a tile from the overworld canvas via ScrollBlocksetCanvasToCurrentTile).
--- a/src/app/editor/sprite/sprite_editor.cc
+++ b/src/app/editor/sprite/sprite_editor.cc
@@ -43,14 +43,18 @@ absl::Status SpriteEditor::Update() {
  static gui::EditorCard vanilla_card("Vanilla Sprites", ICON_MD_PEST_CONTROL_RODENT);
  static gui::EditorCard custom_card("Custom Sprites", ICON_MD_ADD_MODERATOR);

-  if (show_vanilla_editor_ && vanilla_card.Begin(&show_vanilla_editor_)) {
-    DrawVanillaSpriteEditor();
-    vanilla_card.End();
+  if (show_vanilla_editor_) {
+    if (vanilla_card.Begin(&show_vanilla_editor_)) {
+      DrawVanillaSpriteEditor();
+    }
+    vanilla_card.End();  // ALWAYS call End after Begin
  }

-  if (show_custom_editor_ && custom_card.Begin(&show_custom_editor_)) {
-    DrawCustomSprites();
-    custom_card.End();
+  if (show_custom_editor_) {
+    if (custom_card.Begin(&show_custom_editor_)) {
+      DrawCustomSprites();
+    }
+    custom_card.End();  // ALWAYS call End after Begin
  }

  return status_.ok() ? absl::OkStatus() : status_;