Skip to content

Allow the filename_filter property to be changed. #104492

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

Conversation

WinnerWind
Copy link
Contributor

@WinnerWind WinnerWind commented Mar 22, 2025

This fixes #104067 in a hacky way.

image
image
image

A downside of this PR is that if the property is hidden away using this code

@tool
extends FileDialog

func _validate_property(property:Dictionary):
	if property.name == "filename_filter":
		property.usage = PROPERTY_USAGE_NO_EDITOR

The filename_filter property persists and still works. I could not find a fix for this, but suggestions are welcome. Using the code in commit 14f519f47425a2062542f1f70aa68622be617e88 does not work due to some unknown reason. I even tried doing it like this

	if (!filename_filter_box->is_visible()) {
		file_name_filter.clear();
	}

However that failed to provide the desired effect. As of now, it seems that removing the faulty code is the only way to proceed.

It appears that using !show_filename_filter makes the box permanently uneditable as it is being cleared all the time, and using show_filename_filter allows editing but does not clear the box when it is hidden away then un-hidden. Truly quite a deep problem. I couldn't figure out how to print messages so that I could debug the state of show_filename_filter, and exactly pinpoint what was going wrong.

NOTE : Ready to merge.

@WinnerWind WinnerWind requested a review from a team as a code owner March 22, 2025 17:27
This fixes godotengine#104067 in a hacky way.

This removes the problematic if statement in FileDialog, which was making the filename_filter box permanently un-editable.
@WinnerWind WinnerWind force-pushed the allow_editing_filename_filter_filedialog branch from f4ed584 to 760aea7 Compare March 23, 2025 05:38
@Chaosus Chaosus added this to the 4.5 milestone Mar 23, 2025
@KoBeWi
Copy link
Member

KoBeWi commented Apr 3, 2025

Simply removing the clear() will make the filter take effect when invisible, which can be confusing when you open a FileDialog and see no files, because of some hidden text field.
You can use _validate_property() to hide the property conditionally when the filter is disabled.

Although I wonder why is the filter even exposed as a property?

@akien-mga akien-mga changed the title Allow the filename_filter property to be changed. Allow the filename_filter property to be changed. Apr 3, 2025
@WinnerWind
Copy link
Contributor Author

WinnerWind commented Apr 3, 2025

Hello, that was something I was unfortunately un-able to figure out; how to make the filter clear itself when the box is not visible. It seems that something is horribly wrong in the back-end, or I'm really looking at it wrong. Either way, I'm not sure how to proceed, and I'm open to ideas.

I'm not sure what you mean by :

You can use _validate_property() to hide the property conditionally when the filter is disabled.
Although I wonder why is the filter even exposed as a property?

Apologies.

@KoBeWi
Copy link
Member

KoBeWi commented Apr 3, 2025

Ok I looked at FileDialog again (sorry, I wasn't familiar with filename_filter property) and the visibility of the filter is not exposed.
If there was a property for filter field visibility, you could hide the property from the inspector, but in the current state it's not possible.

I think filename_filter should've never been exposed as a property, because it doesn't make sense. What you can do for now is clear the filename_filter in set_show_filename_filter() method, when the filter box is supposed to appear. This, or make the filter ignored when hidden.

@WinnerWind
Copy link
Contributor Author

Hey.
Considering the existence of another array of filters, literally called filters, I'm not sure why filename_filter exists, because in theory the user can just put their required filter in the filters property. I've never done this, because according to the documentation, filters is for file extension filters and the other is for file name filters. But that's not the scope of this PR; to determine whether or not this property is required. I guess in a way it is, because native file dialogs (Like GTKs file dialog) will never respond to the file name being filtered but will always respond to the extensions being filtered.

Those are good compromises/solutions. I'll give those a try and see which one is easier.

Thanks!

@KoBeWi
Copy link
Member

KoBeWi commented Apr 4, 2025

But that's not the scope of this PR; to determine whether or not this property is required.

Yes, that should've been discussed in the original PR. Now it's too late .-.
The problem with this property is that it mimics the input field that user has control over. It's completely redundant. At most you can use it as "default search text", but idk why would you do that.

filters is part of dialog's configuration that customize it's behavior.
It doesn't help that the two have similar names, so it can be confusing 😅

@WinnerWind
Copy link
Contributor Author

WinnerWind commented Apr 6, 2025

Apologies if that was a joke that I did not get, but what PR are you talking about?

There could be uses for a "default search text". I've used this property myself in my (now defunct) M3U editor. When clicking the name of an artist, a file dialog opens with the default search text (filename_filter) being the text of the artist name. Same applies to the song title. Admittedly later on I switched to using native dialogs.

If it's intended to be used as a default search text, it could be renamed to something like search_query (Considering the property changes while the user is playing the game, calling it default_search_query would be incorrect.)

What do you suggest I do? Patch the bug or completely remove this property?

EDIT: When I say patch the bug, I'll find a new solution to the filename_filter not resetting properly when the box is hidden.
Thanks!

@KoBeWi
Copy link
Member

KoBeWi commented Apr 6, 2025

what PR are you talking about?

I meant the implementation pull request (#88673). It's fairly recent, but it was merged in 4.4, so removing or renaming the property is not possible (it would break compatibility).

Though if you say that default search text has some use then fine.

I came up with another solution. My problem with the current one is that it can result in the dialog filtering entries with invisible filter. It can be solved by making the filter field automatically appear when filename_filter is not empty.

@WinnerWind
Copy link
Contributor Author

It's a niche use indeed, but I feel it's still important to keep this property. It's not like we can remove it anyway.

I'll take a look at that PR and see what I can gather about the current implementation.

I'll also try all the solutions we've discussed and see which one works out the best. I didn't have much time recently but hopefully I will in this week.

Thanks!

@WinnerWind
Copy link
Contributor Author

Hello, just a status update.
I'll be out of town between 11th April and 25th April. I probably will not be able to work on this in between this period. Apologies.

@WinnerWind
Copy link
Contributor Author

Hello.
I am not able to figure out how to do this. Sorry.

The issue stems from the fact that using _validate_property() in the source code side gets over-ridden by _validate_property() on the gdscript side, which means that I'm unable to force the (editor) box to be visible if its set to PROPERTY_USAGE_NO_EDITOR. Similarly, it seems that I'm unable to access file_name_filter from that function as well. (Admittedly I'm not super knowledgeable on C++)

@akien-mga
Copy link
Member

Superseded by #106135. Thanks for the contribution!

@akien-mga akien-mga closed this May 7, 2025
@akien-mga akien-mga removed this from the 4.5 milestone May 7, 2025
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.

FileDialog's filename_filter field can't be edited in the inspector
4 participants