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 text editor stealing focus from "Find in Files" dialog on X11 #93682

Merged

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Jun 27, 2024

Fixes #88847

Works in my testing on Fedora 40 with GNOME using xwayland.

This reverts one of the changes from PR #86441, but not all of them.

Marking as a draft for now, because I'd like to attempt to determine if the original bug that PR #86441 fixed is still fixed with the changes here. Since this doesn't revert the whole thing, it's possible that bug is still fixed, and everybody wins! However, PR #86441 didn't fix the X11 bugs that I was personally having - I made that PR on behalf of another user, so I'll need to figure out how to reproduce the original bug.

UPDATE: I confirmed that this PR doesn't re-introduce at least one of the bugs fixed by PR #86441. See this comment.

@dsnopek
Copy link
Contributor Author

dsnopek commented Jun 27, 2024

I did manage to reproduce @Mequam's original focus issue on XFCE 4 that's shown in issue #74378, when using Godot 4.3-dev2 (it was fixed by PR #86441 which was released in Godot 4.3-dev3).

However, when testing with the changes in this PR, that bug is still fixed!

There were a number of other focus bugs on a number of other window managers described on issue #74378 that folks say got fixed by PR #86441, though, so it's entirely possible that the change here is necessary to fix one of them.

That said, I think this is a good enough sign to take this PR out of draft! :-)

@dsnopek dsnopek changed the title [DRAFT] Fix text editor stealing focus from "Find in Files" dialog on X11 Fix text editor stealing focus from "Find in Files" dialog on X11 Jun 27, 2024
@dsnopek dsnopek marked this pull request as ready for review June 27, 2024 23:03
@dsnopek
Copy link
Contributor Author

dsnopek commented Jun 27, 2024

Looking a little deeper at the change in this PR, it's changing DisplayServerX11::window_set_ime_active(), which is probably being called by TextEdit::_notification(NOTIFICATION_FOCUS_ENTER) when the search field on the "Find in Files" dialog grabs focus.

Based on my (admittedly rough) understanding of the problems described on issue #74378, I don't think the change reverted in this PR is really needed to fix those bugs.

Those bugs seem to center around Godot grabbing focus due to window-related events. However, the window_set_ime_active() happens when text controls gain focus. And, so, maybe we don't need the "protections" added in PR #86441 in this case?

Anyway, the only way to really know for sure is for this change to get lots more testing :-)

@dsnopek dsnopek added the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jun 27, 2024
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Tested on Fedora 40, with KDE Plasma, both Wayland (XWayland) and X11 flavors of it.

On both, I can reproduce the bug in master, and this PR fixes it.

The bug isn't reproducible with the Wayland DisplayDriver, but it doesn't support multiple windows so that's expected.

@akien-mga akien-mga merged commit ac9181c into godotengine:master Jun 28, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@pancelor
Copy link
Contributor

This fixed the issue for me -- Manjaro / KDE Plasma 6 / X11 -- thank you!

(full system info: Godot v4.3.beta (811ce36c6) - Manjaro Linux #1 SMP PREEMPT_DYNAMIC Mon May 27 03:41:25 UTC 2024 - X11 - GLES3 (Compatibility) - AMD Radeon RX 6600 (radeonsi, navi23, LLVM 17.0.6, DRM 3.54, 6.6.32-1-MANJARO) - AMD FX(tm)-8350 Eight-Core Processor (8 Threads))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release platform:linuxbsd regression topic:porting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Text editor steals focus from "Find in Files" dialog
3 participants