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

Godot Editor and Project Manager steals focus on a window manager on Linux. #68305

Open
nargacu83 opened this issue Nov 5, 2022 · 47 comments
Open

Comments

@nargacu83
Copy link

nargacu83 commented Nov 5, 2022


Bugsquad note: This issue has been confirmed several times already. No need to confirm it further.


Godot version

v4.0.beta4.official [e675154] (any 4.0 version)

System information

Arch Linux with kernel 6.0.7-arch1-1 on AwesomeWM, X11

Issue description

The project manager and the editor steals the focus on Window Managers on Linux with X11 when opening other programs or when changing layouts. It doesn't happen on Godot 3.x.

I tested it on AwesomeWM, Hypr, LeftWM and DWM. Only LeftWM and DWM doesn't have the issue but i'm pretty sure they don't "implement" ConfigureNotify but i don't know much in the display server area.

However i managed to "fix" the issue by commenting out this if statement.

if ((xwa.map_state == IsViewable) && !wd.no_focus && !wd.is_popup) {
XSetInputFocus(x11_display, wd.x11_window, RevertToPointerRoot, CurrentTime);
}

Steps to reproduce

  1. Login with AwesomeWM
  2. Be on the awful.layout.suit.tile layout
  3. Launch any version of Godot 4.0
  4. Launch another program

Minimal reproduction project

You can use the default configuration of AwesomeWM.
No project needed.

@fjfnaranjo
Copy link

I can also confirm this bug for Fluxbox on Debian 10.

LXDE on Debian 11 works as expected.

The fastest way to reproduce it is opening Godot with an empty project list. The project list window and the "open assets store" popup fight for the focus.

@lihop
Copy link

lihop commented Dec 27, 2022

I can confirm this issue for Godot 4.0-beta10 with XMonad 0.17.1 on NixOS 2.11.

Expected and actual behavior is shown below. I have configured XMonad to draw a bright green border around the currently focused window.

Expected Behavior (Godot 3.4-stable) Actual Behavior (Godot 4.0-beta10)
focus-right focus-wrong

As can be seen above, the correct window is focused (terminal window has a bright green border), but keyboard input is going to the unfocused Godot window.

The issue also occurs when launching Godot with the --single-window option.

@lihop
Copy link

lihop commented Jan 14, 2023

Some other impacts of this issue:

  • Input focus is stolen from newly created windows (for example, when opening a new terminal window in the workspace the new terminal window should have focus). In this case the new window has focus, but input goes to Godot.

  • Input focus is stolen when resizing windows. Again, the window that had focus before resizing still has focus, but input goes to Godot.

    Expected (Godot 3.5) Actual (Godot 4.0-beta12)
    good bad

In both cases, clicking the window that should have focus does not allow it to regain input focus as the window manager already considers the window to be focused. Instead you have to click another window so that it loses focus and then click back to the target window so it regains full focus (including input focus).

@nargacu83
Copy link
Author

nargacu83 commented Jan 16, 2023

After studying the problem a little bit, it seems like the if i commented out doesn't impact any editor behavior neither the game windows behavior. By looking at the XSetInputFocus function definition and comments, it only ensure that the focus isn't lost. I have no clue what really that means in the context of most Desktop Environments or Window Managers, maybe you can lose the focus on a more barebone WM or DE that doesn't have it's focus system or something like it?

The problem doesn't happen on KDE Plasma with the Bismuth addon, so that's my temporary solution for now.

I'm trying to do a fix for it but unsure how to produce a test for this or if it's even necessary to have this piece of code in here, since the ConfigureNotify event mostly informs godot that it's geometry has changed, which is normal in a non floating window manager i guess. After testing it on AwesomeWM and KDE Plasma without the addon, it seems to work fine.

If we can't just remove it, maybe we can fix this by making sure one of the parent windows doesn't already have the focus before executing the function?

@clayjohn clayjohn modified the milestones: 4.0, 4.x Jan 26, 2023
@fjfnaranjo
Copy link

