feat(UI): delete theme button

[brucenguyen]

Now that we can detect shipped versus user presets, we can show a delete button.

Summary by CodeRabbit

• New Features

  • Users can delete custom themes via a confirmed "Delete Theme" dialog to prevent accidental removal.
  • Deletion is available from both the theme list and the "Create New Theme" popup.
  • After deletion the app reloads themes and switches back to the default preset automatically; canceling closes the dialog.
  • Improved confirmation messaging and consistent tooltip feedback for delete actions.

• Style

  • Improved spacing and inline placement of action buttons for a cleaner theme settings layout.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cab4bae and 1cf68e1.

📒 Files selected for processing (1)

• src/Menu/SettingsTabRenderer.cpp

---

📝 Walkthrough

Walkthrough

Adds a delete-confirmation popup and Delete button for non-preset themes in SettingsTabRenderer; deletion calls SafeDelete on the theme file, refreshes themes, loads the "Default" preset, updates currentThemeInfo/currentThemePreset usage, and clears modal state. Also adjusts ImGui spacing for Selected Theme display. (41 words)

Changes

Cohort / File(s)

Summary

Theme deletion & UI adjustments src/Menu/SettingsTabRenderer.cpp

Introduces static deleteThemePopup state and delete-confirmation modal; shows inline Delete button for non-preset themes with filePath and tooltip; on confirm calls SafeDelete, refreshes themes and loads "Default" preset; moves currentThemeInfo retrieval earlier and uses it across Save/Delete flows; adjusts ImGui::SameLine(0, 0) spacing; mirrors delete flow in Create New Theme popup and ensures modal state cleanup.

Sequence Diagram

sequenceDiagram
    participant User
    participant UI as SettingsTabRenderer
    participant Modal as DeleteModal
    participant FS as ThemeFileSystem
    participant Manager as ThemeManager

    User->>UI: Click Delete button for selected theme
    UI->>Modal: Open "Delete Theme" popup (deleteThemePopup = true)
    Modal->>User: Show theme name and Confirm/Cancel
    alt Confirm
        User->>Modal: Click Confirm
        Modal->>FS: SafeDelete(theme file path)
        FS-->>Modal: success/failure
        Modal->>Manager: Refresh themes
        Manager-->>Modal: themes reloaded
        Modal->>Manager: Load "Default" preset
        Manager-->>Modal: Default loaded
        Modal->>UI: Close popup, reset deleteThemePopup and related UI state
    else Cancel
        User->>Modal: Click Cancel
        Modal->>UI: Close popup, reset deleteThemePopup
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• feat(ui): theme management fixes and enhancements #1700 — Edits the same SettingsTabRenderer.cpp theme UI flows (create/edit theme UI), likely overlaps with delete-modal changes.

Suggested reviewers

• alandtse
• doodlum

Poem

🐇 I nudged the theme with trembling paw,
A little popup asked me to withdraw.
The file hopped off, the default came near,
I wiggled my whiskers and gave a cheer.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main feature addition: a delete button for theme management in the UI.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

