Fixes for windows in X11 tiling WMs #38727

Riteo · 2020-05-13T18:19:50Z

This PR aims to fix various window issues on the platform "linuxbsd" regarding titling WMs.
All popups seem to work intermittently, behaviour which i believe is caused by the current implementation of _create_window()

The current way of creating windows works by mapping them before changing their properties.
This approach works on floating window based WMs but not on tiling ones, which as soon as they are mapped get resized to full screen and get cleared of most of their properties, as shown in this part of an i3 debug log:

09/05/2020 03:01:22 - WM_CLASS changed to Godot_Engine (instance), [PROJECT_NAME] (class)
09/05/2020 03:01:22 - WM_NAME changed to "Godot"
09/05/2020 03:01:22 - Using legacy window title. Note that in order to get Unicode window titles in i3, the application has to set _NET_WM_NAME (UTF-8)
09/05/2020 03:01:22 - window.c:window_update_name:64 - _NET_WM_NAME not specified, not changing
09/05/2020 03:01:22 - window.c:window_update_leader:139 - CLIENT_LEADER not set on window 0x05a00053.
09/05/2020 03:01:22 - window.c:window_update_transient_for:164 - TRANSIENT_FOR not set on window 0x05a00053.
09/05/2020 03:01:22 - window.c:window_update_strut_partial:189 - _NET_WM_STRUT_PARTIAL not set.
09/05/2020 03:01:22 - window.c:window_update_role:214 - WM_WINDOW_ROLE not set.
09/05/2020 03:01:22 - window.c:window_update_hints:377 - WM_HINTS not set.
09/05/2020 03:01:22 - window.c:window_update_normal_hints:291 - Clearing maximum size
09/05/2020 03:01:22 - window.c:window_update_normal_hints:312 - Clearing size increments
09/05/2020 03:01:22 - window.c:window_update_normal_hints:326 - Clearing base size
09/05/2020 03:01:22 - window.c:window_update_normal_hints:335 - Setting geometry x=1616 y=87 w=294 h=218
09/05/2020 03:01:22 - window.c:window_update_normal_hints:359 - Clearing aspect ratios

This whole issue is all pretty messy as it seems to happen in two different processes at the same time, so the results vary a little sometimes, letting the window work by sheer luck.
By mapping the window later, we avoid all of this.

Almost everything works, but there is some stuff left to do, so i'm making this a draft PR. There are also some other issues, but they seem related to X11 in general, such as focus(#37531) and offset(#38591) related ones.

Edit: made the issue clearer

akien-mga · 2020-05-13T18:56:51Z

Note that one of your commit includes modules/stXI98cE which is a temporary binary from the compiler, it should be removed by amending the commit.

Riteo · 2020-05-13T20:31:19Z

Oh, sorry, gonna fix it immediately!

Riteo · 2020-05-13T20:51:55Z

I tried removing it, only changed the latest commit, i'm going to see how to remove it later.
Edit: Wait, I did it, lol, it worked and I didn't notice it, I just needed to change the last commit. For some reason I thought you meant something like "change it in all commits".

groud · 2020-05-14T07:59:45Z

Oh, nice !
I am eager to see the result, as my awesomeWM doesn't like Godot that much for now. :D

Riteo · 2020-05-14T14:34:33Z

I'm trying to keep this PR as minimal as possible, but there is a little change in the generic DisplayServer implementation by adding show_window(WindowID p_id) to it, and requiring first to create a window, then setting its properties, and only then showing it. This also changes other classes creating a window, but for now only 2 were changed.

All platforms seem to have a "show window" function, but if one doesn't have it and directly shows one on creation, probably setting a window's properties after showing will work. In that case an empty implementation of show_window(WindowID p_id)should work. For now, its default implementation throws an error regarding missing window support.

EIREXE · 2020-05-15T05:28:04Z

Is this also related to godot not getting the proper window ID for the game until it's started?

Riteo · 2020-05-15T13:06:20Z

I don't think so, but i may be not fully get what you're referring to. This PR fixes some issues with screen sized popups on tiling window managers

Riteo · 2020-07-09T02:31:20Z

After some time, as I got the opportunity to work on this again, a lot of issues seem to have been fixed in one form or the other, leaving this PR usable, I tried it a bit and seems to work quite well. Still, without this PR, there are some tiling WM specific issues, so I just got to implement the other platforms and I think that this PR will be ready for review!

Riteo · 2020-07-22T02:46:16Z

