Enable keyboard & dpad navigation for advanced settings - #401
Merged
ikostan merged 24 commits intoFeb 15, 2026
Merged
Conversation
ikostan
added this to the
Milestone 11: Polish & Platform-wide Enhancements (Menus, Testing & Configuration) milestone
6 tasks
Contributor
Reviewer's GuideCentralizes menu focus handling and updates main, options, pause, and advanced settings menus to use a safer, logging-backed initial-focus helper, while ensuring focus is restored correctly when exiting Advanced Settings. Sequence diagram for focus handling when entering and exiting Advanced SettingssequenceDiagram
actor Player
participant MainMenu
participant OptionsMenu
participant AdvancedSettings
participant Globals
participant Viewport
Player->>MainMenu: Open game
MainMenu->>Globals: ensure_initial_focus(start_button, [start_button, options_button, quit_button], Main_Menu)
Globals->>Viewport: gui_get_focus_owner()
alt No menu control has focus
Globals-->>MainMenu: call_deferred(grab_focus on start_button)
else Menu control already has focus
Globals-->>MainMenu: Skip initial focus grab
end
Player->>MainMenu: Press options_button
MainMenu->>OptionsMenu: Show OptionsMenu
OptionsMenu->>OptionsMenu: _grab_first_button_focus()
OptionsMenu->>Globals: ensure_initial_focus(candidate_button, [advanced_settings_button, audio_settings_button, key_mapping_button, gameplay_settings_button, options_back_button], Options_Menu)
Globals->>Viewport: gui_get_focus_owner()
alt No options control has focus
Globals-->>OptionsMenu: call_deferred(grab_focus on candidate_button)
else Options control already has focus
Globals-->>OptionsMenu: Skip initial focus grab
end
Player->>OptionsMenu: Activate advanced_settings_button
OptionsMenu->>AdvancedSettings: Show AdvancedSettings
AdvancedSettings->>Globals: ensure_initial_focus(log_lvl_option, [log_lvl_option, advanced_back_button, advanced_reset_button], Advanced_Settings)
Globals->>Viewport: gui_get_focus_owner()
alt No advanced control has focus
Globals-->>AdvancedSettings: call_deferred(grab_focus on log_lvl_option)
else Advanced control already has focus
Globals-->>AdvancedSettings: Skip initial focus grab
end
Player->>AdvancedSettings: Press advanced_back_button
AdvancedSettings->>OptionsMenu: Show prev_menu (OptionsMenu)
AdvancedSettings->>OptionsMenu: Get advanced_settings_button
alt advanced_settings_button is valid
AdvancedSettings->>OptionsMenu: call_deferred(grab_focus on advanced_settings_button)
else advanced_settings_button invalid
AdvancedSettings->>AdvancedSettings: Log warning and keep current focus
end
Class diagram for menu focus handling with Globals.ensure_initial_focusclassDiagram
class Globals {
<<singleton>>
+LogLevel DEBUG
+LogLevel WARNING
+ensure_initial_focus(candidate Control, allowed_controls Array~Control~, context String) void
+log_message(message String, level LogLevel) void
}
class MainMenu {
+start_button Button
+options_button Button
+quit_button Button
+_ready() void
+_on_start_pressed() void
}
class OptionsMenu {
<<CanvasLayer>>
+advanced_settings_button Button
+audio_settings_button Button
+key_mapping_button Button
+gameplay_settings_button Button
+options_back_button Button
+options_vbox VBoxContainer
+_ready() void
+_grab_first_button_focus() void
}
class AdvancedSettings {
<<CanvasLayer>>
+log_lvl_option Control
+advanced_back_button Button
+advanced_reset_button Button
+_ready() void
+_on_advanced_back_button_pressed() void
+_on_advanced_reset_js() void
}
class PauseMenu {
<<CanvasLayer>>
+resume_button Button
+options_button Button
+back_to_main_button Button
+toggle_pause() void
+_on_resume_button_pressed() void
}
class Viewport {
+gui_get_focus_owner() Control
}
Globals ..> Viewport : uses
MainMenu ..> Globals : calls_ensure_initial_focus
OptionsMenu ..> Globals : calls_ensure_initial_focus
AdvancedSettings ..> Globals : calls_ensure_initial_focus
PauseMenu ..> Globals : calls_ensure_initial_focus
AdvancedSettings ..> OptionsMenu : restores_focus_to_advanced_settings_button
MainMenu o-- Button : owns
OptionsMenu o-- Button : owns
OptionsMenu o-- VBoxContainer : owns
AdvancedSettings o-- Button : owns
AdvancedSettings o-- Control : owns
PauseMenu o-- Button : owns
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds centralized initial-focus logic and guarded/deferred focus grabs across menus; wires explicit focus neighbors in Advanced Settings scene and replaces several StyleBox resources with StyleBoxFlat variants. No public/exported API signature removals. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (keyboard/controller)
participant AdvScene as AdvancedSettings (scene)
participant AdvScript as AdvancedSettings.gd
participant Globals as Globals.ensure_initial_focus
participant PrevMenu as OptionsMenu
participant Engine as GodotEngine
User->>AdvScene: open Advanced Settings
AdvScene->>AdvScript: _ready()
AdvScript->>Globals: ensure_initial_focus(LogLevelControl, [Back,Reset], "Advanced Settings")
Globals->>Engine: if no allowed control has focus -> defer candidate.grab_focus()
Engine-->>Globals: grab scheduled / ignored
User->>AdvScene: press BackButton
AdvScript->>PrevMenu: reveal_previous_menu()
AdvScript->>PrevMenu: find AdvancedSettingsButton
alt AdvancedSettingsButton found
AdvScript->>Globals: ensure_initial_focus(AdvancedSettingsButton, [Start,Options,Quit], "Options Menu")
Globals->>Engine: defer AdvancedSettingsButton.grab_focus()
else not found
AdvScript->>Engine: log warning
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
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. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider guarding the
log_lvl_option.call_deferred("grab_focus")call with a null/validity check to avoid potential runtime errors if the node is not found or renamed. - The hard-coded node path
"Panel/OptionsVBoxContainer/AdvancedSettingsButton"couples focus logic tightly to the scene hierarchy; consider exposing this button via an exported NodePath or onready variable so hierarchy changes don’t silently break focus restoration.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider guarding the `log_lvl_option.call_deferred("grab_focus")` call with a null/validity check to avoid potential runtime errors if the node is not found or renamed.
- The hard-coded node path `"Panel/OptionsVBoxContainer/AdvancedSettingsButton"` couples focus logic tightly to the scene hierarchy; consider exposing this button via an exported NodePath or onready variable so hierarchy changes don’t silently break focus restoration.
## Individual Comments
### Comment 1
<location> `scripts/advanced_settings.gd:186-187` </location>
<code_context>
Globals.log_message("Showing menu: " + prev_menu.name, Globals.LogLevel.DEBUG)
hidden_menu_found = true
+
+ # Focus on AdvancedSettingsButton
+ var advanced_btn: Button = prev_menu.get_node(
+ "Panel/OptionsVBoxContainer/AdvancedSettingsButton"
+ )
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `get_node` here can throw if the button path changes or is missing; consider a safer lookup.
Since `get_node` will error if `Panel/OptionsVBoxContainer/AdvancedSettingsButton` is missing, this can fail before `is_instance_valid` runs. To harden this, use `get_node_or_null` and check for null (or otherwise guard the lookup) so the handler still works if the path changes or the button isn’t present.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scenes/advanced_settings.tscn`:
- Around line 167-168: The controls currently only set focus_next and
focus_previous (e.g., focus_next = NodePath("../../BtnContainer/BackButton" and
focus_previous = NodePath("../../BtnContainer/ResetButton")) which only affects
Tab navigation; add explicit D‑Pad/arrow navigation properties by setting
focus_neighbor_top, focus_neighbor_bottom, focus_neighbor_left, and
focus_neighbor_right for the same UI controls so the engine uses deterministic
neighbors instead of spatial auto-detection—use the same NodePath targets
(BtnContainer/BackButton, BtnContainer/ResetButton, and any adjacent buttons
referenced at the other locations called out in the comment) when assigning each
focus_neighbor_* property.
🧹 Nitpick comments (2)
scenes/advanced_settings.tscn (1)
72-86: Duplicate StyleBoxFlat resources:StyleBoxFlat_cfdt5andStyleBoxFlat_itopbare identical.Both sub-resources have the same
bg_color,corner_radius_*, andshadow_sizevalues. They can be consolidated into a single resource referenced by bothBackButtonandResetButton, reducing scene bloat.scripts/advanced_settings.gd (1)
185-194: Useget_node_or_nullinstead ofget_nodeto avoid error noise when the path is missing.If
prev_menudoesn't have the expected node hierarchy,get_node()will print an error to the console even thoughis_instance_validhandlesnullgracefully on the next line.get_node_or_null()avoids the spurious error output.Suggested fix
# Focus on AdvancedSettingsButton - var advanced_btn: Button = prev_menu.get_node( + var advanced_btn: Button = prev_menu.get_node_or_null( "Panel/OptionsVBoxContainer/AdvancedSettingsButton" )
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
scenes/advanced_settings.tscnscripts/advanced_settings.gd
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Browser Functional Tests / test
🔇 Additional comments (1)
scripts/advanced_settings.gd (1)
88-89: LGTM — deferred focus grab ensures the UI is ready before focusing.Using
call_deferredis the correct approach for grabbing focus during_ready.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
focus_next/focus_previous only control Tab/Shift+Tab navigation. D-Pad and arrow key navigation in Godot 4 requires explicit focus_neighbor_top, focus_neighbor_bottom, focus_neighbor_left, and focus_neighbor_right properties. Without them, the engine falls back to spatial auto-detection, which may not produce the intended focus cycle.
…anges or is missing; consider a safer lookup. Since get_node will error if Panel/OptionsVBoxContainer/AdvancedSettingsButton is missing, this can fail before is_instance_valid runs. To harden this, use get_node_or_null and check for null (or otherwise guard the lookup) so the handler still works if the path changes or the button isn’t present.
Consider guarding the log_lvl_option.call_deferred("grab_focus") call with a null/validity check to avoid potential runtime errors if the node is not found or renamed.
Owner
Author
All addressed |
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The focus management for
advanced_btnrelies on a hard-coded node path ("Panel/OptionsVBoxContainer/AdvancedSettingsButton"); consider using an exportedNodePathor a more robust lookup mechanism so UI refactors don’t silently break focus restoration. - The new warning logs for missing
log_lvl_optionandAdvancedSettingsButtonmight be noisy in normal play; if these are non-critical, consider downgrading them to DEBUG or gating them behind a verbose flag.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The focus management for `advanced_btn` relies on a hard-coded node path (`"Panel/OptionsVBoxContainer/AdvancedSettingsButton"`); consider using an exported `NodePath` or a more robust lookup mechanism so UI refactors don’t silently break focus restoration.
- The new warning logs for missing `log_lvl_option` and `AdvancedSettingsButton` might be noisy in normal play; if these are non-critical, consider downgrading them to DEBUG or gating them behind a verbose flag.
## Individual Comments
### Comment 1
<location> `scripts/advanced_settings.gd:191` </location>
<code_context>
Globals.log_message("Showing menu: " + prev_menu.name, Globals.LogLevel.DEBUG)
hidden_menu_found = true
+
+ # Focus on AdvancedSettingsButton
+ var advanced_btn: Button = prev_menu.get_node_or_null(
+ "Panel/OptionsVBoxContainer/AdvancedSettingsButton"
</code_context>
<issue_to_address>
**suggestion:** Hard-coded node path for the AdvancedSettingsButton may be brittle across scene/layout changes.
This hard-coded path couples the script to the exact scene hierarchy, so any UI restructure (renames, reparenting) will break focus without errors. Prefer wiring this via an exported NodePath, a group, or an onready var set from the scene to keep it stable across layout changes.
Suggested implementation:
```
prev_menu.visible = true
Globals.log_message("Showing menu: " + prev_menu.name, Globals.LogLevel.DEBUG)
hidden_menu_found = true
# Focus on AdvancedSettingsButton
var advanced_btn: Button = null
# Prefer a configurable NodePath (exported/onready) if available, to avoid coupling
# to a specific scene hierarchy. Fall back to the legacy hard-coded path if needed.
if typeof(advanced_settings_button_path) == TYPE_NODE_PATH \
and String(advanced_settings_button_path) != "" \
and prev_menu.has_node(advanced_settings_button_path):
advanced_btn = prev_menu.get_node(advanced_settings_button_path)
else:
advanced_btn = prev_menu.get_node_or_null(
"Panel/OptionsVBoxContainer/AdvancedSettingsButton"
)
if is_instance_valid(advanced_btn):
advanced_btn.call_deferred("grab_focus")
Globals.log_message(
"Focused on AdvancedSettingsButton after back.", Globals.LogLevel.DEBUG
)
else:
Globals.log_message(
"AdvancedSettingsButton not found—skipping focus.", Globals.LogLevel.WARNING
)
```
To fully implement the suggestion and make this robust, you should also:
1. Declare a configurable `NodePath` in `scripts/advanced_settings.gd` (near the top of the script):
- For Godot 4.x:
- `@export var advanced_settings_button_path: NodePath`
- For Godot 3.x:
- `export(NodePath) var advanced_settings_button_path`
2. In the scene(s) using this script, assign `advanced_settings_button_path` to point to the AdvancedSettingsButton node **relative to `prev_menu`** (i.e., the menu you are returning to).
For example, if the button is now under `Buttons/AdvancedSettingsButton`, set the property to `Buttons/AdvancedSettingsButton`.
3. Once all scenes are updated to use the exported path, you may optionally remove the fallback hard-coded path block to fully decouple the script from the old layout.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…e brittle across scene/layout changes. This hard-coded path couples the script to the exact scene hierarchy, so any UI restructure (renames, reparenting) will break focus without errors. Prefer wiring this via an exported NodePath, a group, or an onready var set from the scene to keep it stable across layout changes.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
_on_advanced_back_button_pressed, the warning"AdvancedSettingsButton not found—skipping focus."will also trigger whenprev_menuis not anOptionsMenu; consider only logging a warning whenprev_menu is OptionsMenuto avoid noisy logs in other navigation flows. - The initial focus on
log_lvl_optionin_ready()is deferred correctly, but you might want to guard this behavior behind a flag or check whether another control already has meaningful focus to avoid unexpectedly overriding user-driven focus when the menu is reopened.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_on_advanced_back_button_pressed`, the warning `"AdvancedSettingsButton not found—skipping focus."` will also trigger when `prev_menu` is not an `OptionsMenu`; consider only logging a warning when `prev_menu is OptionsMenu` to avoid noisy logs in other navigation flows.
- The initial focus on `log_lvl_option` in `_ready()` is deferred correctly, but you might want to guard this behavior behind a flag or check whether another control already has meaningful focus to avoid unexpectedly overriding user-driven focus when the menu is reopened.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
In _on_advanced_back_button_pressed, the warning "AdvancedSettingsButton not found—skipping focus." will also trigger when prev_menu is not an OptionsMenu; consider only logging a warning when prev_menu is OptionsMenu to avoid noisy logs in other navigation flows.
Check for an existing GUI focus owner before calling grab_focus in multiple menus (advanced_settings.gd, main_menu.gd, options_menu.gd, pause_menu.gd). Add debug/warning logs when focus is already set or when target controls are missing, and return early where appropriate to avoid overriding user or system focus. Minor comment/whitespace cleanups included. The initial focus on log_lvl_option in _ready() is deferred correctly, but you might want to guard this behavior behind a flag or check whether another control already has meaningful focus to avoid unexpectedly overriding user-driven focus when the menu is reopened.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/advanced_settings.gd`:
- Around line 88-97: The comment "Give keyboard focus to the log level slider"
is misleading because the control is an OptionButton; update the comment near
the block that references log_lvl_option (and related calls
get_viewport().gui_get_focus_owner() and
log_lvl_option.call_deferred("grab_focus")) to say something like "Give keyboard
focus to the log level OptionButton" or similar to accurately reflect the
control type.
🧹 Nitpick comments (2)
scripts/pause_menu.gd (1)
69-74: Unnecessaryreturnon line 74.The
elsebranch'sreturnon line 74 has no effect — there is no code after thisif/elseblock intoggle_pause(). It's harmless but slightly misleading, as it suggests there's subsequent logic being skipped.Suggested cleanup
if visible and is_instance_valid(resume_button): if get_viewport().gui_get_focus_owner() == null: resume_button.call_deferred("grab_focus") Globals.log_message("Grabbed initial focus on: resume_button" , Globals.LogLevel.DEBUG) else: Globals.log_message("Focus already set—skipping initial grab.", Globals.LogLevel.DEBUG) - returnscripts/options_menu.gd (1)
135-143: Hoist the focus-owner check out of the loop.
gui_get_focus_owner()is evaluated on every loop iteration but its result won't change within this synchronous function. Moving the check before theforloop avoids redundant viewport queries and makes the intent clearer.Suggested refactor
func _grab_first_button_focus() -> void: + if get_viewport().gui_get_focus_owner() != null: + Globals.log_message("Focus already set—skipping initial grab.", Globals.LogLevel.DEBUG) + return for child in options_vbox.get_children(): - if get_viewport().gui_get_focus_owner() == null: - if child is Button and child.visible and not child.disabled: - child.grab_focus() - Globals.log_message("Grabbed initial focus on: " + child.name, Globals.LogLevel.DEBUG) - return - else: - Globals.log_message("Focus already set—skipping initial grab.", Globals.LogLevel.DEBUG) + if child is Button and child.visible and not child.disabled: + child.grab_focus() + Globals.log_message("Grabbed initial focus on: " + child.name, Globals.LogLevel.DEBUG) return Globals.log_message( "No Button found in OptionsVBoxContainer for initial focus!", Globals.LogLevel.WARNING )
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
scripts/advanced_settings.gdscripts/main_menu.gdscripts/options_menu.gdscripts/pause_menu.gd
🧰 Additional context used
🪛 GitHub Actions: Pull Request Pipeline
scripts/pause_menu.gd
[error] 1-1: gdformat formatting check failed. Code would be reformatted by 'gdformat --diff --check'. Run 'gdformat --write' to fix.
scripts/options_menu.gd
[error] 1-1: gdformat formatting check failed. Code would be reformatted by 'gdformat --diff --check'. Run 'gdformat --write' to fix.
scripts/main_menu.gd
[error] 1-1: gdformat formatting check failed. Code would be reformatted by 'gdformat --diff --check'. Run 'gdformat --write' to fix.
🔇 Additional comments (3)
scripts/main_menu.gd (1)
75-83: LGTM — guarded focus logic is consistent with the project-wide pattern.The deferred
grab_focuswith null-check ongui_get_focus_owner()and theis_instance_validguard are well-structured. The warning log whenstart_buttonis missing is helpful for debugging.scripts/options_menu.gd (1)
15-15: LGTM —class_name OptionsMenuenables type-safeischecks in other scripts.This is used by
advanced_settings.gd(prev_menu is OptionsMenu) to safely accessadvanced_settings_button, which is a clean approach.scripts/advanced_settings.gd (1)
194-207: LGTM — well-guarded back-navigation focus logic.The
is OptionsMenutype check, direct member access toadvanced_settings_button, validity guard, and deferredgrab_focuswith logging all look solid. This properly addresses the reviewer's concern about hard-coded node paths.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Refine initial focus logic in multiple menu scripts (advanced_settings.gd, main_menu.gd, options_menu.gd, pause_menu.gd). Replace direct null checks with a guarded focus_owner inspection (is_instance_valid) and per-menu whitelists so we only grab focus when the viewport owner isn't already one of this menu's interactive controls. Add debug log messages when focus is grabbed or skipped and tidy up some log text/formatting.
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The focus-initialization logic (checking
gui_get_focus_owner,already_has_focus, and logging) is duplicated acrossadvanced_settings.gd,options_menu.gd,main_menu.gd, andpause_menu.gd; consider extracting this into a small shared helper function to keep the behavior consistent and easier to maintain. - In
_grab_first_button_focusyou recomputefocus_ownerand potentially early-return inside the loop for every child; it would be clearer and less error-prone to determine whether focus is already on one of the menu buttons once before iterating, then either immediately return or run a straightforward loop to find the first focusable button. - In
options_menu.gdthekey_mapping_buttonis listed twice in the focus-owner checks; clean this up to avoid confusion and make it clearer which controls are considered part of the focus group.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The focus-initialization logic (checking `gui_get_focus_owner`, `already_has_focus`, and logging) is duplicated across `advanced_settings.gd`, `options_menu.gd`, `main_menu.gd`, and `pause_menu.gd`; consider extracting this into a small shared helper function to keep the behavior consistent and easier to maintain.
- In `_grab_first_button_focus` you recompute `focus_owner` and potentially early-return inside the loop for every child; it would be clearer and less error-prone to determine whether focus is already on one of the menu buttons once before iterating, then either immediately return or run a straightforward loop to find the first focusable button.
- In `options_menu.gd` the `key_mapping_button` is listed twice in the focus-owner checks; clean this up to avoid confusion and make it clearer which controls are considered part of the focus group.
## Individual Comments
### Comment 1
<location> `scripts/main_menu.gd:75-84` </location>
<code_context>
# Fallback: Grab focus immediately if tween isn't running (e.g., error or instant)
# Give keyboard focus to the first button after the fade-in completes
- start_button.call_deferred("grab_focus")
+ if is_instance_valid(start_button):
+ var focus_owner := get_viewport().gui_get_focus_owner()
+
+ var already_has_focus := false
+ if is_instance_valid(focus_owner):
+ # Only skip if focus is already on one of our own interactive controls
+ if (
+ focus_owner == start_button
+ or focus_owner == options_button
+ or focus_owner == quit_button
+ ):
+ already_has_focus = true
+
+ if not already_has_focus:
+ start_button.call_deferred("grab_focus")
+ Globals.log_message("Grabbed initial focus on start_button", Globals.LogLevel.DEBUG)
+ else:
+ Globals.log_message("Focus already set—skipping initial grab.", Globals.LogLevel.DEBUG)
+ else:
+ Globals.log_message(
</code_context>
<issue_to_address>
**suggestion:** The focus-handling pattern is duplicated across multiple menus and could be centralized.
Very similar focus-guard logic now appears in `main_menu.gd`, `pause_menu.gd`, `advanced_settings.gd`, and `options_menu.gd`. This duplication will make it harder to keep focus behavior consistent and to adjust it later.
Consider extracting a shared helper (e.g., in `Globals` or a UI utility) that:
- accepts the candidate control, the allowed focus-owner controls, and optional log context,
- checks whether focus is already on one of those controls, and
- otherwise calls `call_deferred("grab_focus")` and logs.
Centralizing this logic would reduce duplication and make future focus-policy changes safer.
Suggested implementation:
```
Globals.log_message("Invalid tween—grabbing focus immediately.", Globals.LogLevel.WARNING)
# Fallback: Grab focus immediately if tween isn't running (e.g., error or instant)
# Give keyboard focus to the first button after the fade-in completes
Globals.ensure_initial_focus(
start_button,
[start_button, options_button, quit_button],
"main_menu"
)
# Connect START button signal
@warning_ignore("return_value_discarded")
start_button.pressed.connect(_on_start_pressed)
```
To fully implement the refactor:
1. Add a shared helper (e.g. in `Globals.gd` or a dedicated UI utility), something like:
```gdscript
static func ensure_initial_focus(candidate: Control, allowed_controls: Array, context: String = "") -> void:
if not is_instance_valid(candidate):
Globals.log_message(
"Button %s not found—skipping focus. (%s)".sprintf([str(candidate), context]),
Globals.LogLevel.WARNING
)
return
var focus_owner := candidate.get_viewport().gui_get_focus_owner()
var already_has_focus := false
if is_instance_valid(focus_owner) and focus_owner in allowed_controls:
already_has_focus = true
if not already_has_focus:
candidate.call_deferred("grab_focus")
Globals.log_message(
"Grabbed initial focus on %s (%s)".sprintf([candidate.name, context]),
Globals.LogLevel.DEBUG
)
else:
Globals.log_message(
"Focus already set—skipping initial grab. (%s)".sprintf([context]),
Globals.LogLevel.DEBUG
)
```
Adjust exact logging text/levels to match your conventions.
2. Update the other scripts you mentioned (`pause_menu.gd`, `advanced_settings.gd`, `options_menu.gd`, etc.) to replace their duplicated focus-guard blocks with `Globals.ensure_initial_focus(...)`, passing the appropriate candidate button and allowed-controls list for each menu.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
In options_menu.gd the key_mapping_button is listed twice in the focus-owner checks; clean this up to avoid confusion and make it clearer which controls are considered part of the focus group.
In _grab_first_button_focus you recompute focus_owner and potentially early-return inside the loop for every child; it would be clearer and less error-prone to determine whether focus is already on one of the menu buttons once before iterating, then either immediately return or run a straightforward loop to find the first focusable button.
…menus and could be centralized.
Very similar focus-guard logic now appears in main_menu.gd, pause_menu.gd, advanced_settings.gd, and options_menu.gd. This duplication will make it harder to keep focus behavior consistent and to adjust it later.
Consider extracting a shared helper (e.g., in Globals or a UI utility) that:
accepts the candidate control, the allowed focus-owner controls, and optional log context,
checks whether focus is already on one of those controls, and
otherwise calls call_deferred("grab_focus") and logs.
Centralizing this logic would reduce duplication and make future focus-policy changes safer.
Owner
Author
All addressed |
Owner
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
ensure_initial_focushelper only considers focus already being on one of the explicitallowed_controlsnodes; if focus is on a child/descendant (e.g., a button inside a container that isn’t listed), it will still steal focus, so consider taking a parent container as input or checkingis_ancestor_of(focus_owner)to avoid overriding valid in-menu focus. - In
_on_advanced_back_button_pressedyou directlygrab_focusonadvanced_settings_buttoninstead of going throughensure_initial_focus, which means this path can behave differently from the other menus’ focus logic; consider reusing the helper for consistency or documenting why this path should always force focus.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `ensure_initial_focus` helper only considers focus already being on one of the explicit `allowed_controls` nodes; if focus is on a child/descendant (e.g., a button inside a container that isn’t listed), it will still steal focus, so consider taking a parent container as input or checking `is_ancestor_of(focus_owner)` to avoid overriding valid in-menu focus.
- In `_on_advanced_back_button_pressed` you directly `grab_focus` on `advanced_settings_button` instead of going through `ensure_initial_focus`, which means this path can behave differently from the other menus’ focus logic; consider reusing the helper for consistency or documenting why this path should always force focus.
## Individual Comments
### Comment 1
<location> `scripts/pause_menu.gd:69-70` </location>
<code_context>
"_on_advanced_reset_js", "advancedResetPressed"
)
+ # Give keyboard focus to the log level slider (only if nothing in this menu already has focus)
+ Globals.ensure_initial_focus(
+ log_lvl_option,
+ [log_lvl_option, advanced_back_button, advanced_reset_button],
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid calling the focus helper when the pause menu is being hidden.
This block used to run only when `visible` was true, but now `ensure_initial_focus` is called on every `toggle_pause()`, including when hiding the menu. That can wrongly move focus to `resume_button` while unpausing, interfering with gameplay input. Please guard this call with `if visible:` (after toggling) or pass `visible` into the helper so it no-ops when the menu is hidden.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Replace the MockGlobals autoload node with method stubs to prevent side effects in pause_menu tests. Removed MockGlobals class and original_globals handling; added stub calls for Globals.log_message, Globals.load_scene_with_loading, and Globals.load_options in before_each. Updated comments and simplified after_each cleanup to no longer manage the mock Globals node.
…is being hidden. This block used to run only when visible was true, but now ensure_initial_focus is called on every toggle_pause(), including when hiding the menu. That can wrongly move focus to resume_button while unpausing, interfering with gameplay input. Please guard this call with if visible: (after toggling) or pass visible into the helper so it no-ops when the menu is hidden.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@scripts/pause_menu.gd`:
- Around line 66-71: The toggle_pause flow currently flips visible and always
calls Globals.ensure_initial_focus which runs even when hiding the menu; change
toggle_pause so that after flipping visible and setting get_tree().paused you
only call Globals.ensure_initial_focus(resume_button, [back_to_main_button,
options_button, resume_button], "Pause Menu") when visible is true (i.e., the
menu was just shown), leaving focus untouched when the menu is being
hidden/unpaused.
🧹 Nitpick comments (3)
scripts/globals.gd (1)
36-72: Well-structured centralized focus helper.The implementation cleanly validates the candidate, checks existing focus ownership, and defers the grab. Good use of contextual logging.
One minor simplification: on Line 54, since
get_tree().root(aWindow) already extendsViewport, calling.get_viewport()on it is redundant. You could useget_viewport().gui_get_focus_owner()directly.Simplify viewport access
- var focus_owner: Control = get_tree().root.get_viewport().gui_get_focus_owner() + var focus_owner: Control = get_viewport().gui_get_focus_owner()scripts/options_menu.gd (1)
129-155: Clean delegation to centralized focus helper.The logic correctly finds the first visible/enabled
Buttonand delegates toensure_initial_focus. One small observation: theallowed_controlslist on Lines 143-148 is hardcoded separately from the dynamic loop on Line 135. If a button is added to the container but not to this list,ensure_initial_focuswould override existing focus on it. Consider buildingallowed_controlsdynamically from the same loop to stay in sync, though this is low risk given the current structure.test/gut/test_pause_menu.gd (1)
62-77: String-based dynamic variable access is fragile.
ensure_actionusesset(added_var_name, ...)andset(events_var_name, ...)with string-based variable names. A typo in the caller (e.g.,"added_pausee") would silently create a new property viaset()rather than failing, making bugs hard to track.This is acceptable for a small test helper, but consider using a
Dictionarykeyed by action name instead if the pattern grows.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
scripts/advanced_settings.gdscripts/globals.gdscripts/main_menu.gdscripts/options_menu.gdscripts/pause_menu.gdtest/gut/test_pause_menu.gd
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/main_menu.gd
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Browser Functional Tests / test
🔇 Additional comments (3)
scripts/advanced_settings.gd (1)
190-203: Clean focus restoration with proper type guard.Using
prev_menu is OptionsMenubefore accessingadvanced_settings_buttonis safe and avoids noisy warnings when navigating back to non-Options menus. Good improvement over hard-coded node paths.scripts/options_menu.gd (1)
15-15: Good: Exposingclass_name OptionsMenuenables safe type checks.This allows
advanced_settings.gdto useprev_menu is OptionsMenuinstead of brittle string/path-based checks.test/gut/test_pause_menu.gd (1)
38-40: GUT'sstub()successfully intercepts calls on autoload singletons — verified by working tests. The stubs onGlobals.log_message,Globals.load_scene_with_loading, andGlobals.load_optionsare effective in preventing side effects.Note:
Globals.ensure_initial_focusdoes run live duringtoggle_pause()calls (PM-03/04/05), but its logging output is already suppressed by thelog_message()stub, and its other operations (grab_focus,gui_get_focus_owner) are safe for tests. This is intentional integration-style testing, not an oversight.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
In _on_advanced_back_button_pressed you directly grab_focus on advanced_settings_button instead of going through ensure_initial_focus, which means this path can behave differently from the other menus’ focus logic; consider reusing the helper for consistency or documenting why this path should always force focus.
Perform a comprehensive review of Milestone 11 upon its completion, and update the project's README.md file to reflect the delivered content. This ensures the documentation accurately captures the progress in this Godot 4.5 learning project on Windows 10 64-bit, highlighting new features, refactors, and improvements for better readability and onboarding.
Owner
Author
|
@sourcery-ai summary |
ikostan
deleted the
enable-keyboard-&-dpad-navigation-for-advanced-settings
branch
February 15, 2026 07:55
github-project-automation
Bot
moved this from In Progress
to Done
in Sky Lock Assault Project
ikostan
restored the
enable-keyboard-&-dpad-navigation-for-advanced-settings
branch
February 15, 2026 08:01
This was referenced Feb 15, 2026
This was
linked to
issues
Feb 24, 2026
8 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add full keyboard navigation support to the Advanced Settings menu in SkyLockAssault, allowing users to navigate, modify, and apply advanced configuration options using only the keyboard. This includes all advanced toggles, selectors, sliders, and sub-sections within the Advanced Settings menu.
Enable full D-Pad (controller) navigation support within the Advanced Settings menu to ensure consistent gamepad-only usability across SkyLockAssault.
Perform a comprehensive review of Milestone 11 upon its completion, and update the project's README.md file to reflect the delivered content.
name: Default Pull Request Template
about: Suggesting changes to SkyLockAssault
title: ''
labels: ''
assignees: ''
Description
What does this PR do? (e.g., "Fixes player jump physics in level 2" or "Adds
new enemy AI script")
Related Issue
Closes #ISSUE_NUMBER (if applicable)
Changes
system")
Testing
works on Win10 with 60 FPS")
Checklist
Additional Notes
Anything else? (e.g., "Tested on Win10 64-bit; needs Linux validation")
Summary by Sourcery
Improve focus handling when navigating to and from the Advanced Settings menu to better support keyboard and D-pad navigation.
Enhancements:
Summary by Sourcery
Standardize and centralize UI focus handling across menus to improve keyboard and controller navigation, particularly around the Advanced Settings flow.
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
User Interface
Improvements