Migrated UID Viewer from bottom‑panel dock to editor tool#1190
Migrated UID Viewer from bottom‑panel dock to editor tool#1190TheAenema wants to merge 1 commit into
Conversation
WalkthroughRefactors the UID Viewer from a dock (UIDViewerDock/VBoxContainer) into a Window-based tool (UIDViewer), adds EditorNode integration (instantiation, GUI child), exposes a Tools menu entry/shortcut that calls Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EditorNode
participant GUI
participant UIDViewer
participant FileSystem
User->>EditorNode: Select "Tools → UID Viewer" (menu/shortcut)
EditorNode->>UIDViewer: uid_viewer->_open_tool()
UIDViewer->>GUI: popup_centered() / show window
activate UIDViewer
UIDViewer->>FileSystem: request UID list / refresh
FileSystem-->>UIDViewer: return UID data
UIDViewer-->>GUI: populate tree, focus search
User->>UIDViewer: search / activate / context actions
UIDViewer->>FileSystem: optional lookup/action
Note right of UIDViewer: On close -> _on_tool_closed()
deactivate UIDViewer
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
editor/file_system/uid_viewer.h (1)
2-2:⚠️ Potential issue | 🟡 MinorCopyright header filename mismatch.
The comment still references the old filename
uid_viewer_dock.h, but this file has been renamed touid_viewer.h. This is confirmed by the pipeline failure indicating copyright header changes are needed.Proposed fix
-/* uid_viewer_dock.h */ +/* uid_viewer.h */editor/file_system/uid_viewer.cpp (2)
1-3:⚠️ Potential issue | 🟡 MinorUpdate the file banner to match the new filename.
The banner still says
uid_viewer_dock.cpp, which no longer matches the path and can trigger the copyright-header hook. Rename it touid_viewer.cpp.🧾 Proposed fix
-/* uid_viewer_dock.cpp */ +/* uid_viewer.cpp */
101-142:⚠️ Potential issue | 🟠 MajorReset
last_selected_itemand hide context menu when the tree is rebuilt to prevent use-after-free.
uid_tree->clear()frees all TreeItems in Godot 4, invalidating cached pointers. If the context menu is open when_refresh_uid_list()is called,last_selected_itemremains pointing to freed memory. A subsequent call to_on_context_menu_id_pressed()will dereference this invalid pointer (the null check on line 182 won't catch freed memory—only nullptr). Reset the pointer and close the menu to prevent use-after-free.🛠️ Proposed fix
void UIDViewer::_refresh_uid_list() { uid_tree->clear(); + last_selected_item = nullptr; + if (context_menu) { + context_menu->hide(); + } TreeItem *root = uid_tree->create_item();
🧹 Nitpick comments (1)
editor/file_system/uid_viewer.h (1)
35-36: Consider removing potentially redundant include.
scene/gui/control.hmay be unnecessary sinceWindowinherits fromControl, and the header would be transitively included. However, this is a minor optimization and can be kept if explicit includes are preferred for clarity.Regarding the static analysis error for
scene/main/window.h: this is the standard Godot engine path for the Window class and should resolve correctly in the actual build environment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@editor/file_system/uid_viewer.cpp`:
- Around line 267-274: The current UIDViewer::_open_tool() only checks is_open
and skips reopening if the tool is hidden but still marked open; update
_open_tool() to also test the actual visibility (e.g., using
is_visible_in_tree() or is_visible()) and if is_open is true but the control is
not visible, call popup_centered() and grab_focus() to reshow it; also ensure
the "close_requested" connection (callable_mp(this,
&UIDViewer::_on_tool_closed)) is only created once (or guarded) so duplicate
connects aren't made.
| void UIDViewer::_open_tool() { | ||
| if (!is_open) { | ||
| connect("close_requested", callable_mp(this, &UIDViewer::_on_tool_closed)); | ||
| popup_centered(); | ||
| is_open = true; | ||
| } else { | ||
| grab_focus(); | ||
| } |
There was a problem hiding this comment.
Guard against hidden-but-open state.
If the window is hidden via another path (layout restore, editor teardown), is_open stays true and _open_tool() won’t reshow it. Consider re-opening when not visible.
Suggested fix
void UIDViewer::_open_tool() {
- if (!is_open) {
- connect("close_requested", callable_mp(this, &UIDViewer::_on_tool_closed));
- popup_centered();
- is_open = true;
- } else {
- grab_focus();
- }
+ if (!is_open || !is_visible()) {
+ if (!is_open) {
+ connect("close_requested", callable_mp(this, &UIDViewer::_on_tool_closed));
+ }
+ popup_centered();
+ is_open = true;
+ } else {
+ grab_focus();
+ }
}🤖 Prompt for AI Agents
In `@editor/file_system/uid_viewer.cpp` around lines 267 - 274, The current
UIDViewer::_open_tool() only checks is_open and skips reopening if the tool is
hidden but still marked open; update _open_tool() to also test the actual
visibility (e.g., using is_visible_in_tree() or is_visible()) and if is_open is
true but the control is not visible, call popup_centered() and grab_focus() to
reshow it; also ensure the "close_requested" connection (callable_mp(this,
&UIDViewer::_on_tool_closed)) is only created once (or guarded) so duplicate
connects aren't made.
2 participants
Repulled in #1249