Ok, so now i should have implemented show_window on Windows, but there's a big issue: MacOS.
Currently I have no way of compiling nor testing for this platform. Should I add a dummy implementation of show_window(that should still work) and set this PR as ready for review?

Riteo · 2020-07-22T04:46:41Z

Oh well, after some testing i found some other issues, i'm aware of them and i'm working on it.

bruvzg · 2020-07-22T05:06:51Z

but there's a big issue: MacOS.

Relevant core for macOS:

Split this part to the show_window function:

godot/platform/osx/display_server_osx.mm

Lines 2320 to 2324 in a5fb445

    
           if (wd.no_focus) { 
        
           	[wd.window_object orderFront:nil]; 
        
           } else { 
        
           	[wd.window_object makeKeyAndOrderFront:nil]; 
        
           }

and replace this line with show_window(MAIN_WINDOW_ID, 0);:

godot/platform/osx/display_server_osx.mm

Line 3758 in a5fb445

[windows[main_window].window_object makeKeyAndOrderFront:nil];

Riteo · 2020-07-22T15:48:18Z

Thanks! I'll implement 'show_window' for MacOS with your changes ASAP!
Btw, the previously mentioned issues were caused by some mistake I made during my testing(I should really stop doing this stuff at 5 AM). Still, I'll keep an eye open for any issue.

bruvzg · 2020-07-22T16:41:14Z

platform/osx/display_server_osx.mm

