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

Redirect mouse events to the window under cursor, simplify popup menu input processing. #88392

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bruvzg
Copy link
Member

@bruvzg bruvzg commented Feb 16, 2024

  • Redirect all mouse events to the window under the cursor.
  • Simplify PopupMenu input handling.

Fixes #88324
Fixes #70361
Fixes #77246
Fixes #66273
Fixes #73926

Supersede #85583
Supersede #88366
Supersede #88342

@bruvzg
Copy link
Member Author

bruvzg commented Feb 16, 2024

Seems to be working on macOS (and sidecar on iOS), Windows, X11/GNOME, and in a single-window mode. Confined/Captured mode seems to work fine. As well as on Android and iOS (with touch screen instead of mouse).

@bruvzg bruvzg marked this pull request as ready for review February 16, 2024 17:02
@bruvzg bruvzg requested review from a team as code owners February 16, 2024 17:02
@akien-mga akien-mga requested a review from a team February 16, 2024 18:54
@bruvzg
Copy link
Member Author

bruvzg commented Feb 16, 2024

Actually, there is an extra case that probably require additional handling, and I have not accounted for: WINDOW_FLAG_MOUSE_PASSTHROUGH (not sure if it's used for anything in editor, but should pass event to the underlying window if set).

@bruvzg
Copy link
Member Author

bruvzg commented Feb 17, 2024

Actually, there is an extra case that probably require additional handling, and I have not accounted for: WINDOW_FLAG_MOUSE_PASSTHROUGH

Added extra check for X11, should be already handled on other platforms.

@akien-mga
Copy link
Member

Seems to work fine for me on KDE Wayland.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

Tested on Windows and it works great.

Can't comment much on the code. Though I wonder if calling get_window_at_screen_position() for every mouse event won't affect performance.

Copy link
Contributor

@Sauermann Sauermann left a comment

Choose a reason for hiding this comment

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

This PR is unifying the behavior of different display servers, which is a step in the right direction.

I have tested this PR rebased on 8f0c20e on Linux X11 Xfce in two scenarios.

The root window is not embedding windows (single window mode off)

I can confirm, that it fixes: #88324, #77246 and #66273
I was unable to recreate #73926 (not on platform:Linux) and #70361 (interference from #88324) in current master.

Currently (8f0c20e) it is possible to drag and drop from a secondary window to the root window. With this PR, this functionality is no longer possible. This can be verified in the editor by making the FileSystem dock floating and trying to drag and drop the icon.svg into the respective slot in the Inspector of a Sprite2D node.

I have tested this PR rebased on #67531 and found, that merging this PR will make changes to the other PR necessary. Don't merge at the same time!

The root window is embedding windows (single window mode on)

I can confirm, that it fixes: #66273
The PR doesn't resolve issue #70361 and #77246 for me.
I was unable to recreate #73926 (not on platform:Linux) and #88324 (native windows only) in current master.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 18, 2024

@Sauermann Are you using Windows 10 or 11? I'm not able to reproduce #88324 on Windows 10, but I can confirm the PR fixes #73926 and #70361 for me.

@Sauermann
Copy link
Contributor

@KoBeWi Please note, that I have tested this on Linux.

@KoBeWi
Copy link
Member

KoBeWi commented Feb 18, 2024

Ah ok, I misunderstood your comment.

@DanielSnd
Copy link
Contributor

@Sauermann Are you using Windows 10 or 11? I'm not able to reproduce #88324 on Windows 10, but I can confirm the PR fixes #73926 and #70361 for me.

I was able to reproduce #88324 on windows 11 and this PR has solved the issue for me.

@akien-mga
Copy link
Member

Did some testing on Linux, I think I mostly have the same results as @Sauermann. I wrote them in each relevant issue to keep things a bit clearer about what is reproducible in master and whether this PR improves it.

I confirm this PR fixes #88324 and #66273.

For me #70361 is only reproducible in single-window mode, and this PR doesn't fix it on Linux. Does it fix it on Windows for single-window mode? If not, we might want to keep the issue open for single-window mode, or open a new one.

Likewise for #77246, it seems specific to single-window mode on Linux, and this PR doesn't fix it.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

I think this is good to go, but we might need to clarify whether all the listed bugs should indeed be closed or not, as some aspects of them are still reproducible at least on Linux + single-window mode.

@bruvzg
Copy link
Member Author

bruvzg commented Feb 19, 2024

#70361 and #77246 are reproducible (and fixed) in multi window mode on Windows, not reproducible on macOS, and I have not tested single-window mode.

@Maran23
Copy link
Contributor

Maran23 commented Feb 19, 2024

I can also confirm that the Popup behaviour is much better with this PR.
Although it still does not feel perfect yet, e.g. when moving the mouse a lot, the highlighting may not be visible. Still an improvement.

What I'm wondering here: When I debugged this issue on Windows, I saw weird or even wrong behaviour what _get_focused_window_or_popup returned, thats why my PR fixed the issue by not calling it (#85583). Windows did already send the event with the correct window id (hwnd), why do we need to 'calculate' a receiving window and use it instead?

@Inve1951
Copy link

Don't miss @Sauermann's comment:

Currently (8f0c20e) it is possible to drag and drop from a secondary window to the root window. With this PR, this functionality is no longer possible. This can be verified in the editor by making the FileSystem dock floating and trying to drag and drop the icon.svg into the respective slot in the Inspector of a Sprite2D node.

That's a regerssion.

@EmrysMyrddin
Copy link
Contributor

EmrysMyrddin commented Feb 19, 2024

This does not actually fixes #88324 but it helps working around it.

The root cause of the issue is still there, but this reduces the chances to trigger the bug. See #88324 (comment)

@kitbdev
Copy link
Contributor

kitbdev commented Feb 20, 2024

On Windows 10.0.22621.
I can confirm that this PR fixes #70361, #77246, #73926, and #66273 in multiwindow mode.
#73926 and #66273 are only reproducible on multiwindow mode, so these can be closed.
#70361 and #77246 are present on single window mode still. I believe this is related to #84466 and Viewport's mouse focus, and can be fixed separately.

This breaks drag and drop between multiple windows for me as well.

The root cause of the issue is still there, but this reduces the chances to trigger the bug. See #88324 (comment)

I cannot reproduce #88324, but I believe that is a separate timing issue, since this PR changes popups to need mouse up.

@WhalesState
Copy link
Contributor

WhalesState commented Feb 20, 2024

I Just have some concerns related to the changes in the popup menu class.
Your changes to the display server is a replacement for this pull request #86952 which have caused a lot of issues to the popup menu recently, so we can just revert it and we merge your pull request without the changes to the popup menu files to redirect all mouse events to the window under the cursor, which was the correct fix to the first issue instead of the other hack to enable a missing feature. I also have made the popup menu updates the selection when the mouse is pressed, to make it work fine with touch input if we enable emulate mouse from touch in project settings. here's the changes in my pull request #87462 with the old behavior and the only missing fix is to redirect the mouse release event to the popup menu window under the cursor when it opens to allow opening the popup and triggering an action in a single mouse click for non-embedded subwindows. Thank you.

@WhalesState
Copy link
Contributor

I think it will be easier to backup your popup menu changes and you submit them later on another pull request after merging my pull request to avoid rebasing and conflict. Since most of the popup menu issues have been fixed after reverting and more issues was fixed in my pull request.

@kitbdev
Copy link
Contributor

kitbdev commented May 13, 2024

@akien-mga akien-mga modified the milestones: 4.3, 4.4 Jul 9, 2024
@Anutrix
Copy link
Contributor

Anutrix commented Sep 24, 2024

Needs a rebase since #67531 got merged.

akien-mga added a commit to akien-mga/godot that referenced this pull request Nov 6, 2024
Redirect mouse events to the window under cursor, simplify popup menu input processing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment