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 window_get_current_screen for X11 display server #42117

Merged
merged 1 commit into from
Oct 28, 2021

Conversation

pouleyKetchoupp
Copy link
Contributor

This method used to check which screen contains the top-left corner of the window (and default to the first screen in case none is found), which is not accurate in some cases.

Now the area of overlap with each screen is calculated, so we can get the best candidate based on the window's position.

This makes window_get_current_screen consistent with Windows platform, and fixes an issue where popups appear on the main screen when the main window is slightly moved outside of the desktop on the top or left.

@Calinou
Copy link
Member

Calinou commented Mar 20, 2021

How does this PR compare to the older #37506?

@pouleyKetchoupp
Copy link
Contributor Author

Both are potentially valid changes.

This PR just fixes the way the screen is selected for a given window position on Linux, to make it more reliable, and consistent with Windows platform. It might supersede the X11 part of #37506 but I'm not 100% sure.

#37506 does more general improvements on multi-screen configurations for all platforms. It was done before some heavy changes in the display servers and popup windows, so it would need to be checked again to make sure the way it's implemented still makes sense.

@pouleyKetchoupp pouleyKetchoupp requested a review from a team March 22, 2021 16:19
@pouleyKetchoupp pouleyKetchoupp force-pushed the x11-get-window-screen branch 2 times, most recently from 044fa95 to 1dfc434 Compare March 22, 2021 16:43
@pouleyKetchoupp
Copy link
Contributor Author

Rebased.

@akien-mga
Copy link
Member

Is this still relevant for current master? Also, there seems to be partial overlap with changes in #41565. Both should likely be rebased and re-tested, and if we can't find an X11 expert to review them, maybe just merged and tested in 4.0 alpha to see how it fares?

@pouleyKetchoupp
Copy link
Contributor Author

@akien-mga Yes, I've just rebased and re-tested and it's still relevant on master. It fixes a bug where a popup appears on the wrong screen if the top-left corner of the window is outside of the screen. With this PR the result is identical between Linux and Windows.

I agree it could help to merge for the alpha so we have more time for user testing.

This method used to check which screen contains the top-left corner of
the window (and default to the first screen in case none is found),
which is not accurate in some cases.

Now the area of overlap with each screen is calculated, so we can get
the best candidate based on the window's position.

This makes window_get_current_screen consistent with Windows platform,
and fixes an issue where popups appear on the main screen when the main
window is slightly moved outside of the desktop on the top or left.
@akien-mga akien-mga merged commit 9fb9b99 into godotengine:master Oct 28, 2021
@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.

3 participants