[PATCH v3 0/4] MR1153: winex11.drv: Correctly handle clipping for fullscreen windows.
When we are clipping (due to fullscreen), certain operations are bugged (such as creating another window on top, grabbing the window during a size-move operation, changing mouse capture, etc) so we must temporarily unclip first. Seems unclipping it temporarily during such operation is enough to fix this. This probably wasn't very obvious in practice because most times "fullscreen" means a single non-resizeable window with no menus and so on (e.g. fullscreen games), but that's not always the case. -- v3: win32u: Don't clip while capturing mouse pointer. winex11: Ungrab clipping window when capturing mouse pointer. winex11: Don't clip while doing move/resize dragging. winex11: Don't clip while creating X Window. https://gitlab.winehq.org/wine/wine/-/merge_requests/1153
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53831 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/winex11.drv/window.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index 974bd376fe6..f24630c57f5 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1613,6 +1613,7 @@ static void create_whole_window( struct x11drv_win_data *data ) WCHAR text[1024]; COLORREF key; BYTE alpha; + BOOL was_clipped = FALSE; DWORD layered_flags; HRGN win_rgn; POINT pos; @@ -1641,10 +1642,21 @@ static void create_whole_window( struct x11drv_win_data *data ) if (!(cy = data->whole_rect.bottom - data->whole_rect.top)) cy = 1; else if (cy > 65535) cy = 65535; + /* unclip it while we create the Window, see Wine-Bug 53831 */ + if (x11drv_thread_data() && x11drv_thread_data()->clipping_cursor) + { + was_clipped = TRUE; + ungrab_clipping_window(); + } pos = virtual_screen_to_root( data->whole_rect.left, data->whole_rect.top ); data->whole_window = XCreateWindow( data->display, root_window, pos.x, pos.y, cx, cy, 0, data->vis.depth, InputOutput, data->vis.visual, mask, &attr ); + if (was_clipped) + { + clipping_cursor = TRUE; + retry_grab_clipping_window(); + } if (!data->whole_window) goto done; set_initial_wm_hints( data->display, data->whole_window ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1153
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53831 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/winex11.drv/mouse.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/dlls/winex11.drv/mouse.c b/dlls/winex11.drv/mouse.c index f2375a807b2..494aec81c30 100644 --- a/dlls/winex11.drv/mouse.c +++ b/dlls/winex11.drv/mouse.c @@ -1442,6 +1442,7 @@ void move_resize_window( HWND hwnd, int dir ) POINT pos; int button = 0; XEvent xev; + BOOL was_clipped = FALSE; Window win, root, child; unsigned int xstate; @@ -1471,7 +1472,15 @@ void move_resize_window( HWND hwnd, int dir ) /* need to ungrab the pointer that may have been automatically grabbed * with a ButtonPress event */ - XUngrabPointer( display, CurrentTime ); + if (!clipping_cursor) + XUngrabPointer( display, CurrentTime ); + + /* unclip it during drag, see Wine-Bug 53831 */ + if (x11drv_thread_data() && x11drv_thread_data()->clipping_cursor) + { + was_clipped = TRUE; + ungrab_clipping_window(); + } XSendEvent(display, root_window, False, SubstructureNotifyMask | SubstructureRedirectMask, &xev); /* try to detect the end of the size/move by polling for the mouse button to be released */ @@ -1514,6 +1523,7 @@ void move_resize_window( HWND hwnd, int dir ) if (!(xstate & (Button1Mask << (button - 1)))) break; NtUserMsgWaitForMultipleObjectsEx( 0, NULL, 100, QS_ALLINPUT, 0 ); } + if (was_clipped) grab_clipping_window( &clip_rect ); TRACE( "hwnd %p/%lx done\n", hwnd, win ); send_message( hwnd, WM_EXITSIZEMOVE, 0, 0 ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1153
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53831 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/winex11.drv/window.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index f24630c57f5..f3c1c12c472 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -2456,6 +2456,8 @@ void X11DRV_SetCapture( HWND hwnd, UINT flags ) if (!(data = get_win_data( NtUserGetAncestor( hwnd, GA_ROOT )))) return; if (data->whole_window) { + if (data->managed && thread_data->clipping_cursor) + ungrab_clipping_window(); XFlush( gdi_display ); XGrabPointer( data->display, data->whole_window, False, PointerMotionMask | ButtonPressMask | ButtonReleaseMask, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1153
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> And revert back to fullscreen clipping when capture is done, if necessary. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=53831 Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/win32u/input.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dlls/win32u/input.c b/dlls/win32u/input.c index 205b792d784..6365b10f15f 100644 --- a/dlls/win32u/input.c +++ b/dlls/win32u/input.c @@ -1748,6 +1748,9 @@ BOOL set_capture_window( HWND hwnd, UINT gui_flags, HWND *prev_ret ) { user_driver->pSetCapture( hwnd, gui_flags ); + if (!hwnd) + clip_fullscreen_window( NtUserGetForegroundWindow(), FALSE ); + if (previous) send_message( previous, WM_CAPTURECHANGED, 0, (LPARAM)hwnd ); @@ -2485,6 +2488,7 @@ BOOL clip_fullscreen_window( HWND hwnd, BOOL reset ) if (!NtUserGetWindowRect( hwnd, &rect )) return FALSE; if (!NtUserIsWindowRectFullScreen( &rect )) return FALSE; + if (get_capture()) return FALSE; if (NtGetTickCount() - thread_info->clipping_reset < 1000) return FALSE; if (!reset && clipping_cursor && thread_info->clipping_cursor) return FALSE; /* already clipping */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1153
I've rebased it with after clipping changes, and split it into 4 different patches now. They all solve parts of the same bug though. As a recap, the problem lies when we're clipping fullscreen windows and then doing window operations like creating a new X Window, move/resize dragging, and capturing mouse pointer. It's likely not a very common situation since most times "fullscreen windows" means games or video players or the like, which have only one window, but the bug report should have details that it can happen with normal window creations or menus or whatever, and affects at least one app. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1153#note_35898
Rémi Bernon (@rbernon) commented about dlls/winex11.drv/window.c:
+ /* unclip it while we create the Window, see Wine-Bug 53831 */ + if (x11drv_thread_data() && x11drv_thread_data()->clipping_cursor) + { + was_clipped = TRUE; + ungrab_clipping_window(); + } pos = virtual_screen_to_root( data->whole_rect.left, data->whole_rect.top ); data->whole_window = XCreateWindow( data->display, root_window, pos.x, pos.y, cx, cy, 0, data->vis.depth, InputOutput, data->vis.visual, mask, &attr ); + if (was_clipped) + { + clipping_cursor = TRUE; + retry_grab_clipping_window(); + }
So, is this a problem with the clip fullscreen window option? Or maybe because we also clip fullscreen windows when they are on a single monitor, like I described in https://gitlab.winehq.org/wine/wine/-/merge_requests/3076? I'm not sure to understand how cursor clipping is causing issues with window creation. Even if the cursor is clipped and the clipping window receives all the input, we should still dispatch it to the right window after that? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1153#note_35908
Rémi Bernon (@rbernon) commented about dlls/winex11.drv/mouse.c:
/* need to ungrab the pointer that may have been automatically grabbed * with a ButtonPress event */ - XUngrabPointer( display, CurrentTime ); + if (!clipping_cursor) + XUngrabPointer( display, CurrentTime ); + + /* unclip it during drag, see Wine-Bug 53831 */ + if (x11drv_thread_data() && x11drv_thread_data()->clipping_cursor) + { + was_clipped = TRUE; + ungrab_clipping_window(); + }
Maybe it would be better to use `NtUserClipCursor( NULL )` here, I don't think ungrabbing the cursor from another process does anything, and I'd say it's not completely obvious that the current thread is also the foreground one? Maybe test whether WM_SYSCOMMAND does that already? Is the cursor still constrained on Windows when you move the window? If not, maybe we can just clear the Win32 clipping rect regardless? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1153#note_35910
Rémi Bernon (@rbernon) commented about dlls/winex11.drv/window.c:
if (!(data = get_win_data( NtUserGetAncestor( hwnd, GA_ROOT )))) return; if (data->whole_window) { + if (data->managed && thread_data->clipping_cursor) + ungrab_clipping_window(); XFlush( gdi_display );
Same here as for WM_SYSCOMMAND, how does `SetCapture` behaves against `ClipCursor`? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1153#note_35911
Rémi Bernon (@rbernon) commented about dlls/win32u/input.c:
{ user_driver->pSetCapture( hwnd, gui_flags );
+ if (!hwnd) + clip_fullscreen_window( NtUserGetForegroundWindow(), FALSE ); +
I'm not sure we need to restore the fullscreen clipping here, if the application uses SetCapture, maybe we can rely on it being called later when mouse is clicked? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1153#note_35912
(I'm starting to wonder whether we should simply disable automatic fullscreen clipping entirely unless the option is explicitly enabled) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1153#note_35913
On Fri Jun 16 17:20:24 2023 +0000, Rémi Bernon wrote:
So, is this a problem with the clip fullscreen window option? Or maybe because we also clip fullscreen windows when they are on a single monitor, like I described in https://gitlab.winehq.org/wine/wine/-/merge_requests/3076? I'm not sure to understand how cursor clipping is causing issues with window creation. Even if the cursor is clipped and the clipping window receives all the input, we should still dispatch it to the right window after that? I honestly don't know much about clipping, or why it's done (I've had this MR open for a while, so it's not related to recent changes, I just rebased it now). The issue seems to be with the Window Manager though, so it's most likely X11 specific, it only appears with managed windows.
I don't know how it's related either, but the WM doesn't seem to like it when we're clipping (can easily test with REAPER as described in the bug report). I mean opening menus popup or creating a window (like Project Settings) when we're clipping fullscreen. !3076 doesn't seem to fix it, unfortunately. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1153#note_35914
On Fri Jun 16 17:20:25 2023 +0000, Rémi Bernon wrote:
I'm not sure we need to restore the fullscreen clipping here, if the application uses SetCapture, maybe we can rely on it being called later when mouse is clicked? Yeah, as you probably notice I'm not familiar when it's done or not, so I was just paranoid and wanted to restore it. :)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1153#note_35929
On Fri Jun 16 17:20:24 2023 +0000, Rémi Bernon wrote:
Maybe it would be better to use `NtUserClipCursor( NULL )` here, I don't think ungrabbing the cursor from another process does anything, and I'd say it's not completely obvious that the current thread is also the foreground one? Maybe test whether WM_SYSCOMMAND does that already? Is the cursor still constrained on Windows when you move the window? If not, maybe we can just clear the Win32 clipping rect regardless? To be honest, it seems only the first and last patches are needed now (it didn't use to be the case, maybe things changed). So you're right that maybe SYSCOMMAND does that already now.
Note that without the last patch in win32u, you can't drag windows on top of a fullscreen window at all. I will remove these 2 patches then and only keep the bare necessary. Thanks for the review! -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1153#note_35930
participants (2)
-
Gabriel Ivăncescu -
Rémi Bernon