I want to report the issue still happens in the RC1 for 4.0. I will keep track of this issue and check again in 4.1. I will stick to the latest stable (3.x) for now and if I need 4.x for a project I already checked the next patch fixes the issue for me:

diff --git a/platform/linuxbsd/x11/display_server_x11.cpp b/platform/linuxbsd/x11/display_server_x11.cpp
index 525c62fbf2..0f741ef035 100644
--- a/platform/linuxbsd/x11/display_server_x11.cpp
+++ b/platform/linuxbsd/x11/display_server_x11.cpp
@@ -4215,7 +4215,7 @@ void DisplayServerX11::process_events() {
 				// RevertToPointerRoot is used to make sure we don't lose all focus in case
 				// a subwindow and its parent are both destroyed.
 				if ((xwa.map_state == IsViewable) && !wd.no_focus && !wd.is_popup) {
-					XSetInputFocus(x11_display, wd.x11_window, RevertToPointerRoot, CurrentTime);
+					//XSetInputFocus(x11_display, wd.x11_window, RevertToPointerRoot, CurrentTime);
 				}
 
 				_window_changed(&event);

As @nargacu83 pointed out, the challenge for this is understanding whats that line trying to do exactly and why it was necessary in the first place. I worry removing it may produce a regression if we don't try at least the 5/6 most common WM for X11.

@GrammAcc
Copy link
Contributor

Reporting that this bug is present on:

  • Godot version: 4.1.dev.custom_build.4c677c88e
  • Arch Linux version: Linux version 6.1.7-arch1-1 (linux@archlinux) (gcc (GCC) 12.2.1 20230111, GNU ld (GNU Binutils) 2.40)
  • With a Fluxbox-only X11 launched via xinit.

For me, this issue is a hard blocker for using Godot4 because even in single-window mode, the editor and the debugger fight for focus when running the game via F5, which is a core part of my workflow when prototyping in GDScript.

Also, while the two windows are fighting for focus, they steal all input, so I can't even use an xbindkeys binding to kill the engine or Fluxbox's keybindings to kill X11, and I have to force shutdown my computer.

Currently using this shell script to test:

!#/bin/sh
godot4 &
sleep 60
pkill -x godot4

Commenting out the conditional in display_server_x11.cpp that @nargacu83 identified appears to fix the problem, but I agree that there must be a reason why it's there, so I second the desire for more thorough testing.

I don't know enough C++ to contribute any code, but I would be happy to help with regression testing if someone else submits a PR.

Let me know if there is anything specific that I can help test. :)

@fjfnaranjo
Copy link

Seeing that @bruvzg is assigned, if you need further details on this, feel free to ask. I'm also available on Discord if you need to discuss it.

If you want to get to an old commit when the majority of the design for the definition file is done, you are probably looking at 4396e98 .

DEBUG_LOG_X11 can help you a lot. Adding more lines to that log will probably help to future developers.

You are also probably interested in an application called xev. Is a simple X11 test program that opens a window and logs to the standard output its X11 events as sent by the window manager.

@reduz deserves a shout out for that implementation. It works very well, handles lots of edge cases and it looks like he was losing his mind in the process:

https://github.com/godotengine/godot/blob/2eec9a67d564b11326f44c5ef8b6b6f9aec251b9/platform/linuxbsd/x11/display_server_x11.cpp#LL4322C7-L4322C85

@br3eze
Copy link

br3eze commented Jul 8, 2023

Still breaks everything.
Arch Linux, X11, Fluxbox
Godot 3+ and 4.1

I didn't open a new report for 4.1

@fjfnaranjo
Copy link

Godot 3+ and 4.1

Looks like a different bug. Godot 3+ was fine in my system (also X11+Fluxbox). Maybe they backported the changes from 4, but I don't think so. Are you experiencing the same behavior? The easiest way to reproduce this is opening the editor with a empty project list.

@br3eze
Copy link

br3eze commented Jul 8, 2023

I'm sorry, it's not about 3.5.2 (checked). My mistake.

Tried:
Godot_v4.0.3-stable_linux.x86_64
Godot_v4.1-stable_linux.x86_64

There were no problems a ~ week or two ago. But now I just can't create or open projects. Main window and dialog starts blinking. It hangs whole WM, until killall Godot....
Got all Arch updates.

@br3eze
Copy link

br3eze commented Jul 21, 2023

So, if it kills all the thing, why is it there???
Just can't understand.

XSetInputFocus(x11_display, wd.x11_window, RevertToPointerRoot, CurrentTime);

I mean 4.1.1 still has this issue..

@nargacu83
Copy link
Author

So, if it kills all the thing, why is it there???

Well, i may have an answer for that. If i understood correctly. It appears to handle an internal focus state. Godot will check if any window and it's parent should have the focus. The problem here, is this focus state is not in sync with the WM focus.

@crestofthebeast
Copy link

Just wanted to leave my two cents as this issue makes godot nearly unusable for me as someone who is reliant on window managers to be able to use my computer without my hands exploding in a comical puff of dust.

If the presence of lines 3855 ~ 3857 in display_server_x11.cpp causes these issues, and we aren't clear on if removing them is a regression, surely removing the lines and seeing what the bug reports that rise from that look like would be nominally more useful than blindly trying to figure out how to fix this?

Idk, definitely a small-scope take from me that likely won't work in a project as big as Godot. Just really hope to see this fixed and to not have to deal with using a bespoke fork on all my linux machines.

@fjfnaranjo
Copy link

surely removing the lines and seeing what the bug reports that rise from that look like would be nominally more useful than blindly trying to figure out how to fix this?

This is fine for any of us to do in our local builds. And it will certainly be one of the steps tried by whoever tackles the issue. But Godot is used by lots of people and using them as guinea pigs for debugging is not good engineering and it will hurt the project.

I automated the creation of the fork a long time ago and I suggest you do the same. This doesn't affects many users and looks insanely complex.

If they ever fix it they will be doing us a favor. With only the benefit of one of them training on X11. And X11 is dying (sadly, because my hands also explode if my carefully keyboard-based crafted environment changes).

There is also the option of the bounty... But I guess it will be also difficult to find someone for it.

@GrammAcc
Copy link
Contributor

Just in case anyone finds it useful, I created a fork that includes the workaround patch so that affected users that aren't comfortable with editing the C++ code can start using Godot 4 without maintaining a patched version of the engine themselves.

https://github.com/GrammAcc/godot4-workaround-68305

It's in the README on the fork, but just for the sake of CYA, I'll reiterate it here: This fork is not intended for use in production. Since we don't know what kind of regressions the workaround might introduce, you should NOT export any projects to production using this fork.

I can't contribute much to the engine itself, but hopefully this is helpful. :)

@crestofthebeast
Copy link

Just in case anyone finds it useful, I created a fork that includes the workaround patch so that affected users that aren't comfortable with editing the C++ code can start using Godot 4 without maintaining a patched version of the engine themselves.

https://github.com/GrammAcc/godot4-workaround-68305

It's in the README on the fork, but just for the sake of CYA, I'll reiterate it here: This fork is not intended for use in production. Since we don't know what kind of regressions the workaround might introduce, you should NOT export any projects to production using this fork.

I can't contribute much to the engine itself, but hopefully this is helpful. :)

Will try developing on this fork and report any unexpected behaviour in the issues there -- can hopefully be useful to the assignee of this issue.

@GrammAcc
Copy link
Contributor

Reporting a possible regression with the workaround identified in this thread.

When running the project with F5, everything seems to work when the editor is on the 2D, 3D, or Script editor tabs, but when running with the editor on the Asset Lib tab, the editor main window steals focus from the launched game window.

This isn't a serious problem since the game window can still be manually focused after it opens, but this is an indication that something is not working correctly with this workaround.

Additionally, there is a noticeable flicker in the main editor window when running via F5 on the 2D, 3D, and Script tabs, so I think they might still be vying for focus when the game window is opened and it just happens that the Asset Lib tab ends up with the focus where the others don't.

