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 project manager stealing focus on i3 #96829

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Sep 10, 2024

Fixes #95824

TL;DR: This fix isn't ideal, but I think it's probably OK.

Here's what's going on in detail:

  • In LineEdit, on NOTIFICATION_DRAW (which happens every time the cursor blinks), if the LineEdit is focused, it will call DisplayServer::window_set_ime_active() which will grab focus for the window the LineEdit is in
  • Since Control::has_focus() only checks if the control is focused according to Godot and doesn't check if the window is focused, this can steal focus back to Godot as soon as the cursor blinks again
  • So, this PR adds a check to DisplayServer::window_is_focused()
  • Unfortunately, this on its own doesn't fix the issue, because we're using the X11 FocusOut event to mark a window as not focused, and these events come in asynchronously, creating a race condition. What happens is focus switches away from the project manager window, but before the FocusOut event is received, LineEdit's cursor blinks and it still thinks it's window is focused
  • So, this PR also changes DisplayServer::window_is_focused() to do a synchronous check to see if the window is really focused right now

This isn't ideal for a couple reasons:

  1. It's ignoring the WindowData::focused flag (which we update in response to the X11 FocusIn and FocusOut events) which is used in a whole bunch of other places. However, I think this is OK, because those other places don't seem to be sensitive to similar race conditions, and it sort of makes sense that if we call DisplayServer::window_is_focused() then we probably want the most up-to-date data.
  2. The fix isn't contained to display_server_x11.cpp, it has ventured into the LineEdit and TextEdit nodes. But the only possible fix that I could come up with entirely within display_server_x11.cpp would be to revert PR Fix text editor stealing focus from "Find in Files" dialog on X11 #93682, but that would cause problems with Gnome and KDE. However, I think this fix is OK, because the whole idea of grabbing focus in NOTIFICATION_DRAW seems a little problematic in the first place - adding some extra protections around that makes sense to me.

Please let me know what you think!

@dsnopek dsnopek added bug platform:linuxbsd cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Sep 10, 2024
@dsnopek dsnopek added this to the 4.4 milestone Sep 10, 2024
@dsnopek dsnopek requested review from a team as code owners September 10, 2024 23:00
@Rindbee
Copy link
Contributor

Rindbee commented Sep 10, 2024

I tested this PR and confirmed that it fixes #95824; however, when using F1 to open the Search Help popup, the search bar no longer automatically gets focus.

@dsnopek
Copy link
Contributor Author

dsnopek commented Sep 11, 2024

however, when using F1 to open the Search Help popup, the search bar no longer automatically gets focus.

Hm. I'm able to reproduce this, but only if the code editor is what is focused when I press F1. If the 2D or 3D viewport is showing, it works fine. I think perhaps the CodeEdit control has a similar issue to LineEdit? I'll look into it.

@dsnopek dsnopek force-pushed the x11-focus-bugs-take-twenty-seven-million-and-four branch from e4614cd to 8333b63 Compare September 11, 2024 00:18
@dsnopek
Copy link
Contributor Author

dsnopek commented Sep 11, 2024

My latest push adds the same fix to TextEdit (the parent of CodeEdit) as well, which fixes the issue for me. Please let me know if that works for you, and/or if you notice any other issues :-)

@Rindbee
Copy link
Contributor

Rindbee commented Sep 11, 2024

The latest modification solved this issue for me as well.

@dsnopek dsnopek force-pushed the x11-focus-bugs-take-twenty-seven-million-and-four branch from 8333b63 to b1871cd Compare September 16, 2024 22:00
Comment on lines +3001 to +3005
Window focused_window;
int focus_ret_state;
XGetInputFocus(x11_display, &focused_window, &focus_ret_state);

return wd.x11_window == focused_window;
Copy link
Member

Choose a reason for hiding this comment

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

Should we update wd.focused accordingly after doing this synchronous check?

Copy link
Contributor Author

@dsnopek dsnopek Sep 17, 2024

Choose a reason for hiding this comment

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

My personal inclination would be not to update wd.focused here.

There's a bunch of other state that changes when the FocusIn and FocusOut events are received, and unless we're going to also run all of that, I don't want to just change wd.focused, which could lead to the internal state of DisplayServerX11 being temporarily inconsistent (temporary, because everything else will get changed when the event finally comes in).

But if other folks who are more expert in this code (ex @bruvzg) think it would be better to update wd.focused here, I'd be happy to defer to their judgement and update the PR :-)

@akien-mga akien-mga merged commit 1d3e9b3 into godotengine:master Sep 18, 2024
20 checks passed
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

Godot project manager continuously steals focus on i3
3 participants