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 sorting of files/dirs in dialogs #87874

Merged
1 commit merged into from
Mar 24, 2024
Merged

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Feb 2, 2024

Sorts leading _ before all except ..

Optionally bound to scripting

Optionally added to FileDialog but can omit that if it's compatibility breaking

Added to a few places, think I've gotten all the places in the editor that handles files by ordering

The exact ordering can be further discussed, i.e. sorting _ before just alphanumerics, etc., but keeping it relatively simple here

Based on how sorting is done in Windows file explorer (though in Powershell it sorts _ after uppercase, so it depends, but I think it makes sense based on the reasons in the issue below)

@AThousandShips
Copy link
Member Author

AThousandShips commented Feb 2, 2024

This was originally intentional and done in:

Going with restoring the useful sorting here and unifying everything in this direction instead, the original sorting just used plain string comparisons, this now uses the new comparison setup, originally FOO would sort before _foo which was before foo

Keeping the case insensitive sorting that was already used

};

void sort_files();

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed this as it's not used anywhere that I could find, will restore if necessary

if (this_str && that_str) {
while (*this_str == '.' || *that_str == '.') {
if (*this_str++ != '.') {
static _FORCE_INLINE_ signed char naturalcasecmp_to_base(const char32_t *p_this_str, const char32_t *p_that_str) {
Copy link
Member Author

Choose a reason for hiding this comment

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

As both file and natural sorting use these I merged them to avoid code duplication

doc/classes/String.xml Outdated Show resolved Hide resolved
doc/classes/String.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

Nothing against this by the way, if it's the one way to fix this. I am not convinced on exposing this, since I can't recall anyone explicitly desiring it, but I am not severely opposed to it, either.

Maybe my pure approval will attract the rest of the herd.

@akien-mga akien-mga requested a review from bruvzg March 4, 2024 12:09
core/string/ustring.h Outdated Show resolved Hide resolved
Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

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

Looks good and seems to work fine (tested using the filesystem dialog). I think it makes sense to expose this even though nobody explicitly asked for it, because this is the behavior which was present in Godot 3.x.

@AThousandShips
Copy link
Member Author

Will fixup and squash :)

Sorts leading `_` before other characters except `.`.
@AThousandShips AThousandShips changed the title Fix sorting of files/dirs in editor Fix sorting of files/dirs in dialogs Mar 20, 2024
akien-mga added a commit that referenced this pull request Mar 24, 2024
Fix sorting of files/dirs in dialogs
@akien-mga akien-mga closed this pull request by merging all changes into godotengine:master in 06abc86 Mar 24, 2024
@akien-mga
Copy link
Member

Thanks!

@AThousandShips AThousandShips deleted the sort_fix branch March 24, 2024 08:46
@AThousandShips
Copy link
Member Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants