[PATCH 0/1] MR5076: explorer: Set layered style on systray icons on creation if we don't use our own taskbar.
Turns out I broke the non virtual desktop case in some cases. Sorry about that. This basically uses previous behavior for such (`enable_taskbar` is what's used to also determine layered changes in rest of code) while keeping the fix for virtual desktops (i.e. our own taskbar). Works correctly for me, but let me know if I did something wrong again (welp). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5076
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Fixes a regression introduced by b5c57b9a62c396068d18237bd6e82b37c169fdc5, which broke the systray integration outside of virtual desktops. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- programs/explorer/systray.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/programs/explorer/systray.c b/programs/explorer/systray.c index c76ebdd0c92..02522e7b537 100644 --- a/programs/explorer/systray.c +++ b/programs/explorer/systray.c @@ -733,8 +733,9 @@ static BOOL add_icon(NOTIFYICONDATAW *nid) icon->id = nid->uID; icon->owner = nid->hWnd; icon->display = ICON_DISPLAY_HIDDEN; + icon->layered = !enable_taskbar; - CreateWindowExW( 0, tray_icon_class.lpszClassName, NULL, WS_CLIPSIBLINGS | WS_POPUP, + CreateWindowExW( icon->layered ? WS_EX_LAYERED : 0, tray_icon_class.lpszClassName, NULL, WS_CLIPSIBLINGS | WS_POPUP, 0, 0, icon_cx, icon_cy, 0, NULL, NULL, icon ); if (!icon->window) ERR( "Failed to create systray icon window\n" ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5076
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=143060 Your paranoid android. === debian11b (64 bit WoW report) === adsldp: ldap: Timeout
It worked for me when I tested the host systray embedded mode, could you be more specific what is broken? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5076#note_61019
On Tue Feb 13 19:38:43 2024 +0000, Rémi Bernon wrote:
It worked for me when I tested the host systray embedded mode, could you be more specific what is broken? It's broken on XFCE for some reason, the icon simply doesn't show up. (reported by Alistair on wine-staging with Steam and I tested it)
It's not Steam specific, I could repro it with Winamp (shift + click minimize moves it to systray) and AutoHotkey on XFCE. Of course, this is *without* virtual desktop. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5076#note_61022
On Tue Feb 13 19:38:43 2024 +0000, Gabriel Ivăncescu wrote:
It's broken on XFCE for some reason, the icon simply doesn't show up. (reported by Alistair on wine-staging with Steam and I tested it) It's not Steam specific, I could repro it with Winamp (shift + click minimize moves it to systray) and AutoHotkey on XFCE. Of course, this is *without* virtual desktop. Okay well I think it would be better to figure out what is going on, and find how to fix it properly then.
Your previous change looked right, as the icons were created with the layered attribute but then never painted as layered windows (`UpdateLayeredWindow` was never called because icon->layered is FALSE unless they are docked). Yet, it was also working for me before, because somehow even with that attribute the windows can be painted normally, but I think it's better to only set the style if the windows are layered and going to be painted with `UpdateLayeredWindow`. Alternatively, if you can make the layered icons work for all the cases it might also make the code simpler. `UpdateLayeredWindow` is required for windows embedded in the host systray, it's not required for embedding in Wine systray or taskbar but maybe it could work. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5076#note_61023
On Tue Feb 13 19:48:49 2024 +0000, Rémi Bernon wrote:
Okay well I think it would be better to figure out what is going on, and find how to fix it properly then. Your previous change looked right, as the icons were created with the layered attribute but then never painted as layered windows (`UpdateLayeredWindow` was never called because `icon->layered` is FALSE unless they are docked). Yet, it was also working for me before, because somehow even with that attribute the windows can be painted normally, but I think it's better to only set the style if the windows are layered and going to be painted with `UpdateLayeredWindow`. Alternatively, if you can make the layered icons work for all the cases it might also make the code simpler. `UpdateLayeredWindow` is required for windows embedded in the host systray, it's not required for embedding in Wine systray or taskbar but maybe it could work. Looks like the issue is that X11DRV_SystrayDockInsert is called before we make the window layered. Though I'm not sure how to fix this properly, because we check the return value of it first before setting it to layered.
I tried to make all icons layered but unfortunately that doesn't work for the Wine systray and I've no idea why (not familiar with this code). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5076#note_61026
On Tue Feb 13 20:32:49 2024 +0000, Gabriel Ivăncescu wrote:
Looks like the issue is that X11DRV_SystrayDockInsert is called before we make the window layered. Though I'm not sure how to fix this properly, because we check the return value of it first before setting it to layered. I tried to make all icons layered but unfortunately that doesn't work for the Wine systray and I've no idea why (not familiar with this code). What about adding:
```C /* make sure the window is layered with a surface before embedding them */ NtUserUpdateLayeredWindow( hwnd, NULL, NULL, NULL, NULL, NULL, 0, NULL, 0, NULL ); ``` In `X11DRV_SystrayDockInsert`, after `get_systray_selection_owner`? (no idea if that works) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5076#note_61077
In `X11DRV_SystrayDockInsert`, after `get_systray_selection_owner`? (no idea if that works)
Do you mean a change like this? It doesn't work. ```diff --- wine-9.2/dlls/winex11.drv/window.c.orig 2024-02-14 15:03:26.316078760 +0800 +++ wine-9.2/dlls/winex11.drv/window.c 2024-02-14 15:03:45.242545962 +0800 @@ -2328,6 +2328,9 @@ if (!(systray_window = get_systray_selection_owner( display ))) return FALSE; + /* make sure the window is layered with a surface before embedding them */ + NtUserUpdateLayeredWindow( hwnd, NULL, NULL, NULL, NULL, NULL, 0, NULL, 0, NULL ); + get_systray_visual_info( display, systray_window, &visual ); if (!(data = get_win_data( hwnd ))) return FALSE; ``` On the other hand, changes in https://gitlab.winehq.org/wine/wine/-/merge_requests/5076/diffs restores missing systray icons with wine 9.2. I tested LINE of LY Corporation on LXQt. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5076#note_61120
On Wed Feb 14 07:36:09 2024 +0000, Chih-Hsuan Yen wrote:
In `X11DRV_SystrayDockInsert`, after `get_systray_selection_owner`? (no idea if that works) Do you mean a change like this? It doesn't work.
--- wine-9.2/dlls/winex11.drv/window.c.orig 2024-02-14 15:03:26.316078760 +0800 +++ wine-9.2/dlls/winex11.drv/window.c 2024-02-14 15:03:45.242545962 +0800 @@ -2328,6 +2328,9 @@ if (!(systray_window = get_systray_selection_owner( display ))) return FALSE; + /* make sure the window is layered with a surface before embedding them */ + NtUserUpdateLayeredWindow( hwnd, NULL, NULL, NULL, NULL, NULL, 0, NULL, 0, NULL ); + get_systray_visual_info( display, systray_window, &visual ); if (!(data = get_win_data( hwnd ))) return FALSE;On the other hand, changes in https://gitlab.winehq.org/wine/wine/-/merge_requests/5076/diffs restores missing systray icons with wine 9.2. I tested LINE of LY Corporation on LXQt. Well that can't work because the whole issue is that it's *not* set to layered at that point. We set it to layered *after* this call (in explorer).
`NtUserUpdateLayeredWindow` bails out early if the window is not WS_EX_LAYERED. I could move the call to make it layered before calling the driver function of course, but the function can return FALSE, and then what happens? Should I revert it back, or not sure how to fix it. Alternatively we could set it to layered in the driver as well? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5076#note_61174
I could move the call to make it layered before calling the driver function of course, but the function can return FALSE, and then what happens? Should I revert it back, or not sure how to fix it.
Yes, setting it then calling `paint_layered_icon` once, before trying to dock them, and removing the attribute if docking returns FALSE looks correct. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5076#note_61177
On Wed Feb 14 16:39:19 2024 +0000, Rémi Bernon wrote:
I could move the call to make it layered before calling the driver function of course, but the function can return FALSE, and then what happens? Should I revert it back, or not sure how to fix it. Yes, setting it then calling `paint_layered_icon` once, before trying to dock them, and removing the attribute if docking returns FALSE looks correct. BTW I think I found the reason it fails. It's not the `make_window_embedded` or `set_window_visual` calls. Neither is the `NtUserShowWindow`. For example the following works:
```diff diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index 8ab6944..1ce0165 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -2337,6 +2337,11 @@ BOOL X11DRV_SystrayDockInsert( HWND hwnd, UINT cx, UINT cy, void *icon ) release_win_data( data ); NtUserShowWindow( hwnd, SW_SHOWNA ); + NtUserSetWindowLong( hwnd, GWL_EXSTYLE, NtUserGetWindowLongW( hwnd, GWL_EXSTYLE ) | WS_EX_LAYERED, FALSE ); + + if (!(data = get_win_data( hwnd ))) return FALSE; + window = data->whole_window; + release_win_data( data ); TRACE_(systray)( "icon window %p/%lx\n", hwnd, window ); ``` One important thing to notice is that the whole `window = data->whole_window` **after** setting it to layered is important, so the correct window is sent to `XSendEvent` for the docking message. Without that, it will still fail. So why does this happen? Because setting it to WS_EX_LAYERED makes it eventually end up in `set_window_visual` (from `X11DRV_SetWindowStyle`), which **destroys and recreates the X window**. As in, the old X window no longer exists, so there's nothing to dock! Sure enough the following hack also fixes it: ```diff diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index 8ab6944..0e2ef96 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1812,6 +1812,7 @@ void set_window_visual( struct x11drv_win_data *data, const XVisualInfo *vis, BO Window client_window = data->client_window; Window whole_window = data->whole_window; + return; if (!data->use_alpha == !use_alpha) return; if (data->surface) window_surface_release( data->surface ); data->surface = NULL; ``` Why does it even need to destroy and recreate the window? I'm going to experiment a bit but otherwise I'll go with just setting it earlier. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5076#note_61178
On Wed Feb 14 16:42:28 2024 +0000, Gabriel Ivăncescu wrote:
BTW I think I found the reason it fails. It's not the `make_window_embedded` or `set_window_visual` calls. Neither is the `NtUserShowWindow`. For example the following works: ```diff diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index 8ab6944..1ce0165 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -2337,6 +2337,11 @@ BOOL X11DRV_SystrayDockInsert( HWND hwnd, UINT cx, UINT cy, void *icon ) release_win_data( data );
NtUserShowWindow( hwnd, SW_SHOWNA ); + NtUserSetWindowLong( hwnd, GWL_EXSTYLE, NtUserGetWindowLongW( hwnd, GWL_EXSTYLE ) | WS_EX_LAYERED, FALSE ); + + if (!(data = get_win_data( hwnd ))) return FALSE; + window = data->whole_window; + release_win_data( data );
TRACE_(systray)( "icon window %p/%lx\n", hwnd, window );
``` One important thing to notice is that the whole `window = data->whole_window` **after** setting it to layered is important, so the correct window is sent to `XSendEvent` for the docking message. Without that, it will still fail. So why does this happen? Because setting it to WS_EX_LAYERED makes it eventually end up in `set_window_visual` (from `X11DRV_SetWindowStyle`), which **destroys and recreates the X window**. As in, the old X window no longer exists, so there's nothing to dock! Sure enough the following hack also fixes it: ```diff diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index 8ab6944..0e2ef96 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1812,6 +1812,7 @@ void set_window_visual( struct x11drv_win_data *data, const XVisualInfo *vis, BO Window client_window = data->client_window; Window whole_window = data->whole_window;
+ return; if (!data->use_alpha == !use_alpha) return; if (data->surface) window_surface_release( data->surface ); data->surface = NULL; ``` Why does it even need to destroy and recreate the window? I'm going to experiment a bit but otherwise I'll go with just setting it earlier. How about something like this?
```diff diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index 8ab6944..7bda648 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1822,14 +1822,32 @@ void set_window_visual( struct x11drv_win_data *data, const XVisualInfo *vis, BO destroy_whole_window( data, client_window != 0 /* don't destroy whole_window until reparented */ ); data->vis = *vis; create_whole_window( data ); - if (!client_window) return; - /* move the client to the new parent */ - XReparentWindow( gdi_display, client_window, data->whole_window, - data->client_rect.left - data->whole_rect.left, - data->client_rect.top - data->whole_rect.top ); - data->client_window = client_window; - XSync( gdi_display, False ); /* make sure XReparentWindow requests have completed before destroying whole_window */ - XDestroyWindow( data->display, whole_window ); + if (client_window) + { + /* move the client to the new parent */ + XReparentWindow( gdi_display, client_window, data->whole_window, + data->client_rect.left - data->whole_rect.left, + data->client_rect.top - data->whole_rect.top ); + data->client_window = client_window; + XSync( gdi_display, False ); /* make sure XReparentWindow requests have completed before destroying whole_window */ + XDestroyWindow( data->display, whole_window ); + } + + /* if we were docked, re-send the docking request message */ + if (data->docked_to) + { + XEvent ev; + ev.xclient.type = ClientMessage; + ev.xclient.window = data->docked_to; + ev.xclient.message_type = x11drv_atom( _NET_SYSTEM_TRAY_OPCODE ); + ev.xclient.format = 32; + ev.xclient.data.l[0] = CurrentTime; + ev.xclient.data.l[1] = SYSTEM_TRAY_REQUEST_DOCK; + ev.xclient.data.l[2] = data->whole_window; + ev.xclient.data.l[3] = 0; + ev.xclient.data.l[4] = 0; + XSendEvent( data->display, data->docked_to, False, NoEventMask, &ev ); + } } @@ -2272,6 +2290,7 @@ BOOL X11DRV_SystrayDockRemove( HWND hwnd ) if ((data = get_win_data( hwnd ))) { if ((ret = data->embedded)) data->mapped = FALSE; + data->docked_to = 0; release_win_data( data ); return ret; } @@ -2334,6 +2353,7 @@ BOOL X11DRV_SystrayDockInsert( HWND hwnd, UINT cx, UINT cy, void *icon ) set_window_visual( data, &visual, TRUE ); make_window_embedded( data ); window = data->whole_window; + data->docked_to = systray_window; release_win_data( data ); NtUserShowWindow( hwnd, SW_SHOWNA ); diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h index 71a3582..b1ea370 100644 --- a/dlls/winex11.drv/x11drv.h +++ b/dlls/winex11.drv/x11drv.h @@ -628,6 +628,7 @@ struct x11drv_win_data int wm_state; /* current value of the WM_STATE property */ DWORD net_wm_state; /* bit mask of active x11drv_net_wm_state values */ Window embedder; /* window id of embedder */ + Window docked_to; /* window id of systray if we are docked to it */ unsigned long configure_serial; /* serial number of last configure request */ struct window_surface *surface; Pixmap icon_pixmap; ``` It also fixes it without any change to explorer, since it seems to be a winex11.drv quirk we have (destroying and re-creating the window…). Is this acceptable? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5076#note_61185
On Wed Feb 14 18:12:55 2024 +0000, Gabriel Ivăncescu wrote:
How about something like this? ```diff diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index 8ab6944..7bda648 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1822,14 +1822,32 @@ void set_window_visual( struct x11drv_win_data *data, const XVisualInfo *vis, BO destroy_whole_window( data, client_window != 0 /* don't destroy whole_window until reparented */ ); data->vis = *vis; create_whole_window( data ); - if (!client_window) return; - /* move the client to the new parent */ - XReparentWindow( gdi_display, client_window, data->whole_window, - data->client_rect.left - data->whole_rect.left, - data->client_rect.top - data->whole_rect.top ); - data->client_window = client_window; - XSync( gdi_display, False ); /* make sure XReparentWindow requests have completed before destroying whole_window */ - XDestroyWindow( data->display, whole_window ); + if (client_window) + { + /* move the client to the new parent */ + XReparentWindow( gdi_display, client_window, data->whole_window, + data->client_rect.left - data->whole_rect.left, + data->client_rect.top - data->whole_rect.top ); + data->client_window = client_window; + XSync( gdi_display, False ); /* make sure XReparentWindow requests have completed before destroying whole_window */ + XDestroyWindow( data->display, whole_window ); + } + + /* if we were docked, re-send the docking request message */ + if (data->docked_to) + { + XEvent ev; + ev.xclient.type = ClientMessage; + ev.xclient.window = data->docked_to; + ev.xclient.message_type = x11drv_atom( _NET_SYSTEM_TRAY_OPCODE ); + ev.xclient.format = 32; + ev.xclient.data.l[0] = CurrentTime; + ev.xclient.data.l[1] = SYSTEM_TRAY_REQUEST_DOCK; + ev.xclient.data.l[2] = data->whole_window; + ev.xclient.data.l[3] = 0; + ev.xclient.data.l[4] = 0; + XSendEvent( data->display, data->docked_to, False, NoEventMask, &ev ); + } }
@@ -2272,6 +2290,7 @@ BOOL X11DRV_SystrayDockRemove( HWND hwnd ) if ((data = get_win_data( hwnd ))) { if ((ret = data->embedded)) data->mapped = FALSE; + data->docked_to = 0; release_win_data( data ); return ret; } @@ -2334,6 +2353,7 @@ BOOL X11DRV_SystrayDockInsert( HWND hwnd, UINT cx, UINT cy, void *icon ) set_window_visual( data, &visual, TRUE ); make_window_embedded( data ); window = data->whole_window; + data->docked_to = systray_window; release_win_data( data );
NtUserShowWindow( hwnd, SW_SHOWNA ); diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h index 71a3582..b1ea370 100644 --- a/dlls/winex11.drv/x11drv.h +++ b/dlls/winex11.drv/x11drv.h @@ -628,6 +628,7 @@ struct x11drv_win_data int wm_state; /* current value of the WM_STATE property */ DWORD net_wm_state; /* bit mask of active x11drv_net_wm_state values */ Window embedder; /* window id of embedder */ + Window docked_to; /* window id of systray if we are docked to it */ unsigned long configure_serial; /* serial number of last configure request */ struct window_surface *surface; Pixmap icon_pixmap; ``` It also fixes it without any change to explorer, since it seems to be a winex11.drv quirk we have (destroying and re-creating the window…). Is this acceptable? I think it's better to change it in `explorer.exe`, the systray dock interface contract currently kind of advertise that it'll provide layered icon windows to the drivers. Making sure they are fully layered before calling WINE_SYSTRAY_DOCK_INSERT seems the proper thing to do.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5076#note_61187
participants (4)
-
Chih-Hsuan Yen (＠yan12125) -
Gabriel Ivăncescu
-
Marvin -
Rémi Bernon