No actionable suggestions for changed features.

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/Menu/SettingsTabRenderer.cpp`:
- Around line 749-754: Before deleting, save the current preset string (e.g.,
previousPreset = currentThemePreset); after SafeDelete succeeds, check the
boolean return of globals::menu->LoadThemePreset("Default") and if it returns
false then attempt to restore state by calling themeManager->RefreshThemes() and
trying globals::menu->LoadThemePreset(previousPreset) and resetting
currentThemePreset = previousPreset; if that restore also fails then clear
currentThemePreset (or set to an explicit fallback like "") and log an error so
the UI/state is consistent. Ensure you reference Util::FileHelpers::SafeDelete,
themeManager->RefreshThemes(), globals::menu->LoadThemePreset(...) and
currentThemePreset in the change.
- Around line 728-762: BeginPopupModal is being combined with additional
conditions which can skip calling ImGui::EndPopup() and corrupt ImGui's popup
stack; change the conditional so ImGui::BeginPopupModal("Delete Theme",
&showDeleteThemePopup, ImGuiWindowFlags_AlwaysAutoResize) is evaluated on its
own and then inside that block check currentThemeInfo and
currentThemeInfo->filePath.empty() before rendering the delete UI. Ensure
ImGui::EndPopup() is always called when BeginPopupModal returns true; keep the
delete logic that calls
Util::FileHelpers::SafeDelete(currentThemeInfo->filePath, "Theme '" +
currentThemePreset + "'"), themeManager->RefreshThemes(),
globals::menu->LoadThemePreset("Default") and the logger::warn(...) in the inner
conditional that verifies currentThemeInfo.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8804d94 and 20605df.

📒 Files selected for processing (1)

• src/Menu/SettingsTabRenderer.cpp

[coderabbitai]

♻️ Duplicate comments (2)

src/Menu/SettingsTabRenderer.cpp (2)

749-754: ⚠️ Potential issue | 🟠 Major

Ignoring LoadThemePreset("Default") return leaves UI in an inconsistent state on failure.

If LoadThemePreset fails (e.g. the Default theme file is missing or corrupt), currentThemePreset is still unconditionally set to "Default" and no error is surfaced, leaving the user looking at a stale or blank theme.

🔧 Proposed fix

 if (result.success) {
     themeManager->RefreshThemes();
-    globals::menu->LoadThemePreset("Default");
-    currentThemePreset = "Default";
+    if (globals::menu->LoadThemePreset("Default")) {
+        currentThemePreset = "Default";
+    } else {
+        logger::error("Deleted theme '{}' but failed to load fallback 'Default'", currentThemePreset);
+        globals::menu->GetSettings().SelectedThemePreset = "Default";
+    }
 } else {

As per coding guidelines src/**/*.{h,hpp,cpp}: "Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Menu/SettingsTabRenderer.cpp` around lines 749 - 754, When deleting a
theme, the code calls themeManager->RefreshThemes() and then unconditionally
calls globals::menu->LoadThemePreset("Default") and sets currentThemePreset =
"Default" even if LoadThemePreset fails; change this by capturing the boolean
return of globals::menu->LoadThemePreset("Default"), check its success, and only
set currentThemePreset when LoadThemePreset returns true; if LoadThemePreset
fails, log an error (use logger::error or logger::warn with the failure context
and include the failure reason if available) and avoid changing
currentThemePreset so the UI stays consistent (optionally trigger a graceful
fallback or user notification instead of silently proceeding).

---

728-761: ⚠️ Potential issue | 🔴 Critical

EndPopup() will be skipped when currentThemeInfo is null — ImGui popup stack corruption.

The condition on line 728 short-circuits EndPopup() on line 760 whenever currentThemeInfo is null or filePath is empty. ImGui requires EndPopup() to be called unconditionally whenever BeginPopupModal returns true; skipping it corrupts the internal popup stack and can produce assertion failures or rendering glitches.

🐛 Proposed fix

-if (ImGui::BeginPopupModal("Delete Theme", &showDeleteThemePopup, ImGuiWindowFlags_AlwaysAutoResize) && currentThemeInfo && !currentThemeInfo->filePath.empty()) {
-    ImGui::Text("Are you sure you want to delete the theme ");
-    // ... rest of content ...
-    ImGui::EndPopup();
-}
+if (ImGui::BeginPopupModal("Delete Theme", &showDeleteThemePopup, ImGuiWindowFlags_AlwaysAutoResize)) {
+    if (currentThemeInfo && !currentThemeInfo->filePath.empty()) {
+        ImGui::Text("Are you sure you want to delete the theme ");
+        // ... rest of content ...
+    } else {
+        showDeleteThemePopup = false;
+        ImGui::CloseCurrentPopup();
+    }
+    ImGui::EndPopup();
+}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Menu/SettingsTabRenderer.cpp` around lines 728 - 761, The
BeginPopupModal/EndPopup pairing is conditional on currentThemeInfo and its
filePath which can skip ImGui::EndPopup() and corrupt the popup stack; change
the code so ImGui::BeginPopupModal("Delete Theme", &showDeleteThemePopup,
ImGuiWindowFlags_AlwaysAutoResize) is the only condition for entering the block
and ensure ImGui::EndPopup() is always called before exiting that block, while
performing null/filePath checks only for the UI elements and delete logic
(references: currentThemeInfo, currentThemeInfo->filePath, currentThemePreset,
themeManager->RefreshThemes(), Util::FileHelpers::SafeDelete).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/Menu/SettingsTabRenderer.cpp`:
- Around line 749-754: When deleting a theme, the code calls
themeManager->RefreshThemes() and then unconditionally calls
globals::menu->LoadThemePreset("Default") and sets currentThemePreset =
"Default" even if LoadThemePreset fails; change this by capturing the boolean
return of globals::menu->LoadThemePreset("Default"), check its success, and only
set currentThemePreset when LoadThemePreset returns true; if LoadThemePreset
fails, log an error (use logger::error or logger::warn with the failure context
and include the failure reason if available) and avoid changing
currentThemePreset so the UI stays consistent (optionally trigger a graceful
fallback or user notification instead of silently proceeding).
- Around line 728-761: The BeginPopupModal/EndPopup pairing is conditional on
currentThemeInfo and its filePath which can skip ImGui::EndPopup() and corrupt
the popup stack; change the code so ImGui::BeginPopupModal("Delete Theme",
&showDeleteThemePopup, ImGuiWindowFlags_AlwaysAutoResize) is the only condition
for entering the block and ensure ImGui::EndPopup() is always called before
exiting that block, while performing null/filePath checks only for the UI
elements and delete logic (references: currentThemeInfo,
currentThemeInfo->filePath, currentThemePreset, themeManager->RefreshThemes(),
Util::FileHelpers::SafeDelete).

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 20605df and f13d2bf.

