Fix folder color not showing up in file dialogs

[synalice]

Fixes #83366.

Here is an example project with a bunch of colored folders to verify that my changes work: preview_project.zip.

I can also provide a compiled Windows 10 executable of the editor if someone needs it.

Screenshots:

[synalice]

Btw, is force-pushing an only option to add new changes to PR? You either squash or amend, but you'll always have to force-push after that, is that right?

[AThousandShips]

Indeed, or it will force you to merge with the upstream commits, unless you just add new commits at the end

[synalice]

Should I mark my PRs as drafts when there are situations like this when I need to add some changes? Or should I just re-request review when I'm ready? What's the workflow here?

[AThousandShips]

Unless you're expecting to have to do a major rework there's no need, unless it's been approved, though if you just communicate you need to fix some things there should be no issue

[KoBeWi]

The colors still do not apply to sub-folders and recent folders:

[synalice]

The colors still do not apply to sub-folders and recent folders:

Oh, yeah, I missed that with subfolders. I'll fix that.

And is there a way to make a folder appear in the recent tab? It's empty for me.

[KoBeWi]

When you open some file (e.g. a scene), its directory will appear in recent.
btw the subfolder color bug also applies to favorites/recent.

[synalice]

Should be it now o(*￣︶￣*)o

[synalice]

Is there any tutorial on how to set up this code style GitHub action locally?

[AThousandShips]

Yes see here

[KoBeWi]

Works correctly now. I was a bit worried about the performance impact of recursively checking folder colors, but I tested the PR in a big project and didn't see any issues (although it's not like I have thousands of deeply nested folders).

[AThousandShips]

I was a bit worried about the performance impact of recursively checking folder colors

If it is a concern maybe there could be some caching or other lookups, unsure how though, but I think the nesting should be shallow enough, though not sure how often it is queried

[synalice]

I've compiled the source and verified that everything still works. I think this can be safely merged now!

\(￣︶￣*\))

[YuriSizov]

If it is a concern maybe there could be some caching

I think the entire implementation is a bit iffy. This information seems like a perfect fit for the EditorFileSystem data structure rather than being limited to the FileSystemDock. EFS would also act as a cache and should make iterations easier.

[YuriSizov]

Thanks! I've decided we can merge this as is to address the existing problem. But I still think we can look into refactoring the implementation and moving color information to the editor file system. So if you're interested, feel free to look into it 🙂

[nathanfranke]

This caused a crash for me on Linux, 6.6.3-arch1-1

Reproduction is simple: Project Manager -> New Project -> Browse project path

Good daddb2b
Bad 6faa5c6

Godot Engine v4.3.dev.custom_build.6faa5c6dc - https://godotengine.org
OpenGL API 4.6 (Core Profile) Mesa 23.2.1-arch1.2 - Compatibility - Using Device: AMD - AMD Radeon RX 6800 XT (navi21, LLVM 16.0.6, DRM 3.54, 6.6.3-arch1-1)
 