+	return id;
+}
+
+void DisplayServerOSX::show_window(const WindowData &wd, uint32_t p_flags) {


It's ... show_window(WindowID p_id ... on other platforms, probably should be the same here. And definition is missing from display_server_osx.h.

void DisplayServerOSX::show_window(WindowID p_id, uint32_t p_flags) { WindowData &wd = windows[p_id]; ....

oh yeah sorry, i messed up, i was distracted and wrote without thinking too much. I should be more careful while writing code i can't compile eheh. I'll fix it immediately!

Riteo · 2020-07-22T17:06:49Z

It doesn't want to compile? This is confusing.
The latest check is giving an error on a line that i removed. What's going on?
For now i think i got a way of removing that 'p_flags' argument. I'll do that before moving on on understading why that happens.

bruvzg · 2020-07-22T17:29:09Z

What's going on?

It's probably still running previous build, not sure how GitHub Actions handle it, but Travis only cancels old build if it's not started.

Riteo · 2020-07-22T17:33:21Z

Well, my new commit(that i'm testing before pushing) will surely freshen it up.

Riteo · 2020-07-22T17:58:39Z

What? Github actions still believes that there's that line! Does it really not compile on MacOS or is actions messed up?

bruvzg · 2020-07-22T18:24:24Z

platform/osx/display_server_osx.h

@@ -230,6 +230,7 @@ class DisplayServerOSX : public DisplayServer {
 	virtual Vector<int> get_window_list() const override;

 	virtual WindowID create_sub_window(WindowMode p_mode, uint32_t p_flags, const Rect2i &p_rect = Rect2i()) override;
+	virtual void show_window(WindowID p_id, uint32_t p_flags) override;


You have not removed p_flags from macOS version.

oooh, that's why! Very very sorry, i was convinced that the issue was still that line!

Fixing it right now!

bruvzg · 2020-07-22T18:26:47Z

What? Github actions still believes that there's that line! Does it really not compile on MacOS or is actions messed up?

Try squashing commits (it's necessary to do it before merging anyway), see: https://docs.godotengine.org/en/stable/community/contributing/pr_workflow.html#modifying-a-pull-request.

Riteo · 2020-07-22T19:42:28Z

Ok, I hope to have squashed the pr correctly. I tried it on linux and a bit on wine(yeah, it isn't the most accurate way of testing it, but it works), but I have no way of testing it on MacOS. Still, the PR should be done. Setting it as ready for review!

When creating a window, Godot would first register it to the WM(show it) and then set its flags. This works fine on a floating WM, but on tiling WMs as soon as a window gets registered the WM immediately acts on the window by scaling it up and treating it as a generic window, being registered without any special flags. This commit separates the showing of the window into another function and calls it after the most important flags are set, making windows with special flags(eg. all popups) work again on tiling WMs. Fixes godotengine#37930

akien-mga · 2020-07-23T08:07:41Z

CC @vnen @groud - since you had issues with tiling WMs, could either of you test this PR and confirm the fix?

groud · 2020-07-23T16:54:42Z

Ok I've just tested the PR. Generally, this works a lot better than before, the popups and subwindows work great.

The only problem I found is that, when I disable the tiling (awesomewm allows me to do so), the popup do not pop at the correct position anymore. I figured out that this position was the one chosen by my windows manager to open a new window.
So basically it's like the popup was considered as a normal window and not moved at the correct position anymore. See the screenshot:

Edit: Note that all other subwindows are popped there too, not only popups.

When tiling is activated it works correctly though.

Riteo · 2020-07-23T18:54:13Z

Do you have multiple screens? It looks a lot like the stuff that #37506 is trying to fix.
The fact that it works in tiling mode confuses me though. I'll investigate further.

groud · 2020-07-23T19:51:15Z

Do you have multiple screens? It looks a lot like the stuff that #37506 is trying to fix.

No I have only one screen (at least active).

We have to make sure this PR does not break the situation for any non-tiling WM though. If someone could test that :)

Riteo · 2020-07-24T01:52:28Z

Ok I've just tested the PR. Generally, this works a lot better than before, the popups and subwindows work great.

The only problem I found is that, when I disable the tiling (awesomewm allows me to do so), the popup do not pop at the correct position anymore. I figured out that this position was the one chosen by my windows manager to open a new window.
So basically it's like the popup was considered as a normal window and not moved at the correct position anymore. See the screenshot:

Edit: Note that all other subwindows are popped there too, not only popups.

When tiling is activated it works correctly though.

I did a quick test before going to bed, and I can't reproduce this issue on i3.

groud · 2020-07-26T13:19:31Z

I did a quick test before going to bed, and I can't reproduce this issue on i3.

Ok. In any case this drastically improves the situation anywayu. It would be good if someone could test the PR on a non-tiling WM though, but on my side it looks good to merge.

akien-mga · 2020-07-26T15:41:23Z

Tested on KDE Plasma / KWin with e7a56a2 + this PR, it seems to work fine.

I get an error when I select a node in the Create New Node dialog, but it seems unrelated to this PR (happens without it too):

ERROR: Condition "!windows.has(p_window)" is true. Returning: -1
   at: window_get_current_screen (platform/linuxbsd/display_server_x11.cpp:807)

akien-mga · 2020-07-26T15:41:32Z

Thanks!

opl- · 2020-07-31T04:39:21Z

Can confirm that this PR greatly improved usability on KDE, making both context menus and dialogs render much quicker and without visual artifacts that were visible before (the windows were displaying pixels that were below them when they were created until the first render). Thanks and good job!

It did however reveal some issues in the X11 display server implementation that makes all the context menus and popups (probably others too) appear in the top left corner of the screen or around the main window, regardless of the requested position. I believe I have found the cause of this tonight. Will try to implement that fix and PR tomorrow. Edit: Done, see #40922.

reduz · 2020-08-19T15:33:58Z

This breaks popups on Stock Ubuntu (Gnome), I suggest reverting until we figure out whats going on, then try the PR again.

reduz · 2020-08-19T15:37:21Z

reduz · 2020-08-19T15:40:23Z

Please test in stock Ubuntu, when it works attempt the PR again.

reduz · 2020-08-19T15:46:14Z

Another problem with this PR is that simple popups (For value editors) do not get focus instantly like they normally did. Please check this too.

pouleyKetchoupp · 2020-08-19T16:23:38Z

I found out that with these changes, XMapWindow is not called anymore when creating sub windows, which might be part of the problem with popups on Ubuntu.

But just adding show_window after _create_window for sub windows doesn't fix the issue, so there's something else going wrong with the order of calls.

pouleyKetchoupp · 2020-08-20T14:48:34Z

@Riteo After some more research, it seems XMapWindow is actually not needed for sub windows, but it requires some extra fixes to work correctly on Ubuntu with Gnome. So I think this PR goes in the right direction, and I'm going to include it along with a new PR for some general fixes for popups I'm working on (I'll co-author you). When it's ready we'll make sure tiling WMs are still working with my extra changes :)

Riteo · 2020-08-22T05:17:30Z

Nice! I also got some new ideas regarding both the DisplayServer API and its X11 implementation. As of now i'm on my summer pause, but on monday I'll come back, ready to contribute to the fix to this issue and Godot in general.

pouleyKetchoupp · 2020-08-22T07:54:23Z

Sounds good! I've made a lot of progress so far but I'm still working on some regressions in specific cases due to my changes. The general idea is to use override_redirect for the popups so the WM doesn't interfere with them, and it has some side effects on how the focus switches between windows. I'll make sure to open a PR at least as a draft later today so you can have a look when you want to jump in.

From PR godotengine#38727 which was reverted in godotengine#41373 because of regressions in Ubuntu with Gnome. Co-authored-by: Lorenzo Cerqua <[email protected]>

Riteo force-pushed the tiling-wm-issues-tests branch from 3a9d67c to 8eae390 Compare May 13, 2020 20:42

Riteo force-pushed the tiling-wm-issues-tests branch from 8eae390 to fbd7caf Compare May 14, 2020 01:04

Calinou added bug platform:linuxbsd topic:editor topic:porting labels May 15, 2020

Riteo mentioned this pull request Jul 22, 2020

[TRACKER] DisplayServer, Window & UI issues #37734

Closed

60 tasks

bruvzg reviewed Jul 22, 2020

View reviewed changes

Riteo force-pushed the tiling-wm-issues-tests branch 3 times, most recently from 3c8f758 to 70df0e7 Compare July 22, 2020 19:36

Riteo force-pushed the tiling-wm-issues-tests branch from 70df0e7 to 2242c0d Compare July 22, 2020 19:45

Riteo force-pushed the tiling-wm-issues-tests branch from 2242c0d to d670a49 Compare July 23, 2020 06:04

akien-mga added this to the 4.0 milestone Jul 23, 2020

akien-mga merged commit 3842e8c into godotengine:master Jul 26, 2020

reduz mentioned this pull request Aug 19, 2020

Revert "Fixes for windows in X11 tiling WMs" #41373

Merged

pouleyKetchoupp mentioned this pull request Aug 22, 2020

Popup fixes for X11 display server #41456

Merged

Riteo deleted the tiling-wm-issues-tests branch October 12, 2020 03:20

rsubtil mentioned this pull request Nov 29, 2021

X11 DisplayServer does not set correct _NET_WM_WINDOW_TYPE for dialogs, causing issues with tiling window managers (DisplayServer regression) #55415

Open

Fixes for windows in X11 tiling WMs #38727

Fixes for windows in X11 tiling WMs #38727

Conversation

Riteo commented May 13, 2020 • edited Loading

akien-mga commented May 13, 2020

Riteo commented May 13, 2020

Riteo commented May 13, 2020 • edited Loading

groud commented May 14, 2020

Riteo commented May 14, 2020

EIREXE commented May 15, 2020

Riteo commented May 15, 2020

Riteo commented Jul 9, 2020

Riteo commented Jul 22, 2020

Riteo commented Jul 22, 2020

bruvzg commented Jul 22, 2020

Riteo commented Jul 22, 2020

bruvzg Jul 22, 2020

Choose a reason for hiding this comment

Riteo Jul 22, 2020

Choose a reason for hiding this comment

Riteo commented Jul 22, 2020 • edited Loading

bruvzg commented Jul 22, 2020

Riteo commented Jul 22, 2020

Riteo commented Jul 22, 2020

bruvzg Jul 22, 2020

Choose a reason for hiding this comment

Riteo Jul 22, 2020

Choose a reason for hiding this comment

Riteo Jul 22, 2020

Choose a reason for hiding this comment

bruvzg commented Jul 22, 2020

Riteo commented Jul 22, 2020

akien-mga commented Jul 23, 2020

groud commented Jul 23, 2020 • edited Loading

Riteo commented Jul 23, 2020

groud commented Jul 23, 2020

Riteo commented Jul 24, 2020

groud commented Jul 26, 2020

akien-mga commented Jul 26, 2020

akien-mga commented Jul 26, 2020

opl- commented Jul 31, 2020 • edited Loading

reduz commented Aug 19, 2020

reduz commented Aug 19, 2020

reduz commented Aug 19, 2020

reduz commented Aug 19, 2020

pouleyKetchoupp commented Aug 19, 2020

pouleyKetchoupp commented Aug 20, 2020 • edited Loading

Riteo commented Aug 22, 2020

pouleyKetchoupp commented Aug 22, 2020

Riteo commented May 13, 2020 •

edited

Loading

Riteo commented May 13, 2020 •

edited

Loading

Riteo commented Jul 22, 2020 •

edited

Loading

groud commented Jul 23, 2020 •

edited

Loading

opl- commented Jul 31, 2020 •

edited

Loading

pouleyKetchoupp commented Aug 20, 2020 •

edited

Loading