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

Popup fixes for X11 display server #41456

Merged
merged 4 commits into from Sep 2, 2020

Conversation

pouleyKetchoupp
Copy link
Contributor

@pouleyKetchoupp pouleyKetchoupp commented Aug 22, 2020

This PR fixes some issues with popups showing with a delay on linux, and also does a few general changes in how Popup nodes are handling focus changes.

Also fixes #37674, fixes #41578

Re-apply "Fixes for windows in X11 tiling WMs"
From PR #38727 by @Riteo which was reverted in #41373 because of regressions in Ubuntu with Gnome.
Regressions are now fixed with the other popup changes in this PR.

Fix popup closed when an ancestor window is focused
This is a general change for Popup which fixes issues that affect all platforms.

Previously, only the direct parent were taken into account.

Popups like contextual menus could stay open if an ancestor which is not a direct parent was focused.

Reproduction steps (any platform):

  • Select a node in the scene tree
  • Left click the node to start renaming
  • Right click to open the copy/paste contextual menu
  • Left click in the scene tree to deselect the node

Also closing popup when focusing out of the application, without waiting for the parent to get focus to do so.

Fix menu popups delay and focus in X11 display server
This is a fix for a linux specific issue with popups. Contextual menus, top bar menus and editable properties were showing with a delay due to some extra time to be initialized in the windows manager.

Now using override_redirect for all utility popups to prevent the WM from interfering with them, so we have more control over focus management and avoid a delay before they show up.

Fix WINDOW_EVENT_FOCUS_IN for popups on Windows
This is a fix for Windows, needed because of the changes in Popup in this PR.
On Windows, WINDOW_EVENT_FOCUS_IN was never sent by the display server for popups, because WM_ACTIVATE events are received during the call to _update_window_style, which happened before the callbacks were set.

This was causing some issues with the way Popup is now handling closing on parent focus.

Now _update_window_style is only called during show_window, after Window initialized callbacks.

pouleyKetchoupp and others added 2 commits August 22, 2020 18:42
From PR godotengine#38727 which was reverted in godotengine#41373 because of regressions in Ubuntu
with Gnome.

Co-authored-by: Lorenzo Cerqua <lorenzocerqua@tutanota.com>
Previously, only the direct parent were taken into account.

Popups like contextual menus could stay open if an ancestor which is
not a direct parent was focused.

Reproduction steps (any platform):
- Select a node in the scene tree
- Left click the node to start renaming
- Right click to open the copy/paste contextual menu
- Left click in the scene tree to deselect the node

Also closing popup when focusing out of the application, without waiting
for the parent to get focus to do so.
@pouleyKetchoupp
Copy link
Contributor Author

Tested on Ubuntu (Gnome & KDE) so far.

If anybody would like to help, I'm interested in feedback with different WMs to make sure these changes don't introduce any critical regression.

Now using override_redirect for menu & tooltip popups to prevent the WM from
interfering with them, so we have more control over focus management
and avoid a delay before they show up.
On Windows, WINDOW_EVENT_FOCUS_IN was never sent by the display server
for popups, because WM_ACTIVATE events are received during the call to
_update_window_style, which happened before the callbacks were set.

This was causing some issues with the way Popup is now handling closing on
parent focus.

Now _update_window_style is only called during show_window, after Window
initialized callbacks.
@pouleyKetchoupp
Copy link
Contributor Author

Pushed some modifications in X11 display server to solve some focus issues with XFCE, along with code cleaning and extra comments.

@pouleyKetchoupp
Copy link
Contributor Author

