GH-1199 Fix ColorPicker closing when pressing spacebar

[JoltedJon]

Fixes #1199
Before when you press spacebar it would cause the window to be closed, now spacebar can be used in the popup window.

Pressing escape, or clicking off the colorpicker still works in closing the color picker.

Summary by CodeRabbit

• Bug Fixes
  • Color picker popup no longer closes when the accept action is pressed.
  • Popup continues to close as expected when the cancel action is used.

[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 d482044 and 8191f31.

📒 Files selected for processing (1)

• scene/gui/color_picker.cpp

💤 Files with no reviewable changes (1)

• scene/gui/color_picker.cpp

---

Walkthrough

Removed handling of the ui_accept input in the color picker popup; the panel no longer intercepts that action and now defers input handling to PopupPanel::_input_from_window, so pressing the accept action no longer closes the popup.

Changes

Cohort / File(s)	Summary
Color Picker Input Handling scene/gui/color_picker.cpp	Removed branch that handled the ui_accept action in ColorPickerPopupPanel::_input_from_window; the method now delegates to PopupPanel::_input_from_window(p_event). No public API/signature changes.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: removing spacebar handling that was causing ColorPicker to close.
Linked Issues check	✅ Passed	The change directly addresses issue #1199 by removing the ui_accept action handler that was closing the picker when spacebar was pressed.
Out of Scope Changes check	✅ Passed	The change is narrowly scoped to fixing the identified issue with no extraneous modifications observed.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 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.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

scene/gui/color_picker.cpp (1)

2417-2422: ⚠️ Potential issue | 🟠 Major

Remove the redundant ui_cancel check to avoid double-close.

The fix to use ui_cancel instead of ui_accept is correct. However, the explicit ui_cancel check at lines 2418–2420 is redundant and causes _close_pressed() to be called twice:

1. First call: ColorPickerPopupPanel::_input_from_window explicitly checks ui_cancel and calls _close_pressed() (line 2419)
2. Second call: The subsequent PopupPanel::_input_from_window(p_event) (line 2421) delegates to Popup::_input_from_window, which also checks ui_cancel and calls _close_pressed()

Since Popup::_input_from_window (the grandparent class) already handles ui_cancel natively, the explicit check in ColorPickerPopupPanel should be removed entirely. The base class behavior is sufficient.

Class hierarchy showing redundant ui_cancel handling

// Popup::_input_from_window (popup.cpp:42-48)
if (get_flag(FLAG_POPUP) && p_event->is_action_pressed(SNAME("ui_cancel"), false, true)) {
    hide_reason = HIDE_REASON_CANCELED;
    _close_pressed();
}
Window::_input_from_window(p_event);

// PopupPanel::_input_from_window (popup.cpp:248-270)
// ... handles mouse clicks ...
Popup::_input_from_window(p_event);  // Delegates to parent

// ColorPickerPopupPanel::_input_from_window (current)
if (p_event->is_action_pressed(SNAME("ui_cancel"), false, true)) {
    _close_pressed();  // REDUNDANT - Popup already handles this
}
PopupPanel::_input_from_window(p_event);  // Will trigger Popup's ui_cancel check again

Recommended fix: Remove lines 2418–2420 entirely and just call PopupPanel::_input_from_window(p_event) directly.

🤖 Prompt for AI Agents

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

In `@scene/gui/color_picker.cpp` around lines 2417 - 2422, Remove the redundant
explicit ui_cancel handling in ColorPickerPopupPanel::_input_from_window: delete
the if block that checks p_event->is_action_pressed(SNAME("ui_cancel"), ...) and
calls _close_pressed(), and let the existing
PopupPanel::_input_from_window(p_event) call handle ui_cancel via the
Popup::_input_from_window path; this prevents _close_pressed() from being
invoked twice.

🤖 Prompt for all review comments with AI agents

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

Outside diff comments:
In `@scene/gui/color_picker.cpp`:
- Around line 2417-2422: Remove the redundant explicit ui_cancel handling in
ColorPickerPopupPanel::_input_from_window: delete the if block that checks
p_event->is_action_pressed(SNAME("ui_cancel"), ...) and calls _close_pressed(),
and let the existing PopupPanel::_input_from_window(p_event) call handle
ui_cancel via the Popup::_input_from_window path; this prevents _close_pressed()
from being invoked twice.

---

ℹ️ 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 82ff6fe and d482044.

📒 Files selected for processing (1)

• scene/gui/color_picker.cpp

[Arctis-Fireblight]

Approved.