I'm not sure what the difference in the Asset Lib plugin and other editors is, but I wanted to pass this along since the difference in focus behavior between the different editor tabs might give a hint as to what is really going on here. :)

Tested this on versions:
4.1.1.stable.custom_build.d11b085c9
4.2.dev.custom_build.4714e9589 (using dev_mode=yes build option)

Linux version 6.4.10-arch1-1 (linux@archlinux) (gcc (GCC) 13.2.1 20230801, GNU ld (GNU Binutils) 2.41.0)

@nargacu83
Copy link
Author

nargacu83 commented Oct 22, 2023

I haven't got much time to work on it since last time, most of my attempts failed but i'm not familiar with C++ and with X11. If anyone wants to fix this issue, here's what i think.

I was trying to keep the focus order in sync without affecting the DE/WM focus. In my understanding, it does keep a focus order already but i wanted to check if the window wanting the focus is actually the correct one. Which could solve problems like @GrammAcc said, the editor can steal focus from other sub-windows like the game debug window.

This is just the one side of the problem since, as mentionned by @lihop @unlessgames, it also steals focus when changing tags/workspaces, that means implementing missing XDG event types or modify the implemented ones, so we may have more work to do in that area but who knows i may be overthinking this.

Also i recently went back to KDE Plasma on Wayland and noticed small focus issues too! It may not be only on Window Managers. (Could be a KDE issue as well). I'm really looking forward to see Godot Wayland to see if this issue will still be here though.

@GrammAcc
Copy link
Contributor

Sorry for ghosting this thread for a while. Been busy with work and internships. :)

@nargacu83 I'd be willing to take over on this one once I'm finished with the other issue I'm working on, but I'm not sure when that will be. I don't have a lot of free time these days, so I pretty much only work on open source stuff on Saturdays. It might be a couple more weeks before I can work on this one. Maybe even longer. :/

Hi @GrammAcc, your workaround applied to the latest godot source fixes some of the focus steal issues on i3 for me while not introducing the same focus loss on popup windows, however a detached text editor wil stilll steal focus from a running game window and still occasionally prevents switching the workspace away from the one with godot.

I wonder about when can the case described above your edit happen?

// Set focus when menu window is re-used.
// RevertToPointerRoot is used to make sure we don't lose all focus in case
// a subwindow and its parent are both destroyed.

When is a window reused and when can a subwindow and its parent be destroyed? I'd appreciate any insight on this.

I'm not sure what combination of events would result in that condition. I simply applied the workaround identified in this thread, so that other people wouldn't have to themselves. 😅 I'm also a C++ newbie and not very familiar with X11 either. That being said, I broke out the pickaxe and found the PR that adds the block in question: #41456, but it is from long before this issue was opened. I noticed these problems on my machine prior to this issue as well, but I wasn't using Godot4 until late 2022, so I'm not sure when this problem was actually introduced. When I get the chance, I will try building from the commit in that PR to see if this problem was introduced at that time, but I doubt it since it wasn't reported until recently. In any case, this should at least give us an idea of whether the root cause is the ConfigureNotify handler or if there is a deeper problem with the display server implementation on X11 causing events to get mishandled. :)

@GrammAcc
Copy link
Contributor

Well I tried to test this out on 2b49cb0 from #41456, but the third party libs in that commit aren't compiling on my machine. 🙃

The other issue I was working on got taken care of by another contributor, so I can start digging deeper into this one. It looks like there was quite a bit of discussion in #74378 regarding tiling WMs, and it seems the common ground between the two issues is that Godot steals focus when windows are resized or moved, even if window focus doesn't actually change. That would explain why commenting out the XSetInputFocus call in the ConfigureNotify event somewhat fixes the issue since that event is fired whenever the properties of a window change, which includes things like geometry and location xlib docs. Also, the problem that I have on Arch with fluxbox where the main editor window and any godot popup infinitely trade focus and lock my entire machine could also be caused by that due to the stacking order change being included in requests that trigger the ConfigureNotify handler.

The input focus section of the Xlib docs seems promising, but I don't have the time or energy to digest it right now.