================================================================
handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.3.dev.custom_build (6faa5c6dc479bebd4b92a5251fafe22b21ca9523)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] /usr/lib/libc.so.6(+0x3e710) [0x7ff742c32710] (??:0)
[2] Dictionary::_ref(Dictionary const&) const (/home/nathan/workspace/public/godot-4/core/variant/dictionary.cpp:222)
[3] Dictionary::Dictionary(Dictionary const&) (/home/nathan/workspace/public/godot-4/core/variant/dictionary.cpp:388)
[4] FileSystemDock::get_assigned_folder_colors() const (/home/nathan/workspace/public/godot-4/editor/filesystem_dock.cpp:3473)
[5] EditorFileDialog::get_dir_icon_color(String const&) (/home/nathan/workspace/public/godot-4/editor/gui/editor_file_dialog.cpp:775)
[6] EditorFileDialog::update_file_list() (/home/nathan/workspace/public/godot-4/editor/gui/editor_file_dialog.cpp:885 (discriminator 3))
[7] EditorFileDialog::_invalidate() (/home/nathan/workspace/public/godot-4/editor/gui/editor_file_dialog.cpp:1190)
[8] void call_with_variant_args_helper<EditorFileDialog>(EditorFileDialog*, void (EditorFileDialog::*)(), Variant const**, Callable::CallError&, IndexSequence<>) (/home/nathan/workspace/public/godot-4/./core/variant/binder_common.h:308)
[9] void call_with_variant_args<EditorFileDialog>(EditorFileDialog*, void (EditorFileDialog::*)(), Variant const**, int, Callable::CallError&) (/home/nathan/workspace/public/godot-4/./core/variant/binder_common.h:418)
[10] CallableCustomMethodPointer<EditorFileDialog>::call(Variant const**, int, Variant&, Callable::CallError&) const (/home/nathan/workspace/public/godot-4/./core/object/callable_method_pointer.h:99)
[11] Callable::callp(Variant const**, int, Variant&, Callable::CallError&) const (/home/nathan/workspace/public/godot-4/core/variant/callable.cpp:57)
[12] CallQueue::_call_function(Callable const&, Variant const*, int, bool) (/home/nathan/workspace/public/godot-4/core/object/message_queue.cpp:222)
[13] CallQueue::flush() (/home/nathan/workspace/public/godot-4/core/object/message_queue.cpp:328)
[14] SceneTree::physics_process(double) (/home/nathan/workspace/public/godot-4/scene/main/scene_tree.cpp:473)
[15] Main::iteration() (/home/nathan/workspace/public/godot-4/main/main.cpp:3750 (discriminator 3))
[16] OS_LinuxBSD::run() (/home/nathan/workspace/public/godot-4/platform/linuxbsd/os_linuxbsd.cpp:933 (discriminator 1))
[17] ./bin/godot.linuxbsd.editor.dev.x86_64(main+0x19b) [0x563adc2e2ea4] (/home/nathan/workspace/public/godot-4/platform/linuxbsd/godot_linuxbsd.cpp:76)
[18] /usr/lib/libc.so.6(+0x27cd0) [0x7ff742c1bcd0] (??:0)
[19] /usr/lib/libc.so.6(__libc_start_main+0x8a) [0x7ff742c1bd8a] (??:0)
[20] ./bin/godot.linuxbsd.editor.dev.x86_64(_start+0x25) [0x563adc2e2c35] (??:?)
-- END OF BACKTRACE --
================================================================
Aborted (core dumped)

Edit: Is it because FileSystemDock doesn't exist in project manager?

[YuriSizov]

Edit: Is it because FileSystemDock doesn't exist in project manager?

No, your stack trace doesn't show it crashing on the singleton or even on the previous line where we access another member of the dock class. It probably crashes because of this assignment:

assigned_folder_colors = ProjectSettings::get_singleton()->get_setting("file_customization/folder_colors");

Project settings don't exist outside of a project, so this probably assigns some gibberish to the member.

Edit: Well, this is pretty weird. I don't quite see why it doesn't crash on accessing the singleton, because yeah, of course there is no FileSystemDock in the PM. But somehow we can use the garbage returned by get_singleton() and call a method on it, and it will just work fine with a hashmap, but not with a dictionary.

In any case, the fix would indeed be to check if FileSystemDock::get_singleton() is nullptr and return the default theme_cache.folder_icon_color color in EditorFileDialog::get_dir_icon_color.

[YuriSizov]

@synalice Are you working on a fix?

Made a PR: #86171

[adamscott]

For more context, the export directory is also broken with the merge of this PR.
(I'll create a new issue, I will link it here)

[YuriSizov]

@adamscott Your problem is different, you get an infinite loop in this:

	String parent_dir = p_dir_path;
	while (parent_dir != "res://" && folder_icon_color == theme_cache.folder_icon_color) {
		if (!parent_dir.ends_with("/")) {
			parent_dir += "/";
		}
		if (assigned_folder_colors.has(parent_dir)) {
			folder_icon_color = folder_colors[assigned_folder_colors[parent_dir]];
		}
		parent_dir = parent_dir.trim_suffix("/").get_base_dir();
	}

Because parent_dir is already an empty string or /, and it never reaches the res:// state.