Remove dockable container#69
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ Finishing touches🧪 Generate unit tests (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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@common/layouts/editor/list_container.gd`:
- Around line 50-52: The handler _on_child_exiting_tree currently calls update()
immediately while the exiting PanelContainer is still in the scene tree, causing
the dropdown to include the soon-to-be-removed node; change the direct call to a
deferred call (e.g. call_deferred("update")) so the update runs after the node
has left the tree and the dropdown reflects the actual children.
- Around line 11-28: The cached index selected_cache may be out-of-bounds after
rebuilding _dropdown; after populating _dropdown (use
_dropdown.get_item_count()), clamp selected_cache to the valid range (0 ..
max(0, count-1)) and only call _on_item_selected(selected_cache) when count > 0;
update the logic in update() around _dropdown creation/population and the final
_on_item_selected call to enforce this clamp.
In `@scenes/main/monologue_editor.gd`:
- Around line 103-105: Remove the unused locals to avoid wasted allocations:
delete the unused declarations of _converter (NodeConverter.new()) and
_storyline (StorylineManager.get_active_storyline()) from the scope where
load_project() is defined (or the top of monologue_editor.gd) since they are not
referenced by any functions like load_project(); if those objects are actually
needed elsewhere, instead move their creation to the exact function that uses
them and reference the symbols _converter and _storyline only where required.
| func update() -> void: | ||
| var selected_cache: int = 0 | ||
| if _dropdown != null: | ||
| selected_cache = _dropdown.selected | ||
| _dropdown.queue_free() | ||
|
|
||
| _dropdown = OptionButton.new() | ||
| _dropdown.item_selected.connect(_on_item_selected) | ||
| add_child(_dropdown) | ||
| move_child(_dropdown, 0) | ||
|
|
||
| for child: Control in get_children(): | ||
| if not child is PanelContainer or child == _dropdown: | ||
| continue | ||
|
|
||
| _dropdown.add_item(child.name) | ||
| child.size_flags_vertical = Control.SIZE_EXPAND_FILL | ||
| _on_item_selected(selected_cache) |
There was a problem hiding this comment.
Potential out-of-bounds index after panel removal.
When a panel is removed, selected_cache may exceed the new item count, causing _on_item_selected(selected_cache) to reference a non-existent dropdown item. This can result in incorrect selection or runtime errors from get_item_text().
🛡️ Proposed fix to clamp the cached index
func update() -> void:
var selected_cache: int = 0
if _dropdown != null:
selected_cache = _dropdown.selected
_dropdown.queue_free()
_dropdown = OptionButton.new()
_dropdown.item_selected.connect(_on_item_selected)
add_child(_dropdown)
move_child(_dropdown, 0)
for child: Control in get_children():
if not child is PanelContainer or child == _dropdown:
continue
_dropdown.add_item(child.name)
child.size_flags_vertical = Control.SIZE_EXPAND_FILL
+
+ var item_count := _dropdown.item_count
+ if item_count > 0:
+ selected_cache = clampi(selected_cache, 0, item_count - 1)
_on_item_selected(selected_cache)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func update() -> void: | |
| var selected_cache: int = 0 | |
| if _dropdown != null: | |
| selected_cache = _dropdown.selected | |
| _dropdown.queue_free() | |
| _dropdown = OptionButton.new() | |
| _dropdown.item_selected.connect(_on_item_selected) | |
| add_child(_dropdown) | |
| move_child(_dropdown, 0) | |
| for child: Control in get_children(): | |
| if not child is PanelContainer or child == _dropdown: | |
| continue | |
| _dropdown.add_item(child.name) | |
| child.size_flags_vertical = Control.SIZE_EXPAND_FILL | |
| _on_item_selected(selected_cache) | |
| func update() -> void: | |
| var selected_cache: int = 0 | |
| if _dropdown != null: | |
| selected_cache = _dropdown.selected | |
| _dropdown.queue_free() | |
| _dropdown = OptionButton.new() | |
| _dropdown.item_selected.connect(_on_item_selected) | |
| add_child(_dropdown) | |
| move_child(_dropdown, 0) | |
| for child: Control in get_children(): | |
| if not child is PanelContainer or child == _dropdown: | |
| continue | |
| _dropdown.add_item(child.name) | |
| child.size_flags_vertical = Control.SIZE_EXPAND_FILL | |
| var item_count := _dropdown.item_count | |
| if item_count > 0: | |
| selected_cache = clampi(selected_cache, 0, item_count - 1) | |
| _on_item_selected(selected_cache) |
🤖 Prompt for AI Agents
In `@common/layouts/editor/list_container.gd` around lines 11 - 28, The cached
index selected_cache may be out-of-bounds after rebuilding _dropdown; after
populating _dropdown (use _dropdown.get_item_count()), clamp selected_cache to
the valid range (0 .. max(0, count-1)) and only call
_on_item_selected(selected_cache) when count > 0; update the logic in update()
around _dropdown creation/population and the final _on_item_selected call to
enforce this clamp.
| func _on_child_exiting_tree(node: Node) -> void: | ||
| if not node is PanelContainer: return | ||
| update() |
There was a problem hiding this comment.
update() called while exiting node is still in tree.
When child_exiting_tree fires, the node hasn't been removed yet. Calling update() will include the exiting node in the dropdown, then it gets removed, leaving the dropdown stale. Consider deferring the update.
🔧 Proposed fix using call_deferred
func _on_child_exiting_tree(node: Node) -> void:
if not node is PanelContainer: return
- update()
+ update.call_deferred()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func _on_child_exiting_tree(node: Node) -> void: | |
| if not node is PanelContainer: return | |
| update() | |
| func _on_child_exiting_tree(node: Node) -> void: | |
| if not node is PanelContainer: return | |
| update.call_deferred() |
🤖 Prompt for AI Agents
In `@common/layouts/editor/list_container.gd` around lines 50 - 52, The handler
_on_child_exiting_tree currently calls update() immediately while the exiting
PanelContainer is still in the scene tree, causing the dropdown to include the
soon-to-be-removed node; change the direct call to a deferred call (e.g.
call_deferred("update")) so the update runs after the node has left the tree and
the dropdown reflects the actual children.
| var _converter := NodeConverter.new() | ||
| var _storyline = StorylineManager.get_active_storyline() | ||
|
|
There was a problem hiding this comment.
Remove unused variables.
_converter and _storyline are instantiated but never used in load_project(). This appears to be leftover code from the refactor. The NodeConverter.new() allocation is wasteful if unused.
🧹 Proposed fix
- var _converter := NodeConverter.new()
- var _storyline = StorylineManager.get_active_storyline()
-
load_editor_sections()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var _converter := NodeConverter.new() | |
| var _storyline = StorylineManager.get_active_storyline() | |
| load_editor_sections() |
🤖 Prompt for AI Agents
In `@scenes/main/monologue_editor.gd` around lines 103 - 105, Remove the unused
locals to avoid wasted allocations: delete the unused declarations of _converter
(NodeConverter.new()) and _storyline (StorylineManager.get_active_storyline())
from the scope where load_project() is defined (or the top of
monologue_editor.gd) since they are not referenced by any functions like
load_project(); if those objects are actually needed elsewhere, instead move
their creation to the exact function that uses them and reference the symbols
_converter and _storyline only where required.
1 participant
Summary by CodeRabbit
Removed Features
Improved Features
✏️ Tip: You can customize this high-level summary in your review settings.