One possibility for a root cause of all these issues that comes to mind is that the current implementation is actually correct as far as what should happen during each event, but due to the asynchronous nature of the client-server communication between X and each different UI application on the desktop, some events are being handled out of order resulting in the same kind of non-deterministic behavior that we get when we have race conditions in concurrent code. I will look into the way that X handles these different events, but if there is a sequence of actions that need to happen before an XSetInputFocus request would be safe to call, then possibly a simple 100-200 ms timer in the event handler could solve the problem, but I'm sure that's just wishful thinking. 😆

I will try to dig into this some more when I have more time, but it seems like an actual fix and not just a workaround will require digging deeper into how all of the events are being handled.

@GrammAcc
Copy link
Contributor

GrammAcc commented Nov 4, 2023

Updated my workaround fork to latest stable version 4.1.3. It still works like the previous versions of the workaround on my machine. :)

I'm also reading up on the X Window System since I don't really understand how all the events are used or how they fit together yet. Will report back here if I figure out anything about the Godot implementation that might be useful.

@GrammAcc
Copy link
Contributor

I have had very little time to work on Godot lately, and I've been preoccupied with a separate issue that was related to a PR I submitted previously, so I have not been able to look into this any more since my last comment at the beginning of November.

I'm starting to wonder if we should just submit the workaround as a PR and go from there since no one has reported any significant regressions with it and no one seems to have the time to really figure out the problem with X11 either?

It doesn't solve the workspace issues mentioned in #68305 (comment), but without it, I can't even open Godot on my machine since any popup will cause a total lock of my machine and I have to force shutdown to fix it. I'm not sure if the bug is that severe on anyone else's setup, but considering the severity on some machines, I think it's probably worth it to just submit the workaround as a PR if we don't have the resources to research a full fix. I do worry that maybe there could be a regression on some other setup and we haven't heard about it since users on other setups aren't affected by the bug in the first place, but in the worst case, the PR reviewer will just reject the PR and we'll be back to scrounging for help with this bug.

@nargacu83 do you want to submit a PR for this since you originally identified the workaround of removing the XSetInputFocus call in the ConfigureNotify event handler? I can also submit the PR and credit you if you don't have the time/don't want to deal with the review and possible corrections. Just let me know. :)

@fjfnaranjo
Copy link

I will reply to this as I replied to: #68305 (comment) ITT

We should not use neither the final Godot users nor the internal time of the reviewers to validate a workaround that we did not validated/understood ourselves.

I deal with this using a different WM when I work with Godot. Obviously is not ideal, but is not a "blocker" issue for that many systems if you add an alternative option to your DM/startx to launch LXDE (or any other working WM).

@GrammAcc
Copy link
Contributor

I will reply to this as I replied to: #68305 (comment) ITT

We should not use neither the final Godot users nor the internal time of the reviewers to validate a workaround that we did not validated/understood ourselves.

I deal with this using a different WM when I work with Godot. Obviously is not ideal, but is not a "blocker" issue for that many systems if you add an alternative option to your DM/startx to launch LXDE (or any other working WM).

I was not trying to suggest that we use the users or maintainers to validate the workaround. I was under the assumption that all of the affected users in this thread are using this workaround, and none of them have reported any regressions for several months. I would consider this to be validation of the workaround in as much as I would consider a thorough fix validated. Ultimately, any fix we provide will need to be tested on other systems/setups to ensure it doesn't cause regressions.

I completely agree with you about using users/maintainers for validation, and I really want more testing before submitting something, but no one here has the time to really dig into this, and I'm not comfortable with the assumption that this critical bug is completely isolated from the rest of the system. This is a bug that causes total system lock on X11 with certain configurations. This might not affect users of Godot, but what about users of Godot games? We have to keep in mind that Godot games are the editor with some extra code included, and since it's a platform bug and not specifically an editor UI bug, it's entirely possible that someone loading up a popular title built with Godot4 on the Steam Deck or a Linux desktop could encounter the same bug when the game pops up a notification window about a Patreon or something.

