godot
godot copied to clipboard
godotengine
Avoid double handling of rename in the file system dock
Fixes #90473.
In that issue, certain change in DisplayServerWindows seems to be the cause of the issue on Windows. However, the report is about Linux. The fact that the issue is cross-platform makes sense if you think about what this PR does.
The floating text box used for rename can be confirmed via closing the popup containing it or via actually confirming the input. Since the latter involves the former, there's some code to avoid handling the user will twice. However, it's such a brittle deambiguation mechanism; namely, if certain keys are down, we assume the text input was already handled. It only makes sense that such a handling is subject to break with subtle changes in input event management across display servers. In fact, that's the problem: the rename operation is attempted twice because the current mechanism fails to block the second event.
This PR reimplements the involved handlers in a more robust way so the issue is gone and very unlikely to inadvertedly reappear again. That said, I'm not super happy with how ad hoc this is even in the new version. Maybe LineEdit could have a property, such as submit_on_parent_close that makes the intended behavior an opt-in feature of it. This PR fixes the issue in the meantime anyway.
Lastly, Tree seems to have a similar deambiguation code, but that one seems to still work with the current DisplayServer implementations. That said, it'd warrant similar love. (CC @bruvzg, that may want to review this PR and later improve the state of affairs.)
2024-05-01: This is now based on #91361, which fixes the underlying issue on Windows. However, sine this issue was not Window-specific and I'd still want to have a more solid mechanism here, I've reworked this PR doing so.
Apparently there is another regression 🤔 Closing name edit with Escape will accept it. It's the same before this PR though, but not in dev5.
Fixed. In the end, some not-very-elegant check has been needed. At least, it's based on an action and not on specific keys. I guess it's good enough until the time where this all is implemented as as first-class feature.
Pushed with a different approach. (Please see the added note to the PR description.)
Can be rebased to avoid the confusion of including two commits when one is already in master.
