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

Make FileDialog filtering case insensitive #85789

Merged

Conversation

DevilboxGames
Copy link
Contributor

When using FileDialog to save files on non-case-sensitive file systems (Windows and often macOS), a file may have an uppercase file extension ( file.TXT ) but the filter is set as lowercase ( *.txt ; Text Files ). The FileDialog will list the file but selecting it to overwrite will not detect that the file matches the filter and append the lowercase extension after the existing uppercase file extension and would result in the selected file being file.TXT.txt

As most other operating systems can be case sensitive, my suggestion is to add a CaseSensitivity enum with the values of True, False and OS Default to the FileDialog for toggling between using f.match(str) and f.matchn(str) in the checks which would allow it to either be explicit based on the developer intent or implicit based on the standard of the operating system.

filedialog-case-sensitivity

Currently if OS Default is used then the code uses a case-sensitive check if the OS is not Windows or macOS. Is there a better approach for this? Are there any other supported OS's are not case-sensitive by default?

Attached is an MRP similar to the one provided in the issue, with the addition of an OptionButton for changing between the CaseSensitivity setting when running the project.
FileDialogCaseSensitivity.zip

Fixes #85788

@DevilboxGames DevilboxGames requested a review from a team as a code owner December 5, 2023 15:02
@AThousandShips
Copy link
Member

You also need to register these enums in _bind_methods, see the other ones

@AThousandShips
Copy link
Member

You also need to document these methods, use --doctool, see here

@DevilboxGames DevilboxGames requested a review from a team as a code owner December 5, 2023 18:02
@DevilboxGames
Copy link
Contributor Author

DevilboxGames commented Dec 5, 2023

You also need to document these methods, use --doctool, see here

I've updated the doc xml now, is this correct? The --doctool tool didn't generate any additional entries in it for the enum or methods. my mistake, I must have run it against the wrong exe, it fixed up the incorrect default when I re-ran it.

Documentation isn't my forte so the wording might need tweaking.

@DevilboxGames DevilboxGames force-pushed the FileDialogCaseSensitivity branch from 4d50f88 to 22a2e5c Compare December 7, 2023 19:29
@YuriSizov YuriSizov requested a review from bruvzg December 8, 2023 16:34
@DevilboxGames DevilboxGames force-pushed the FileDialogCaseSensitivity branch from 22a2e5c to 28deffd Compare February 5, 2024 09:07
@DevilboxGames
Copy link
Contributor Author

I noticed a merge conflict, so I've rebased and resolved it.

@DevilboxGames
Copy link
Contributor Author

It looks like one of the tests failed due to a ubuntu keyserver issue. Could a maintainer request a rerun of the failed test?

I appologise if the ping is innapropriate, but Yuri also requested requested @bruvzg to re-review the PR in December. Is there anything else which needs to be done before it can be approved for merging?

@Calinou
Copy link
Member

Calinou commented Feb 15, 2024

Tested locally on Linux (on a case-sensitive filesystem), I'm a bit confused about the behavior. The only behavior that seems desirable to me here is the one where case sensitive is set to False:

simplescreenrecorder-2024-02-15_20.19.39.mp4

Otherwise, you get a double extension appended. Is there a point for keeping case sensitive True (and OS Default on case-sensitive filesystems)?

Also, the file filter is always case-insensitive regardless of this option, but I guess that's expected.

@DevilboxGames
Copy link
Contributor Author

That behaviour looks as expected. Having the option for true and OS Default is so it doesn't break anything which might be reliant on a case sensitive file picking for whatever reason. I agree, having it set to false is the desirable option in all the use cases I could think of but I didn't feel comfortable making that assumption for everyone, hence having the option and defaulting to match the OS's case sensitivity.

The file filter was case insensitive for listing the files in the dialog originally, it was just the functionality of the detecting the extension on the selected filename and appending if it was missing which wasn't case insensitive, which resulted in the doubling of the file extension.

I could remove the setting and just make this always be case insensitive, or I could have it default to false and leave the option for the rare situtation it might be wanted? I'm open to whichever option is most suitable.

@AThousandShips AThousandShips modified the milestones: 4.3, 4.4 Jul 25, 2024
@DevilboxGames DevilboxGames force-pushed the FileDialogCaseSensitivity branch from 28deffd to 30fdcaa Compare September 3, 2024 13:13
@AThousandShips AThousandShips changed the title Add CaseSensitivity option to File Dialog Make FileDialog filtering case insensitive Sep 3, 2024
@AThousandShips AThousandShips requested a review from a team September 3, 2024 14:15
@KoBeWi
Copy link
Member

KoBeWi commented Sep 28, 2024

IMO this makes sense. The change only affects extension checking and I don't see why would you care about extension casing and make a double extension, like currently happens on mismatch.

The same change should be done in EditorFileDialog.

@DevilboxGames DevilboxGames force-pushed the FileDialogCaseSensitivity branch from 30fdcaa to a9e7e16 Compare October 7, 2024 13:52
@DevilboxGames DevilboxGames requested a review from a team as a code owner October 7, 2024 13:52
@DevilboxGames
Copy link
Contributor Author

IMO this makes sense. The change only affects extension checking and I don't see why would you care about extension casing and make a double extension, like currently happens on mismatch.

The same change should be done in EditorFileDialog.

Good shout, I've added the change to EditorFileDialog too, I also noticed FileDialog::_native_dialog_cb needed the change too so I've dropped it in there.

@KoBeWi
Copy link
Member

KoBeWi commented Oct 7, 2024

Needs to be squashed into 1 commit.

Change FileDialog and EditorFileDialog to use case insensitive string
comparisons when saving files to avoid duplicate file extensions.
@DevilboxGames DevilboxGames force-pushed the FileDialogCaseSensitivity branch from a9e7e16 to cd269b8 Compare October 7, 2024 14:08
@DevilboxGames
Copy link
Contributor Author

Commits squashed 👍

@akien-mga akien-mga merged commit 8de2abd into godotengine:master Nov 29, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

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.

Allow FileDialog filter checks to not be case sensitive
7 participants