This is a very contrived example, but I'm just trying to articulate my concerns with the current situation. I feel like sitting on this when we have a potential fix for the most critical portion of this bug is irresponsible, and I just don't have the time to properly investigate a full fix.

You make a completely valid point about not wasting the maintainers time though. This is a massive project, and they are very busy. I will ask about this in the Godot Contributors Chat to see what the experienced/core contributors think we should do.

I'll report back here once I get an answer. :)

@nargacu83
Copy link
Author

I agree, we should not do a PR about this unless we found and tested a real solution and not just a partial workaround on this issue. I'm sure we will find a suitable solution in the future. I will try to allocate some time to do a proper test on this issue to locate where and why the fix is needed. In the meantime my personal workaround has been to use a Wayland compositor or DE which the issue is isolated in XWayland (some annoyances with color picker or nodes selection modal for example).

@fjfnaranjo
Copy link

fjfnaranjo commented Dec 12, 2023

I'll report back here once I get an answer. :)

It will be really nice to hear their feedback/ETA on this problem. Thanks!


Just to clarify, this only affects some X11 users using a particular set of WMs (in my case, Fluxbox). The issue disappears when using a different WM so it is completely plausible that Godot is doing a proper handling of X11 notifications and the WMs themselves have the problem. There is not reason nor evidence to suspect that Steam Deck or Linux standard users will find this problem affecting them. This will happen only to such users that at the same time are using one of the affected WMs, requiring some knowledge of customization.

