[PATCH 0/2] MR5994: Draft: winewayland.drv: Make owned windows into xdg_popups
in tiling compositors, or those with decorations always on, having popups (like menu items or on-hover tooltips) be set as toplevels causes them to become unusable. Instead, if a HWND is owned by another window that has a wayland surface, create the surface as a xdg_popup of the owner. since i couldn't find any info on this issue aside from https://bugs.winehq.org/show_bug.cgi?id=56278, i decided to try and fix it. those patches are a WIP, currently it manages fine running tools like notepad.exe and notepad++, but gimp and winecfg still don't work. some tests, namely tooltip.c in comctl32 are failing, although i can reproduce the failure on the master branch, will look into it. Signed-off-by: Anna (navi) Figueiredo Gomes <navi(a)vlhl.dev> -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5994
From: "Anna (navi) Figueiredo Gomes" <navi(a)vlhl.dev> mutex locking the data individually allows for two different HWND to have they data accessed at once. this is necessary to create a xdg_popup. Signed-off-by: Anna (navi) Figueiredo Gomes <navi(a)vlhl.dev> --- dlls/winewayland.drv/waylanddrv.h | 1 + dlls/winewayland.drv/window.c | 23 ++++++++++++++++------- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/dlls/winewayland.drv/waylanddrv.h b/dlls/winewayland.drv/waylanddrv.h index d4ef01a8a1e..ae719a3d483 100644 --- a/dlls/winewayland.drv/waylanddrv.h +++ b/dlls/winewayland.drv/waylanddrv.h @@ -295,6 +295,7 @@ struct wayland_win_data /* USER client rectangle relative to win32 parent window client area */ RECT client_rect; BOOL managed; + pthread_mutex_t lock; }; struct wayland_win_data *wayland_win_data_get(HWND hwnd); diff --git a/dlls/winewayland.drv/window.c b/dlls/winewayland.drv/window.c index c1fb5c6e0b5..94bd580358b 100644 --- a/dlls/winewayland.drv/window.c +++ b/dlls/winewayland.drv/window.c @@ -101,12 +101,16 @@ static struct wayland_win_data *wayland_win_data_create(HWND hwnd, if ((rb_entry = rb_get(&win_data_rb, hwnd))) { free(data); - return RB_ENTRY_VALUE(rb_entry, struct wayland_win_data, entry); + data = RB_ENTRY_VALUE(rb_entry, struct wayland_win_data, entry); + } + else + { + rb_put(&win_data_rb, hwnd, &data->entry); } - - rb_put(&win_data_rb, hwnd, &data->entry); TRACE("hwnd=%p\n", data->hwnd); + pthread_mutex_lock(&data->lock); + pthread_mutex_unlock(&win_data_mutex); return data; } @@ -117,9 +121,11 @@ static struct wayland_win_data *wayland_win_data_create(HWND hwnd, static void wayland_win_data_destroy(struct wayland_win_data *data) { TRACE("hwnd=%p\n", data->hwnd); + pthread_mutex_lock(&win_data_mutex); rb_remove(&win_data_rb, &data->entry); + pthread_mutex_unlock(&data->lock); pthread_mutex_unlock(&win_data_mutex); if (data->window_surface) @@ -139,15 +145,18 @@ static void wayland_win_data_destroy(struct wayland_win_data *data) struct wayland_win_data *wayland_win_data_get(HWND hwnd) { struct rb_entry *rb_entry; + struct wayland_win_data *data = NULL; pthread_mutex_lock(&win_data_mutex); - if ((rb_entry = rb_get(&win_data_rb, hwnd))) - return RB_ENTRY_VALUE(rb_entry, struct wayland_win_data, entry); + if ((rb_entry = rb_get(&win_data_rb, hwnd))) { + data = RB_ENTRY_VALUE(rb_entry, struct wayland_win_data, entry); + pthread_mutex_lock(&data->lock); + } pthread_mutex_unlock(&win_data_mutex); - return NULL; + return data; } /*********************************************************************** @@ -158,7 +167,7 @@ struct wayland_win_data *wayland_win_data_get(HWND hwnd) void wayland_win_data_release(struct wayland_win_data *data) { assert(data); - pthread_mutex_unlock(&win_data_mutex); + pthread_mutex_unlock(&data->lock); } static void wayland_win_data_get_config(struct wayland_win_data *data, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5994
From: "Anna (navi) Figueiredo Gomes" <navi(a)vlhl.dev> in tiling compositors, or those with decorations always on, having popups (like menu items or on-hover tooltips) be set as toplevels causes them to become unusable. Instead, if a HWND is owned by another window that has a wayland surface, create the surface as a xdg_popup of the owner. Signed-off-by: Anna (navi) Figueiredo Gomes <navi(a)vlhl.dev> --- dlls/winewayland.drv/wayland_surface.c | 92 +++++++++++++++++++++++++- dlls/winewayland.drv/waylanddrv.h | 4 ++ dlls/winewayland.drv/window.c | 55 ++++++++++++--- 3 files changed, 139 insertions(+), 12 deletions(-) diff --git a/dlls/winewayland.drv/wayland_surface.c b/dlls/winewayland.drv/wayland_surface.c index 39c3976cdfd..96c30a34423 100644 --- a/dlls/winewayland.drv/wayland_surface.c +++ b/dlls/winewayland.drv/wayland_surface.c @@ -131,6 +131,43 @@ static const struct xdg_toplevel_listener xdg_toplevel_listener = xdg_toplevel_handle_close }; +static void xdg_popup_handle_configure(void *data, + struct xdg_popup *xdg_popup, + int32_t x, int32_t y, + int32_t width, int32_t height) +{ + struct wayland_surface *surface; + HWND hwnd = data; + + TRACE("hwnd=%p %dx%d+%d+%d\n", hwnd, width, height, x, y); + + if (!(surface = wayland_surface_lock_hwnd(hwnd))) return; + + if (surface->xdg_popup == xdg_popup) + { + surface->pending.width = width; + surface->pending.height = height; + } + + pthread_mutex_unlock(&surface->mutex); +} + +static void xdg_popup_handle_done(void *data, struct xdg_popup *xdg_popup) +{ + NtUserPostMessage((HWND)data, WM_SYSCOMMAND, SC_CLOSE, 0); +} + +/* Unhandled event: xdg_popup_reposition is never called */ +static void xdg_popup_handle_repositioned(void *data, struct xdg_popup *xdg_popup, uint32_t token) +{} + +static const struct xdg_popup_listener xdg_popup_listener = +{ + xdg_popup_handle_configure, + xdg_popup_handle_done, + xdg_popup_handle_repositioned, +}; + /********************************************************************** * wayland_surface_create * @@ -212,6 +249,12 @@ void wayland_surface_destroy(struct wayland_surface *surface) surface->xdg_toplevel = NULL; } + if (surface->xdg_popup) + { + xdg_popup_destroy(surface->xdg_popup); + surface->xdg_popup = NULL; + } + if (surface->xdg_surface) { xdg_surface_destroy(surface->xdg_surface); @@ -267,6 +310,47 @@ err: ERR("Failed to assign toplevel role to wayland surface\n"); } +/********************************************************************** + * wayland_surface_make_toplevel + * + * Gives the popup role to a plain wayland surface. + */ +void wayland_surface_make_popup(struct wayland_surface *surface, + struct wayland_surface *parent, + const RECT *visible) +{ + struct xdg_positioner *positioner; + int32_t width = visible->right - visible->left; + int32_t height = visible->bottom - visible->top; + if (width <= 0 || height <= 0) goto err; + + TRACE("surface=%p\n", surface); + + surface->xdg_surface = + xdg_wm_base_get_xdg_surface(process_wayland.xdg_wm_base, surface->wl_surface); + if (!surface->xdg_surface) goto err; + xdg_surface_add_listener(surface->xdg_surface, &xdg_surface_listener, surface->hwnd); + + + positioner = xdg_wm_base_create_positioner(process_wayland.xdg_wm_base); + xdg_positioner_set_size(positioner, width, height); + xdg_positioner_set_anchor_rect(positioner, visible->left, visible->top, width, height); + + surface->xdg_popup = xdg_surface_get_popup(surface->xdg_surface, parent->xdg_surface, positioner); + xdg_positioner_destroy(positioner); + if (!surface->xdg_popup) goto err; + xdg_popup_add_listener(surface->xdg_popup, &xdg_popup_listener, surface->hwnd); + + wl_surface_commit(surface->wl_surface); + wl_display_flush(process_wayland.wl_display); + + return; + +err: + wayland_surface_clear_role(surface); + ERR("Failed to assign popup role to wayland surface\n"); +} + /********************************************************************** * wayland_surface_clear_role * @@ -284,6 +368,12 @@ void wayland_surface_clear_role(struct wayland_surface *surface) surface->xdg_toplevel = NULL; } + if (surface->xdg_popup) + { + xdg_popup_destroy(surface->xdg_popup); + surface->xdg_popup = NULL; + } + if (surface->xdg_surface) { xdg_surface_destroy(surface->xdg_surface); @@ -538,7 +628,7 @@ BOOL wayland_surface_reconfigure(struct wayland_surface *surface) struct wayland_window_config *window = &surface->window; int win_width, win_height, width, height; - if (!surface->xdg_toplevel) return TRUE; + if (!surface->xdg_toplevel && !surface->xdg_popup) return TRUE; win_width = surface->window.rect.right - surface->window.rect.left; win_height = surface->window.rect.bottom - surface->window.rect.top; diff --git a/dlls/winewayland.drv/waylanddrv.h b/dlls/winewayland.drv/waylanddrv.h index ae719a3d483..0a031c33143 100644 --- a/dlls/winewayland.drv/waylanddrv.h +++ b/dlls/winewayland.drv/waylanddrv.h @@ -195,6 +195,7 @@ struct wayland_surface struct wl_surface *wl_surface; struct xdg_surface *xdg_surface; struct xdg_toplevel *xdg_toplevel; + struct xdg_popup *xdg_popup; struct wp_viewport *wp_viewport; pthread_mutex_t mutex; struct wayland_surface_config pending, requested, processing, current; @@ -239,6 +240,9 @@ void wayland_output_use_xdg_extension(struct wayland_output *output); struct wayland_surface *wayland_surface_create(HWND hwnd); void wayland_surface_destroy(struct wayland_surface *surface); void wayland_surface_make_toplevel(struct wayland_surface *surface); +void wayland_surface_make_popup(struct wayland_surface *surface, + struct wayland_surface *parent, + const RECT *visible); void wayland_surface_clear_role(struct wayland_surface *surface); void wayland_surface_attach_shm(struct wayland_surface *surface, struct wayland_shm_buffer *shm_buffer, diff --git a/dlls/winewayland.drv/window.c b/dlls/winewayland.drv/window.c index 94bd580358b..a4e3ff93e27 100644 --- a/dlls/winewayland.drv/window.c +++ b/dlls/winewayland.drv/window.c @@ -209,10 +209,31 @@ static void reapply_cursor_clipping(void) NtUserSetThreadDpiAwarenessContext(context); } +static struct wayland_win_data *get_parent_data(HWND handle) +{ + HWND parent = NtUserGetWindowRelative(handle, GW_OWNER); + struct wayland_win_data *data; + + if (parent == 0) return NULL; + + TRACE("parent hwnd: %p\n", parent); + + data = wayland_win_data_get(parent); + if (!data) return NULL; + + if (!data->wayland_surface || !data->wayland_surface->xdg_surface) { + wayland_win_data_release(data); + return NULL; + } + + return data; +} + static void wayland_win_data_update_wayland_surface(struct wayland_win_data *data, const RECT *visible_rect) { struct wayland_surface *surface = data->wayland_surface; HWND parent = NtUserGetAncestor(data->hwnd, GA_PARENT); + struct wayland_win_data *parent_data; BOOL visible, xdg_visible; WCHAR text[1024]; @@ -230,9 +251,8 @@ static void wayland_win_data_update_wayland_surface(struct wayland_win_data *dat /* Otherwise ensure that we have a wayland surface. */ if (!surface && !(surface = wayland_surface_create(data->hwnd))) return; - visible = (NtUserGetWindowLongW(data->hwnd, GWL_STYLE) & WS_VISIBLE) == WS_VISIBLE; - xdg_visible = surface->xdg_toplevel != NULL; + xdg_visible = surface->xdg_toplevel != NULL || surface->xdg_popup != NULL; pthread_mutex_lock(&surface->mutex); @@ -240,17 +260,30 @@ static void wayland_win_data_update_wayland_surface(struct wayland_win_data *dat { /* If we have a pre-existing surface ensure it has no role. */ if (data->wayland_surface) wayland_surface_clear_role(surface); - /* If the window is a visible toplevel make it a wayland - * xdg_toplevel. Otherwise keep it role-less to avoid polluting the - * compositor with empty xdg_toplevels. */ + /* If the window is visible give it a wayland xdg role. + * Otherwise keep it role-less to avoid polluting the + * compositor with empty xdg_toplevels or xdg_popups. */ if (visible) { - wayland_surface_make_toplevel(surface); - if (surface->xdg_toplevel) + /* If the window has a parent that has a wayland surface, + * give it the popup role, so we can position it properly, + * specially in tiling compositors or those with decorations + * enable. Otherwise, make it a toplevel. + * */ + if ((parent_data = get_parent_data(data->hwnd))) { - if (!NtUserInternalGetWindowText(data->hwnd, text, ARRAY_SIZE(text))) - text[0] = 0; - wayland_surface_set_title(surface, text); + wayland_surface_make_popup(surface, parent_data->wayland_surface, visible_rect); + wayland_win_data_release(parent_data); + } + else + { + wayland_surface_make_toplevel(surface); + if (surface->xdg_toplevel) + { + if (!NtUserInternalGetWindowText(data->hwnd, text, ARRAY_SIZE(text))) + text[0] = 0; + wayland_surface_set_title(surface, text); + } } } } @@ -508,7 +541,7 @@ static void wayland_configure_window(HWND hwnd) if (!(surface = wayland_surface_lock_hwnd(hwnd))) return; - if (!surface->xdg_toplevel) + if (!surface->xdg_toplevel && !surface->xdg_popup) { TRACE("missing xdg_toplevel, returning\n"); pthread_mutex_unlock(&surface->mutex); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5994
Hi Anna, thanks for the MR. I have also recently been working on a branch that tackles the same issue (among other things). I pushed my current WIP so you can take a look: https://gitlab.winehq.org/afrantzis/wine/-/commits/wip/use-subsurfaces A major difference between the two approaches is that my WIP is using wl_subsurface instead of xdg_popup. The reason is that the behavior of transient (and other "unmanaged") windows in win32 is supposed to be under the control of the application. wl_subsurface is a good match here because it doesn't introduce any additional compositor behavior, whereas xdg_popup gives a lot more control to the compositor (I have concerns especially about compositor side dismissals). Some other notes: * Win32 owned windows are a more general concept than xdg_popups. Although many owned windows are indeed transient windows, some are not (e.g., dialogs) and are better served by another mechanism (e.g., xdg_toplevel parent/child), or no mechanism at all (see my thoughts above about not introducing any additional behavior on top of win32). * Inversely, some transient windows are not owned, and we need to guess a suitable parent if we want to anchor them to another wayland surface (see, e.g., my WIP branch). In terms of next steps, my proposal is that I first finish and land the subsurfaces functionality, since it provides the least friction in terms of win32/wayland behavior, and is needed for other things anyway. Then I think we will have a clearer view, as a well as a base to compare against, in order to explore how and when to utilize xdg_popups to achieve better integration. How does that sound? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5994#note_75187
hello, thanks for the feedback i, well, had no wine or win32 experience before writing this, and spent half a day yesterday trying to understand both, and since wayland i know, started there. so, apologies i didn't know much of win32.
Win32 owned windows are a more general concept than xdg_popups
considering this, i do agree subsurfaces make more sense in the general case.
Inversely, some transient windows are not owned, and we need to guess a suitable parent if we want to anchor them to another wayland surface (see, e.g., my WIP branch).
i noticed this on winecfg, where the dropdowns were like that and didn't properly get matched, tried to poke at that today but didn't have much time to find out a way.
some are not (e.g., dialogs) and are better served by another mechanism (e.g., xdg_toplevel parent/child),
was using xdg_dialog thought of for win32 dialogs?
Then I think we will have a clearer view, as a well as a base to compare against, in order to explore how and when to utilize xdg_popups to achieve better integration. How does that sound?
that sounds good. -- ps: i've also been working on patches to use xdg_decoration to conditionally use server side decorations like the x11drv does, if that's of any interest -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5994#note_75224
On Fri Jul 5 20:18:21 2024 +0000, Alexandros Frantzis wrote:
Hi Anna, thanks for the MR. I have also recently been working on a branch that tackles the same issue (among other things). I pushed my current WIP so you can take a look: https://gitlab.winehq.org/afrantzis/wine/-/commits/wip/use-subsurfaces A major difference between the two approaches is that my WIP is using wl_subsurface instead of xdg_popup. The reason is that the behavior of transient (and other "unmanaged") windows in win32 is supposed to be under the control of the application. wl_subsurface is a good match here because it doesn't introduce any additional compositor behavior, whereas xdg_popup gives a lot more control to the compositor (I have concerns especially about compositor side dismissals). Some other notes: * Win32 owned windows are a more general concept than xdg_popups. Although many owned windows are indeed transient windows, some are not (e.g., dialogs) and are better served by another mechanism (e.g., xdg_toplevel parent/child), or no mechanism at all (see my thoughts above about not introducing any additional behavior on top of win32). * Inversely, some transient windows are not owned, and we need to guess a suitable parent if we want to anchor them to another wayland surface (see, e.g., my WIP branch). In terms of next steps, my proposal is that I first finish and land the subsurfaces functionality, since it provides the least friction in terms of win32/wayland behavior, and is needed for other things anyway. Then I think we will have a clearer view, as a well as a base to compare against, in order to explore how and when to utilize xdg_popups to achieve better integration. How does that sound? hello, thanks for the feedback
i, well, had no wine or win32 experience before writing this, and spent half a day yesterday trying to understand both, and since wayland i know, started there. so, apologies i didn't know much of win32.
Win32 owned windows are a more general concept than xdg_popups
considering this, i do agree subsurfaces make more sense in the general case.
Inversely, some transient windows are not owned, and we need to guess a suitable parent if we want to anchor them to another wayland surface (see, e.g., my WIP branch).
i noticed this on winecfg, where the dropdowns were like that and didn't properly get matched, tried to poke at that today but didn't have much time to find out a way.
some are not (e.g., dialogs) and are better served by another mechanism (e.g., xdg_toplevel parent/child),
was using xdg_dialog thought of for win32 dialogs?
Then I think we will have a clearer view, as a well as a base to compare against, in order to explore how and when to utilize xdg_popups to achieve better integration. How does that sound?
that sounds good. -- ps: i've also been working on patches to use xdg_decoration to conditionally use server side decorations like the x11drv does, if that's of any interest -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5994#note_75225
On Fri Jul 5 20:18:21 2024 +0000, Anna Figueiredo Gomes wrote:
hello, thanks for the feedback i, well, had no wine or win32 experience before writing this, and spent half a day yesterday trying to understand both, and since wayland i know, started there. so, apologies i didn't know much of win32.
Win32 owned windows are a more general concept than xdg_popups considering this, i do agree subsurfaces make more sense in the general case. Inversely, some transient windows are not owned, and we need to guess a suitable parent if we want to anchor them to another wayland surface (see, e.g., my WIP branch). i noticed this on winecfg, where the dropdowns were like that and didn't properly get matched, tried to poke at that today but didn't have much time to find out a way. some are not (e.g., dialogs) and are better served by another mechanism (e.g., xdg_toplevel parent/child), was using xdg_dialog thought of for win32 dialogs? Then I think we will have a clearer view, as a well as a base to compare against, in order to explore how and when to utilize xdg_popups to achieve better integration. How does that sound? that sounds good. -- ps: i've also been working on patches to use xdg_decoration to conditionally use server side decorations like the x11drv does, if that's of any interest @afrantzis I like that branch `wip/use-subsurfaces`, works amazing, though MS office 2007 like excel or word crashes after right click on editing area. Can't wait for something like this to materialize in some way )
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5994#note_77202
This merge request was closed by Anna Figueiredo Gomes. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5994
participants (4)
-
Alexandros Frantzis (＠afrantzis) -
Anna (navi) Figueiredo Gomes
-
Anna Figueiredo Gomes (＠navi)
-
darkblaze69 (＠darkblaze69)