📒 Files selected for processing (1)

• src/Menu/SettingsTabRenderer.cpp

[coderabbitai]

♻️ Duplicate comments (1)

src/Menu/SettingsTabRenderer.cpp (1)

747-753: LoadThemePreset("Default") return value is still unchecked.

This was raised in a previous review. If the "Default" theme fails to load after a successful delete, the UI state (currentThemePreset = "Default") may not match the actual loaded theme. Consider checking the return value and logging on failure for graceful degradation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Menu/SettingsTabRenderer.cpp` around lines 747 - 753, After deleting a
theme, the call to globals::menu->LoadThemePreset("Default") is unchecked so
currentThemePreset may be set incorrectly; modify the block handling
result.success to capture and check the boolean return of
LoadThemePreset("Default") (or its equivalent return type), log an error with
context if it fails (including result.errorMessage where relevant), and only set
currentThemePreset = "Default" when LoadThemePreset succeeds; keep
themeManager->RefreshThemes() as-is but ensure failure paths do not alter UI
state.

🧹 Nitpick comments (1)

src/Menu/SettingsTabRenderer.cpp (1)

744-756: Ensure currentThemeInfo is explicitly invalidated after RefreshThemes() to prevent accidental reuse in future edits.

GetThemeInfo() returns a pointer into the themes container. When RefreshThemes() is called, it clears that container (via DiscoverThemes() at line 456), invalidating the pointer. While the current code doesn't dereference currentThemeInfo after the refresh, assigning it to nullptr makes this invalidation explicit and prevents subtle bugs if the code is edited later.

Proposed safeguard

 if (result.success) {
     themeManager->RefreshThemes();
+    currentThemeInfo = nullptr;  // invalidated by RefreshThemes
     globals::menu->LoadThemePreset("Default");
     currentThemePreset = "Default";
 } else {

Since currentThemeInfo is declared as const auto* (i.e., const ThemeInfo*—a non-const pointer to const data), reassignment to nullptr is valid.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Menu/SettingsTabRenderer.cpp` around lines 744 - 756, After calling
themeManager->RefreshThemes() (inside the Delete button handler), explicitly
invalidate the pointer returned by GetThemeInfo by assigning currentThemeInfo =
nullptr immediately after RefreshThemes(); do this before calling
globals::menu->LoadThemePreset("Default")/changing currentThemePreset to ensure
you don't hold a dangling const ThemeInfo* after the container is cleared.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/Menu/SettingsTabRenderer.cpp`:
- Around line 747-753: After deleting a theme, the call to
globals::menu->LoadThemePreset("Default") is unchecked so currentThemePreset may
be set incorrectly; modify the block handling result.success to capture and
check the boolean return of LoadThemePreset("Default") (or its equivalent return
type), log an error with context if it fails (including result.errorMessage
where relevant), and only set currentThemePreset = "Default" when
LoadThemePreset succeeds; keep themeManager->RefreshThemes() as-is but ensure
failure paths do not alter UI state.

---

Nitpick comments:
In `@src/Menu/SettingsTabRenderer.cpp`:
- Around line 744-756: After calling themeManager->RefreshThemes() (inside the
Delete button handler), explicitly invalidate the pointer returned by
GetThemeInfo by assigning currentThemeInfo = nullptr immediately after
RefreshThemes(); do this before calling
globals::menu->LoadThemePreset("Default")/changing currentThemePreset to ensure
you don't hold a dangling const ThemeInfo* after the container is cleared.

---

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f13d2bf and cab4bae.

📒 Files selected for processing (1)

• src/Menu/SettingsTabRenderer.cpp

[github-actions]

✅ A pre-release build is available for this PR:
Download

[soda3000]

Looks good, definitely a good idea to have a delete option for themes.