Add custom folder coloring#1304
Conversation
WalkthroughFileSystemDock now supports preset and custom folder colors, persists direct ChangesFolder Color Support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant FileSystemDock
participant ColorPicker
participant ProjectSettings
User->>FileSystemDock: Choose Custom folder color
FileSystemDock->>ColorPicker: Open picker for selected folders
User->>ColorPicker: Select and confirm color
ColorPicker->>FileSystemDock: Return Color value
FileSystemDock->>ProjectSettings: Persist folder color assignments
FileSystemDock->>FileSystemDock: Refresh folder color rendering
🚥 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
editor/docks/filesystem_dock.cpp (2)
3298-3313: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck for
NILinstead ofp_colortruthiness
Variant::booleanize()treatsColor(0, 0, 0, 1)as false, so a user-picked pure black folder color gets erased instead of stored. Use a type check here rather thanif (p_color).🤖 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 3298 - 3313, The folder-color assignment in FileSystemDock::_set_folder_color must distinguish an absent color from a valid black Color value. Replace the truthiness check on p_color with a Variant type check for a Color, storing valid colors and erasing the folder entry only when p_color is NIL.
1074-1118: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHandle direct
Colorfolder assignments here.assigned_folder_colorscan hold preset names orColorvalues, but this lookup still treats every value as a preset key. Direct colors will miss the preset map, render inherited colors incorrectly, and can insert a stray entry intofolder_colors.editor/docks/filesystem_dock.cpp:1084🤖 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 1074 - 1118, The folder color inheritance lookup in the directory scan must support both preset-name and direct Color values in assigned_folder_colors. Update the inheritance logic near the color_scan_dir loop to inspect the stored Variant, resolving string values through folder_colors while using Color values directly, without indexing folder_colors for non-string assignments or creating stray entries.
🤖 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.
Outside diff comments:
In `@editor/docks/filesystem_dock.cpp`:
- Around line 3298-3313: The folder-color assignment in
FileSystemDock::_set_folder_color must distinguish an absent color from a valid
black Color value. Replace the truthiness check on p_color with a Variant type
check for a Color, storing valid colors and erasing the folder entry only when
p_color is NIL.
- Around line 1074-1118: The folder color inheritance lookup in the directory
scan must support both preset-name and direct Color values in
assigned_folder_colors. Update the inheritance logic near the color_scan_dir
loop to inspect the stored Variant, resolving string values through
folder_colors while using Color values directly, without indexing folder_colors
for non-string assignments or creating stray entries.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 63fbc569-a3ba-429e-b954-fdd3cd09b763
⛔ Files ignored due to path filters (1)
editor/icons/FolderQuestion.svgis excluded by!**/*.svg
📒 Files selected for processing (2)
editor/docks/filesystem_dock.cppeditor/docks/filesystem_dock.h
|
@Shakai-Dev There shouldn't be any reason why those two PRs would change it. The only thing that would probably cause issues is #1306 which I would need to rebase this PR |
Ok, then that's good |
Shakai-Dev
left a comment
Shakai-Dev
left a comment
There was a problem hiding this comment.
PR_test.mp4
It only works partially, you can only apply a color to the folder via custom color. You can't use any of the presets. I'm using the build from GH actions (linux-editor-mono)
Co-authored-by: memmoxt <109536933+memmoxt@users.noreply.github.com>
|
Ok @Shakai-Dev It should be fixed, please give it another test |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Around line 1084-1085: Update the folder-color inheritance logic around
assigned_folder_colors and inherited_folder_color to track whether an ancestor
assignment was found separately from whether its color equals
default_folder_color. Stop scanning once an explicit assignment is discovered,
including custom colors matching the default, so ancestor colors cannot override
it.
🪄 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
Run ID: 54e84d8b-81ef-4c68-8e8b-a6555daf4b83
⛔ Files ignored due to path filters (1)
editor/icons/FolderQuestion.svgis excluded by!**/*.svg
📒 Files selected for processing (2)
editor/docks/filesystem_dock.cppeditor/docks/filesystem_dock.h
| Variant assigned_color = assigned_folder_colors[color_scan_dir]; | ||
| inherited_folder_color = assigned_color.is_string() ? folder_colors[assigned_folder_colors[color_scan_dir]] : Color(assigned_color); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Stop inheritance when an explicit default-matching color is found.
A custom color equal to default_folder_color leaves the loop condition true, allowing an ancestor color to override it. Track assignment discovery separately from color equality.
Proposed fix
Color inherited_folder_color = default_folder_color;
+bool inherited_color_found = false;
String color_scan_dir = directory;
-while (color_scan_dir != "res://" && inherited_folder_color == default_folder_color) {
+while (color_scan_dir != "res://" && !inherited_color_found) {
...
if (assigned_folder_colors.has(color_scan_dir)) {
Variant assigned_color = assigned_folder_colors[color_scan_dir];
- inherited_folder_color = assigned_color.is_string() ? folder_colors[assigned_folder_colors[color_scan_dir]] : Color(assigned_color);
+ inherited_folder_color = assigned_color.is_string() ? folder_colors[assigned_color] : Color(assigned_color);
+ inherited_color_found = true;
}📝 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.
| Variant assigned_color = assigned_folder_colors[color_scan_dir]; | |
| inherited_folder_color = assigned_color.is_string() ? folder_colors[assigned_folder_colors[color_scan_dir]] : Color(assigned_color); | |
| Variant assigned_color = assigned_folder_colors[color_scan_dir]; | |
| inherited_folder_color = assigned_color.is_string() ? folder_colors[assigned_color] : Color(assigned_color); |
🤖 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 1084 - 1085, Update the
folder-color inheritance logic around assigned_folder_colors and
inherited_folder_color to track whether an ancestor assignment was found
separately from whether its color equals default_folder_color. Stop scanning
once an explicit assignment is discovered, including custom colors matching the
default, so ancestor colors cannot override it.
Works now |
2 participants
This adds in an option to set custom folder colors in the filesystem as suggested by @TheAenema
SVG courtesy of @memmoxt
Summary by CodeRabbit