[PATCH 0/5] MR6809: winex11: Misc window config code tweaks.
Progressing towards https://gitlab.winehq.org/wine/wine/-/merge_requests/6569, and instead of dropping the old fields (https://gitlab.winehq.org/wine/wine/-/merge_requests/6731) now, which could be considered riskier. - Avoid requesting unnecessary window config / state changes, - avoid triggering spurious window restore by updating _NET_WM_STATE on iconic windows, - simplify some code flow in preparation for delayed Win32 window state updates. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6809
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/window.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index 58786d962cf..4992d8c42c4 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1211,9 +1211,10 @@ static void update_net_wm_fullscreen_monitors( struct x11drv_win_data *data ) static void window_set_net_wm_state( struct x11drv_win_data *data, UINT new_state ) { - UINT i, count; + UINT i, count, old_state = data->pending_state.net_wm_state; if (!data->whole_window) return; /* no window, nothing to update */ + if (old_state == new_state) return; /* states are the same, nothing to update */ if (!data->mapped) /* set the _NET_WM_STATE atom directly */ { @@ -1250,6 +1251,8 @@ static void window_set_net_wm_state( struct x11drv_win_data *data, UINT new_stat for (i = 0; i < NB_NET_WM_STATES; i++) { + if (!((old_state ^ new_state) & (1 << i))) continue; + xev.xclient.data.l[0] = (new_state & (1 << i)) ? _NET_WM_STATE_ADD : _NET_WM_STATE_REMOVE; xev.xclient.data.l[1] = X11DRV_Atoms[net_wm_state_atoms[i] - FIRST_XATOM]; xev.xclient.data.l[2] = ((net_wm_state_atoms[i] == XATOM__NET_WM_STATE_MAXIMIZED_VERT) ? -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6809
From: Rémi Bernon <rbernon(a)codeweavers.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 4992d8c42c4..4eb7c4d06d1 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1271,9 +1271,11 @@ static void window_set_net_wm_state( struct x11drv_win_data *data, UINT new_stat static void window_set_config( struct x11drv_win_data *data, const RECT *new_rect, BOOL above ) { UINT style = NtUserGetWindowLongW( data->hwnd, GWL_STYLE ), mask = 0; + const RECT *old_rect = &data->pending_state.rect; XWindowChanges changes; if (!data->whole_window) return; /* no window, nothing to update */ + if (EqualRect( old_rect, new_rect )) return; /* rects are the same, nothing to update */ /* resizing a managed maximized window is not allowed */ if (!(style & WS_MAXIMIZE) || !data->managed) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6809
From: Rémi Bernon <rbernon(a)codeweavers.com> This tends to trigger window mapping with some window managers. --- dlls/winex11.drv/window.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index 4eb7c4d06d1..0c1738c26c6 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1216,6 +1216,7 @@ static void window_set_net_wm_state( struct x11drv_win_data *data, UINT new_stat if (!data->whole_window) return; /* no window, nothing to update */ if (old_state == new_state) return; /* states are the same, nothing to update */ + if (data->pending_state.wm_state == IconicState) return; /* window is iconic, don't update its state now */ if (!data->mapped) /* set the _NET_WM_STATE atom directly */ { Atom atoms[NB_NET_WM_STATES + 1]; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6809
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/event.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c index b543ebae025..75d61d4d84f 100644 --- a/dlls/winex11.drv/event.c +++ b/dlls/winex11.drv/event.c @@ -1230,7 +1230,7 @@ static int get_window_xembed_info( Display *display, Window window ) static void handle_wm_state_notify( HWND hwnd, XPropertyEvent *event, BOOL update_window ) { struct x11drv_win_data *data; - UINT style, value = 0; + UINT style, value = 0, state_cmd = 0; if (!(data = get_win_data( hwnd ))) return; if (event->state == PropertyNewValue) value = get_window_wm_state( event->display, event->window ); @@ -1272,24 +1272,17 @@ static void handle_wm_state_notify( HWND hwnd, XPropertyEvent *event, BOOL updat if ((style & WS_MAXIMIZEBOX) && !(style & WS_DISABLED)) { TRACE( "restoring to max %p/%lx\n", data->hwnd, data->whole_window ); - release_win_data( data ); - send_message( hwnd, WM_SYSCOMMAND, SC_MAXIMIZE, 0 ); - return; + state_cmd = SC_MAXIMIZE; } - TRACE( "not restoring to max win %p/%lx style %08x\n", data->hwnd, data->whole_window, style ); } else { if (style & (WS_MINIMIZE | WS_MAXIMIZE)) { + BOOL activate = (style & (WS_MINIMIZE | WS_VISIBLE)) == (WS_MINIMIZE | WS_VISIBLE); TRACE( "restoring win %p/%lx\n", data->hwnd, data->whole_window ); - release_win_data( data ); - if ((style & (WS_MINIMIZE | WS_VISIBLE)) == (WS_MINIMIZE | WS_VISIBLE)) - NtUserSetActiveWindow( hwnd ); - send_message( hwnd, WM_SYSCOMMAND, SC_RESTORE, 0 ); - return; + state_cmd = MAKELONG(SC_RESTORE, activate); } - TRACE( "not restoring win %p/%lx style %08x\n", data->hwnd, data->whole_window, style ); } } else if (!data->iconic && data->wm_state == IconicState) @@ -1298,14 +1291,17 @@ static void handle_wm_state_notify( HWND hwnd, XPropertyEvent *event, BOOL updat if ((style & WS_MINIMIZEBOX) && !(style & WS_DISABLED)) { TRACE( "minimizing win %p/%lx\n", data->hwnd, data->whole_window ); - release_win_data( data ); - send_message( hwnd, WM_SYSCOMMAND, SC_MINIMIZE, 0 ); - return; + state_cmd = SC_MINIMIZE; } - TRACE( "not minimizing win %p/%lx style %08x\n", data->hwnd, data->whole_window, style ); } done: release_win_data( data ); + + if (state_cmd) + { + if (LOWORD(state_cmd) == SC_RESTORE && HIWORD(state_cmd)) NtUserSetActiveWindow( hwnd ); + send_message( hwnd, WM_SYSCOMMAND, LOWORD(state_cmd), 0 ); + } } static void handle_xembed_info_notify( HWND hwnd, XPropertyEvent *event ) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6809
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/event.c | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/dlls/winex11.drv/event.c b/dlls/winex11.drv/event.c index 75d61d4d84f..22c15b50274 100644 --- a/dlls/winex11.drv/event.c +++ b/dlls/winex11.drv/event.c @@ -1073,9 +1073,8 @@ static BOOL X11DRV_ConfigureNotify( HWND hwnd, XEvent *xev ) struct x11drv_win_data *data; RECT rect; POINT pos = {event->x, event->y}; - UINT flags; + UINT style, flags = 0, config_cmd = 0; int cx, cy, x, y; - DWORD style; if (!hwnd) return FALSE; if (!(data = get_win_data( hwnd ))) return FALSE; @@ -1139,35 +1138,33 @@ static BOOL X11DRV_ConfigureNotify( HWND hwnd, XEvent *xev ) if ((style & WS_CAPTION) == WS_CAPTION || !data->is_fullscreen) { data->net_wm_state = get_window_net_wm_state( event->display, data->whole_window ); - if ((data->net_wm_state & (1 << NET_WM_STATE_MAXIMIZED))) + if ((data->net_wm_state & (1 << NET_WM_STATE_MAXIMIZED)) && !(style & WS_MAXIMIZE)) { - if (!(style & WS_MAXIMIZE)) - { - TRACE( "win %p/%lx is maximized\n", data->hwnd, data->whole_window ); - release_win_data( data ); - send_message( data->hwnd, WM_SYSCOMMAND, SC_MAXIMIZE, 0 ); - return TRUE; - } + TRACE( "window %p/%lx is maximized\n", data->hwnd, data->whole_window ); + config_cmd = SC_MAXIMIZE; } - else if (style & WS_MAXIMIZE) + else if (!(data->net_wm_state & (1 << NET_WM_STATE_MAXIMIZED)) && (style & WS_MAXIMIZE)) { TRACE( "window %p/%lx is no longer maximized\n", data->hwnd, data->whole_window ); - release_win_data( data ); - send_message( data->hwnd, WM_SYSCOMMAND, SC_RESTORE, 0 ); - return TRUE; + config_cmd = SC_RESTORE; } } - - if ((flags & (SWP_NOSIZE | SWP_NOMOVE)) != (SWP_NOSIZE | SWP_NOMOVE)) + if (!config_cmd && (flags & (SWP_NOSIZE | SWP_NOMOVE)) != (SWP_NOSIZE | SWP_NOMOVE)) { - release_win_data( data ); - NtUserSetRawWindowPos( hwnd, rect, flags, FALSE ); - return TRUE; + TRACE( "window %p/%lx config changed %s -> %s, flags %#x\n", data->hwnd, data->whole_window, + wine_dbgstr_rect(&data->rects.window), wine_dbgstr_rect(&rect), flags ); + config_cmd = MAKELONG(SC_MOVE, flags); } - done: release_win_data( data ); - return FALSE; + + if (config_cmd) + { + if (LOWORD(config_cmd) == SC_MOVE) NtUserSetRawWindowPos( hwnd, rect, HIWORD(config_cmd), FALSE ); + else send_message( hwnd, WM_SYSCOMMAND, LOWORD(config_cmd), 0 ); + } + + return !!config_cmd; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6809
Zhiyi Zhang (@zhiyi) commented about dlls/winex11.drv/window.c:
for (i = 0; i < NB_NET_WM_STATES; i++) { + if (!((old_state ^ new_state) & (1 << i))) continue;
This was removed specifically to fix a KDE bug. Please see c5ec1585f6e5211a2b63e3435748210552250534 for details. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6809#note_87310
On Wed Nov 13 12:36:12 2024 +0000, Zhiyi Zhang wrote:
This was removed specifically to fix a KDE bug. Please see c5ec1585f6e5211a2b63e3435748210552250534 for details. But requesting unnecessary changes triggers all sorts of race conditions, feedback loops and spurious window reconfiguration, making everything much more complicated.
We track the window manager changes, and if it sets the maximized state to a window on its own, we should then apply it to the Win32 state to make it consistent with X11 instead of trying to force the WM to do otherwise. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6809#note_87317
On Wed Nov 13 13:38:46 2024 +0000, Rémi Bernon wrote:
But requesting unnecessary changes triggers all sorts of race conditions, feedback loops and spurious window reconfiguration, making everything much more complicated. We track the window manager changes, and if it sets the maximized state to a window on its own, we should then apply it to the Win32 state to make it consistent with X11 instead of trying to force the WM to do otherwise. Fwiw I don't see the tests failing on KWin, either with or without this change. The only failure is at `win.c:10692`, which is the pre-existing focus race condition.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6809#note_87318
On Wed Nov 13 14:15:06 2024 +0000, Rémi Bernon wrote:
Fwiw I don't see the tests failing on KWin, either with or without this change. The only failure is at `win.c:10692`, which is the pre-existing focus race condition. Okay, it probably doesn't matter anymore.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6809#note_87319
Tested this, and it seems fixes the issue with Office 2010+ in mutter (though didn't see this issue with kwin). https://gitlab.gnome.org/GNOME/mutter/-/issues/3777 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6809#note_87359
participants (3)
-
darkblaze69 (＠darkblaze69) -
Rémi Bernon
-
Zhiyi Zhang (＠zhiyi)