Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix FileSystem dock navigation when using Split Mode #100336

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

larspet
Copy link
Contributor

@larspet larspet commented Dec 12, 2024

Fixes #100277

The regression was introduced by #100010 (most likely), which made bigger changes to _navigate_to_path().
This also simplifies the logic a bit, and does less trimming and appending of / all the time.

@larspet larspet requested a review from a team as a code owner December 12, 2024 19:35
@Sauermann Sauermann added this to the 4.4 milestone Dec 12, 2024
@KoBeWi
Copy link
Member

KoBeWi commented Dec 14, 2024

Show in FileSystem does not work properly in split mode. You need to use it twice to show the file.

@larspet larspet force-pushed the filesystem-dir-navigation branch 2 times, most recently from d9d52ed to 22dca1a Compare December 15, 2024 02:45
@larspet
Copy link
Contributor Author

larspet commented Dec 15, 2024

Show in FileSystem does not work properly in split mode. You need to use it twice to show the file.

Fixed.

Also, I've noticed that every time navigate_to_path() is called, _navigate_to_path() will be called twice, because of the "Filter Files" search box being cleared with file_list_search_box->clear() and thus invoking _search_changed().

This would be simple enough to fix in the case when it's already empty and doesn't need to be cleared, but in the other case it's a little more involved. Since this is also kind of a separate issue, it's probably better handled in a separate PR.

@KoBeWi
Copy link
Member

KoBeWi commented Dec 15, 2024

_navigate_to_path() will be called twice, because of the "Filter Files" search box being cleared with file_list_search_box->clear() and thus invoking _search_changed().

Sounds like #100275 will fix it.

@KoBeWi
Copy link
Member

KoBeWi commented Dec 15, 2024

Another bug is that it changes behavior when navigating to folder depending on / suffix. With the suffix it correctly selects the folder in tree (in split view), without the suffix it selects the parent folder instead.
The previous implementation was a bit hacky, but it was to ensure that folders work correctly both with and without the / suffix.

@larspet larspet force-pushed the filesystem-dir-navigation branch from 22dca1a to a79a4b5 Compare December 15, 2024 20:28
@larspet
Copy link
Contributor Author

larspet commented Dec 15, 2024

It should work now regardless of the path ending with / or not. I also made it default to res:// if the given path is empty.


if (!found) {
TreeItem **directory_ptr = folder_map.getptr(base_dir_path);
if (!directory_ptr) {
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this should result in an error 🤔

}
} else {
(*directory_ptr)->select(0);
_update_file_list(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be optimized to not update list when the base directory is the same, but that would probably require more changes so it's something potentially for later.

Comment on lines +752 to +753
while (item) {
if (item->get_metadata(0).operator String().ends_with(file_name)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is redundant when selecting directory, because it should always match anyway.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works correctly now.

@Repiteo Repiteo merged commit 10b333c into godotengine:master Dec 16, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 16, 2024

Thanks!

KoBeWi comments were all non-blockers, so they can be handled in a separate PR

@larspet larspet deleted the filesystem-dir-navigation branch December 19, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problems navigating folders when using FileSystem split view
4 participants