Also, in my case, I had problems with the initial versions workaround (I don't remember the particulars right now) and I switched my WM to remain in the official Godot release build. I haven't tried the new version of the workaround but it looked more promising that the previous one (keeping the state of the window certainly looks safer than just commenting some lines).

@fjfnaranjo
Copy link

I agree, we should not do a PR about this unless we found and tested a real solution and not just a partial workaround on this issue. I'm sure we will find a suitable solution in the future. I will try to allocate some time to do a proper test on this issue to locate where and why the fix is needed. In the meantime my personal workaround has been to use a Wayland compositor or DE which the issue is isolated in XWayland (some annoyances with color picker or nodes selection modal for example).

I always was suspicious about something (either Godot or the WM) considering X11 notifications (or a wrong subset of them) as focus grab intents. I didn't have the time to confirm this but peeking around using xev gave me that impression. Were you able to pinpoint this a little more? What do you think about the "every notification is a grab" theory?

@GrammAcc
Copy link
Contributor

I got a response in the Contributors Chat from @Riteo, a contributor working on the Wayland implementation:

pr it
worst case it gets closed
but AFAIK it's not the first nor the last hack inside that backend
we don't have any dedicated X11 maintainers anyways, so a hack is probably the best thing we're going to get for the time being

I can submit a PR with the workaround after work today. I think I will just submit the original workaround that simply removes the XSetInputFocus in ConfigureNotify since @unlessgames had mentioned some odd focus behavior with the WIP solution @nargacu83 has going.

Even if it gets merged, this is just a stop-gap, so eventually we will need a proper fix, and there is also #74378 to consider as well.

One thing that will help with peace of mind is if the affected users who are running other DEs to workaround this issue can test out the version with the workaround included in Godot on the DEs that they have already confirmed work without it. That should give us at least some regression coverage, and if it does break on those DEs, we would then actually know that there is a problem with this hack.

Let me know if anyone has any questions about the PR. I don't think it should have much affect on work we do on this issue going forward though. :)

@GrammAcc
Copy link
Contributor

I submitted a workaround PR #86101. Since this is a low priority issue for the maintainers, it probably won't be reviewed right away (especially with 4.2.1 in RC rn). If a breakthrough is made on this issue in the meantime and it looks like we'll have a full fix soon, I can convert it to a draft to prevent complications. Feel free to @-mention me for help with testing. I'm too busy to put a lot of time into development these days, but I can at least compile and run the engine to see if a patch fixes this issue. :)

@GrammAcc
Copy link
Contributor

Hey everyone, the maintainers gave some feedback on the workaround PR. In particular: #86101 (comment)

If anyone here who is affected by this bug is able to test the commit in the PR db4a662 on unaffected DEs/WMs, please comment if you see any issues or if the problem appears on other setups.

It looks like the maintainers have already confirmed it works on i3 and KDE Plasma, so other non-tiling WMs/DEs would be good to test if possible. I'm going to try to get a few other DEs/WMs installed and tested after work this week, but any additional testing/confirmation is greatly appreciated. :)

Thanks!

@unlessgames
Copy link

Thanks for still caring about this issue @GrammAcc

Your deletions fix the consistent focus stealing which I reported on with #80170 that seemed to happen under any circumstance in i3.

However the following issue can still be reproduced:
If I have my cursor over the detached editor window, it will consistently take back focus for itself making it impossible to move away from Godot via keyboard.

Here is a video, the first part shows stepping the focus through windows via keyboard working as expected.
The second part I move my cursor over the detached code editor window and keep pressing focus left repeatedly, the result is the focus going to the main window and then back to editor when it should go to the terminal on the left.

godot_focus_i3.mp4

As you can see the left terminal shows the X11 debug logs and this particular issue might be related to incorrect handling of MapNotify?

Another thing to point out is that triggering of the NOTIFICATION_APPLICATION_FOCUS_OUT notification within Godot does not happen consistently even if indeed all focus is lost. Then if it does get fired it is delayed from the actual loss of focus which might be also related to these bugs?

@fjfnaranjo : What do you think about the "every notification is a grab" theory?

This isn't and shouldn't be the case. Currently there is VisibilityNotify when a workspace becomes visible with a Godot window on it or when the i3 bar overlays on top of the windows, there is also ConfigureNotify for example when other windows get resized or moved on the same workspace (and the tiling WM resizes Godot as a result), this also doesn't result in focus grab.

@GrammAcc
Copy link
Contributor

Yeah, the workaround PR only fixes the most critical problem which is total system lockup on certain specific configurations (mine lol) and is really just a stop-gap to keep that from happening in exported games, so I don't expect it to really fix most of the focus issues we're dealing with between this issue and #74378.

This is super helpful info for actually fixing the real problem though. Thanks!

Unfortunately, as Riteo mentioned in the contributor's chat, it looks like the X11 backend is pretty hacked together, and based on the issues referenced by maintainers in the workaround PR discussion, I think the focus behavior has gone through several iterations and will probably require a pretty significant rework of the various event handlers to solve these problems without introducing others.

I still haven't had time to really read up on X11, and what I have read up on is very difficult to make sense of. It doesn't
help that they use multiple terms for the same thing in different situations.

Here's the API documentation for the MapNotify event:

This event is reported to clients selecting StructureNotify on the window and to clients selecting SubstructureNotify on the parent. It is generated when the window changes state from unmapped to mapped. The event is the window on which the event was generated, and the window is the window that is mapped. The override-redirect flag is from the window's attribute.

I still haven't figured out what an event is in X11. 😅

I think the terms unmapped and mapped mean not created/spawned and created/spawned, so I would think that MapNotify shouldn't be firing on focus swap, only when the window is opened initially. I may be wrong about that though.

I wish the X11 docs had a glossary. :)

@Riteo
Copy link
Contributor

Riteo commented Dec 15, 2023

I think the terms unmapped and mapped mean not created/spawned and created/spawned, so I would think that MapNotify shouldn't be firing on focus swap, only when the window is opened initially. I may be wrong about that though.

@GrammAcc FTR, "mapped" should mean "shown" as in "I created this window object but still haven't shown/mapped it on the screen".

@redoxcode
Copy link

I had this issue using arch and fluxbox. Kept me from switching to Godot 4 for some time, since it's hard to find what exactly causes the issue if you have no clue. I first suspected issues within my window manager. Even when I narrowed things down to an issue with godot, this PR wasn't easy to find.
However, this PR fixes the issue for me. #74378 is still present, but that issue is far less crippling. Thank You!
I hope this gets merged or fixed with an other PR soon, as I guess it keeps new users from trying godot under linux.

@GrammAcc
Copy link
Contributor