Extra tests with a tiling WM:
Tested with i3, popups are still on the right position and size (fix from #38727) and focus works correctly.
With i3 it also freezes for a few seconds for me when any window opens, or when switching the focus from and to the editor. I'm not sure what is causing it, but it seems unrelated to this PR since I experience the same thing on latest master.

@Riteo
Copy link
Contributor

Riteo commented Aug 26, 2020

I can confirm freezing on i3 too.

@pouleyKetchoupp
Copy link
Contributor Author

@Riteo Thanks for checking. Were you experiencing it before? I can reproduce it on latest master, but I'm not sure if it's a recent issue.

@Riteo
Copy link
Contributor

Riteo commented Aug 26, 2020

Why not bisecting this issue? Soon I should be able to do that.

@Riteo
Copy link
Contributor

Riteo commented Aug 27, 2020

After a lot of tests, I couldn't pinpoint the exact reason of the freeze.
Bisecting doesn't seem to work, as even commit d670a49(from PR #38727, which I used a lot) freezes now.

As of now I tried on Arch Linux with i3 running a build of commit d670a49 to:

  • Turn on the debugging log on i3, which gave some strange output(even though I could not find a pattern behind it at first glance) on which I'll probably investigate later on.
    But it worked before and thus, since the only big factor that I could think of would be a system update, something must have changed somewhere else(Maybe some unwanted behaviour change in some X related package after an update?).
  • Roll back i3-wm to 4.18.1-1, xorg-server-* to 1.20.8-2 and libx11 to 1.6.9-7, which are the oldest versions of those packages which i could find in my pacman cache.
  • Compile Godot with both target debug and release_debug

Other things to try would be running Godot on other tiling WMs, rolling back any other possibly related package and maybe debugging any weird output on i3's debug log(thing which I'll try to do as soon as I can, but lately I'm getting very slowed down due to personal reasons).

Still, I'm reading a lot of X11 documentation and questioning some choices done in the X11 DisplayServer implementation, thing which I would like to discuss eventually on IRC with @reduz and whoever worked on it(I really don't want to sound rude, there is surely some technical reason behind those choices and I would like to know why some things are made in a certain way in order to contribute with code that doesn't go against them).

It's 4 AM here and I'm very tired so I think that I'll stop for now.

@pouleyKetchoupp
Copy link
Contributor Author

Thanks for all the info.

I'm currently contracting to work on the X11 display server, so I'm responsible for this topic. We can talk about your concerns directly, either here or on IRC.

About the freezing issues with i3, I've added it to my list but it's on a lower priority for me since the goal is to first make the display server work correctly with standard WMs.

@Meriipu
Copy link
Contributor

Meriipu commented Aug 27, 2020

#40024 this is an issue on ctwm (and probably twm too), although it has not been introduced by this PR. It may still be related since it was introduced with the 4.0 changes to popups.

Godot either

  • does not properly make exclusive windows always on top, impossible to lower, and the parent uninteractible (like thunar does with Help>About)
  • annoyingly raises the child window a few split seconds after the parent has been raised, instead of not doing anything to change window stacking beyond possibly disallowing input to the parent while the child is open.

@Riteo
Copy link
Contributor

Riteo commented Aug 28, 2020

I think we should make a separate issue for this freezing, as it doesn't seem related to this PR.
I'm still investigating about it though, with some very interesting results.

I'm currently contracting to work on the X11 display server, so I'm responsible for this topic. We can talk about your concerns directly, either here or on IRC.

@pouleyKetchoupp I would like to not go too much off topic on this PR page, but this weekend i should be a little more free, maybe we could talk about it then on IRC?

It may still be related since it was introduced with the 4.0 changes to popups.

@Meriipu I don't think so, since this freezing issue started arising only very recently, at least only on i3.

@pouleyKetchoupp
Copy link
Contributor Author

@Riteo I've just opened a separate issue for the freezing in i3.
Unfortunately, I personally won't be available on IRC this weekend but I'll be available the next one or anytime during the week. You can still talk about it with other devs if you want to of course and I can check the logs later.

@duane-r
Copy link

duane-r commented Sep 2, 2020

This seems to work perfectly on openbox (archlinux).

@akien-mga akien-mga merged commit 2a8531c into godotengine:master Sep 2, 2020
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

Editor Menus and Text Entry Boxes Appear at Origin Tooltip renders under window
5 participants