fix: add margin to file thumbnails in file explorer window - #1338
fix: add margin to file thumbnails in file explorer window#1338joey-wheeler99 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThumbnail-mode file items in the filesystem dock now use a horizontal separation of ChangesFilesystem dock updates
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@editor/docks/filesystem_dock.cpp`:
- Line 970: Update the h_separation theme constant override in the
thumbnail-mode setup to multiply the value 10 by EDSCALE, matching the existing
scaling used for thumbnail_size.
- Line 970: Remove or reset the files control’s h_separation theme override in
the non-thumbnail/list-mode branch, alongside the existing thumbnail setup, so
switching modes after files->clear() restores the intended list spacing. Update
the mode-switching logic around the files control without changing
thumbnail-mode spacing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3306c194-11e1-446b-9af1-582a58e105c0
📒 Files selected for processing (1)
editor/docks/filesystem_dock.cpp
| files->set_fixed_column_width(thumbnail_size * 3 / 2); | ||
| files->set_max_text_lines(2); | ||
| files->set_fixed_icon_size(Size2(thumbnail_size, thumbnail_size)); | ||
| files->add_theme_constant_override("h_separation", 10); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'filesystem_dock\.cpp|editor_profiler\.cpp' . | sed 's#^\./##'
echo "== relevant filesystem_dock section =="
if [ -f editor/docks/filesystem_dock.cpp ]; then
nl -ba editor/docks/filesystem_dock.cpp | sed -n '930,1005p'
fi
echo "== search theme constant overrides and EDSCALE usage =="
rg -n "add_theme_constant_override|\"h_separation\"|EDSCALE|thumbnail_size" editor/docks/filesystem_dock.cpp editor/debugger/editor_profiler.cpp 2>/dev/null || true
echo "== nearby profiler snippet =="
if [ -f editor/debugger/editor_profiler.cpp ]; then
nl -ba editor/debugger/editor_profiler.cpp | rg -n "h_separation|thumbnail_size|EDSCALE" -C 3
fiRepository: Redot-Engine/redot-engine
Length of output: 368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'filesystem_dock\.cpp|editor_profiler\.cp' . | sed 's#^\./##' || true
echo "== relevant filesystem_dock section =="
if [ -f editor/docks/filesystem_dock.cpp ]; then
nl -ba editor/docks/filesystem_dock.cpp | sed -n '930,1005p'
fi
echo "== search theme constant overrides and EDSCALE usage =="
rg -n "add_theme_constant_override|\"h_separation\"|EDSCALE|thumbnail_size" editor/docks/filesystem_dock.cpp editor/debugger/editor_profiler.cpp 2>/dev/null || true
echo "== nearby profiler snippet =="
if [ -f editor/debugger/editor_profiler.cpp ]; then
nl -ba editor/debugger/editor_profiler.cpp | rg -n "h_separation|thumbnail_size|EDSCALE" -C 3 || true
fiRepository: Redot-Engine/redot-engine
Length of output: 368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant filesystem_dock section =="
awk 'NR>=930 && NR<=1005 {printf "%6d\t%s\n", NR, $0}' editor/docks/filesystem_dock.cpp
echo "== search theme constant overrides and EDSCALE usage =="
grep -nE 'add_theme_constant_override|"[^"]*h_separation[^"]*"|EDSCALE|thumbnail_size' editor/docks/filesystem_dock.cpp editor/debugger/editor_profiler.cpp 2>/dev/null || true
echo "== profiler relevant lines =="
grep -nE 'h_separation|thumbnail_size|EDSCALE' editor/debugger/editor_profiler.cpp 2>/dev/null -C 2 || trueRepository: Redot-Engine/redot-engine
Length of output: 8184
Scale h_separation with EDSCALE.
thumbnail_size is computed with EDSCALE, but the thumbnail-mode h_separation override remains 10. Multiply this override by EDSCALE so the separation scales with the thumbnails.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@editor/docks/filesystem_dock.cpp` at line 970, Update the h_separation theme
constant override in the thumbnail-mode setup to multiply the value 10 by
EDSCALE, matching the existing scaling used for thumbnail_size.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)filesystem_dock\.cpp$|(^|/)theme_override|local_theme_override' || true
echo
echo "filesystem_dock.cpp around relevant lines:"
sed -n '900,990p' editor/docks/filesystem_dock.cpp
echo
echo "Search h_separation/theme overrides in filesystem_dock.cpp:"
rg -n 'h_separation|theme_constant_override|clear\(\)' editor/docks/filesystem_dock.cpp
echo
echo "Nearby update_file_list outlines/functions:"
ast-grep outline editor/docks/filesystem_dock.cpp --match _update_file_list --view expanded || true
sed -n '840,930p' editor/docks/filesystem_dock.cppRepository: Redot-Engine/redot-engine
Length of output: 7281
🌐 Web query:
Godot Control remove_theme_constant_override docs Control local theme overrides clear remove_local_theme_override
💡 Result:
In Godot, local theme overrides for Control nodes are managed using specific methods provided by the Control class [1][2]. These overrides allow a node to ignore the values provided by its assigned Theme and use a custom, local value instead [3]. To remove a specific theme constant override that was previously set (either via code or the Inspector), you should use the remove_theme_constant_override method [2][4]. Method Signature: void remove_theme_constant_override(name: StringName) Functionality: The remove_theme_constant_override method removes the local override for a theme constant with the specified name [4]. Once removed, the Control will revert to using the value provided by its active Theme (or the default theme if no custom theme is assigned) [1][3]. Other similar methods exist for different theme data types, following the same naming convention [5][6]: - remove_theme_color_override(name: StringName) [2][5] - remove_theme_font_override(name: StringName) [5][6] - remove_theme_font_size_override(name: StringName) [5][6] - remove_theme_icon_override(name: StringName) [5][6] - remove_theme_style_override(name: StringName) [5][6] Note that these methods are distinct from the Theme class's own methods (such as clear_constant), which are used for modifying Theme resources themselves, rather than individual Control node overrides [7]. If you are working with an older version of Godot (prior to these methods being added), you would have had to manually reset values to their defaults [8], but in current Godot 4.x versions, these dedicated remove methods are the standard and recommended approach [2][4].
Citations:
- 1: https://docs.godotengine.org/en/stable/classes/class_control.html
- 2: https://docs.godotengine.org/en/4.4/classes/class_control.html
- 3: https://docs.godotengine.org/en/stable/tutorials/ui/gui%5Fskinning.html
- 4: https://docs.w3cub.com/godot~4.2/classes/class_window
- 5: https://github.com/godotengine/godot/blob/8dcf5b4e/scene/gui/control.h
- 6: https://github.com/godotengine/godot/blob/4a919adc/scene/main/window.h
- 7: https://docs.godotengine.org/en/stable/classes/class_theme.html
- 8: Not clear what "Theme Properties" are and how to retrieve and set them godotengine/godot-docs#2889
Do not leave the thumbnail override in list mode.
files->clear() does not remove theme constant overrides, so switching from thumbnails back to the list view reuses the same files control while keeping h_separation set. Remove the override in the non-thumbnail branch or restore the intended list-mode spacing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@editor/docks/filesystem_dock.cpp` at line 970, Remove or reset the files
control’s h_separation theme override in the non-thumbnail/list-mode branch,
alongside the existing thumbnail setup, so switching modes after files->clear()
restores the intended list spacing. Update the mode-switching logic around the
files control without changing thumbnail-mode spacing.
Shakai-Dev
left a comment
Shakai-Dev
left a comment
There was a problem hiding this comment.
Code LGTM but needs testing
|
Is this a problem anymore? On 26.2 I couldn't see this behavior, so I think it might have already been fixed? versus 26.2 |
I can't reproduce the issue either. I added small margin to prevent the edge case of the original issue. edit: do you think I should close this PR for now and wait to see if it pops up again? |
|
I think the changes are fine. It is not entirely clear if Godot addressed this directly or not, or if this was an edge case on certain systems. I don't mind adding some explicit margin. I also do agree with CodeRabbit's review comments here. |
Should I multiply the margin with |
Yes, that should have the correct effect. |
joey-wheeler99
left a comment
joey-wheeler99
left a comment
There was a problem hiding this comment.
Multiply margin with EDSCALE
JoltedJon
left a comment
JoltedJon
left a comment
There was a problem hiding this comment.
Tested and looks good to me
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@editor/docks/filesystem_dock.cpp`:
- Line 3524: Update the add_icon_item call in the filesystem dock to use the
canonical “GDExtension” capitalization in both the theme icon key passed to
get_editor_theme_icon and the visible label passed to TTRC, preserving the
existing reload action.
- Around line 2563-2566: Replace the no-op FILE_MENU_RELOAD_GDEXTENSION branch
with a call to GDExtensionManager::reload_extension(p_selected[0]) and handle
its returned LoadStatus, including reporting failure to the user; alternatively
remove this menu item until reload is implemented.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3edee5c3-d862-4004-aea0-2d3d3844c881
📒 Files selected for processing (2)
editor/docks/filesystem_dock.cppeditor/docks/filesystem_dock.h
| case FILE_MENU_RELOAD_GDEXTENSION: { | ||
| // TODO: implement reload gdextension here | ||
|
|
||
| } break; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not expose a no-op reload command.
This case only executes break. Selecting the menu item does not reload the extension or report an error. Call GDExtensionManager::reload_extension(p_selected[0]), handle the returned LoadStatus, or remove the menu item until the implementation is complete.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@editor/docks/filesystem_dock.cpp` around lines 2563 - 2566, Replace the no-op
FILE_MENU_RELOAD_GDEXTENSION branch with a call to
GDExtensionManager::reload_extension(p_selected[0]) and handle its returned
LoadStatus, including reporting failure to the user; alternatively remove this
menu item until reload is implemented.
There was a problem hiding this comment.
Sorry, I worked on the wrong branch
There was a problem hiding this comment.
@joey-wheeler99, understood. The FILE_MENU_RELOAD_GDEXTENSION change is outside this PR’s thumbnail-spacing objective. Please remove the unrelated change when you update the branch. The finding remains valid while the no-op menu action is present.
You are interacting with an AI system.
| } | ||
|
|
||
| if (gdextension_valid) { | ||
| p_popup->add_icon_item(get_editor_theme_icon(SNAME("Reload GDEextension")), TTRC("Reload GDEextension"), FILE_MENU_RELOAD_GDEXTENSION); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the canonical GDExtension spelling.
The new label uses GDEextension. Change both strings to Reload GDExtension so the visible text and theme icon key use the project’s standard name.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@editor/docks/filesystem_dock.cpp` at line 3524, Update the add_icon_item call
in the filesystem dock to use the canonical “GDExtension” capitalization in both
the theme icon key passed to get_editor_theme_icon and the visible label passed
to TTRC, preserving the existing reload action.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
4 participants
This should fix #670
Summary by CodeRabbit