FYI for anyone who is watching this issue. The workaround that I was adding to releases in my fork has been merged into master, so anyone who is using that fork should switch back to the official binaries on the next release. I'll probably delete the fork after the next official release as long as my other open PRs are merged or closed by then. There isn't really any reason to maintain it at this point, and I've fallen behind on patching the stable releases as well. :)

Also, it would be great if anyone who is affected by this issue could build from source on the latest master to see how things go. The workaround that was merged in #86101 doesn't fully resolve this issue, but it fixes the system lockup on my setup, and other users have reported some improvements with it previously. A couple other PRs related to focus issues on X11 (#86441 #86671) have been merged since then though, so any feedback on the current build would be appreciated. Thanks!

@speakk
Copy link

speakk commented Mar 20, 2024

With current v4.3 dev5 the focus issue still happens with the find dialog on linux

@GrammAcc
Copy link
Contributor

With current v4.3 dev5 the focus issue still happens with the find dialog on linux

When you say "find dialog", you are referring to an OS dialog like an app picker, right? Sorry if this is common terminology, I use a bespoke WM setup, so I don't have most standard linux UI features. 🙃

I ask because there was another issue of Godot windows and sub-windows fighting for focus amongst themselves that should be fixed in latest dev, so I wanted to make sure it wasn't a problem with that fix.

If you can confirm your distro and WM/DE, that would be helpful info for anyone working on this as well. :)

@speakk
Copy link

speakk commented Mar 20, 2024 via email

@GrammAcc
Copy link
Contributor

@speakk Thanks for clarifying!

I normally use nvim as an external editor, so I actually didn't know that this feature existed. 😅
I was unable to reproduce this on my Fluxbox setup (also Arch):

screen-1_2024-03-20_18:02:16

It might be something specific to tiling window managers. There have been several other focus problems reported on tiling WMs (mainly i3). Some of which have been addressed in recent PRs. I don't think there are any dedicated issues for focus problems on tiling WMs open rn, but #74378 might be of interest.

I think a lot of different focus issues have become tangled up in this issue and #74378, so it's kind of hard to tell what has been fixed so far. :)

sambler pushed a commit to sambler/godot that referenced this issue Apr 6, 2024
…window manager on Linux

This is a workaround for the most critical portion of the WM focus bug
described in godotengine#68305. On some specific X11 WM configurations, the
editor's main window and any popups it creates will fight for focus,
which causes a total system lockup due to mouse and keyboard input being
stolen as well. Getting out of this infinite loop requires force
restarting the system.

It can be tested with the following shell script:

```bash
	!#/bin/sh

	godot4 &
	sleep 30
	pkill -x godot4
```

The workaround identified in godotengine#68305 is to remove the call to
XSetInputFocus in the ConfigureNotify event handler, so I have removed
the conditional block that calls this as well as the setup code above it
since there is no need to allocate the memory for the variables if they
won't be used in that call anymore.

This is just a hack and is not a complete fix for godotengine#68305. Multiple
developers are collaborating on a proper fix in the discussion in that
issue, but time is a valuable resource that no one has enough of, so I
am committing this workaround as a stop-gap to prevent the most critical
problem while we work on a full solution for the underlying cause.
@GrammAcc
Copy link
Contributor

I realized that because users still have to build from source if they were using my fork, there isn't any point in keeping it around until the binaries are released, so I went ahead and deleted it. If anyone was still using that fork, you should be fine to simply build from master now. :)

@nargacu83
Copy link
Author

I realized that because users still have to build from source if they were using my fork, there isn't any point in keeping it around until the binaries are released, so I went ahead and deleted it. If anyone was still using that fork, you should be fine to simply build from master now. :)

They can use the 4.3-dev5, the partial fix is apparently included in it. ;)

@hsandt
Copy link
Contributor

hsandt commented May 8, 2024

With current v4.3 dev5 the focus issue still happens with the find dialog on linux

Note that the Find in Files lose focus bug is a specific regression between v4.2.1 and v4.2.2 which has already been bisected, see #88847

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests