Avoid double handling of rename in the file system dock #91112
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
RandomShaper
force-pushed
the
fix_double_confirm
branch
from
f43bcb9 to
a60179b
Compare
|
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. |
|
Renaming in tree mode is now broken. |
RandomShaper
force-pushed
the
fix_double_confirm
branch
from
a60179b to
9f34996
Compare
|
Pushed with a different approach. (Please see the added note to the PR description.) |
RandomShaper
force-pushed
the
fix_double_confirm
branch
2 times, most recently
from
d2d9dc2 to
141d2e8
Compare
KoBeWi
approved these changes
May 1, 2024
|
Thank you very much for your continuous help testing this! |
RandomShaper
force-pushed
the
fix_double_confirm
branch
2 times, most recently
from
55b3093 to
fb30c1f
Compare
bruvzg
approved these changes
May 7, 2024
|
Can be rebased to avoid the confusion of including two commits when one is already in |
RandomShaper
force-pushed
the
fix_double_confirm
branch
from
fb30c1f to
eb2bd41
Compare
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #90473.
Bugsquad edit: Fixes #91648
In that issue, certain change in
DisplayServerWindowsseems 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
LineEditcould have a property, such assubmit_on_parent_closethat makes the intended behavior an opt-in feature of it. This PR fixes the issue in the meantime anyway.Lastly,
Treeseems to have a similar deambiguation code, but that one seems to still work with the currentDisplayServerimplementations. 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.