From bd5c4f4a4a79743df77db40d18eb32686312f320 Mon Sep 17 00:00:00 2001 From: Kirill Primak Date: Wed, 26 Jul 2023 11:36:24 +0300 Subject: [PATCH] xdg-shell: rework roles --- include/types/wlr_xdg_shell.h | 11 +- include/wlr/types/wlr_xdg_shell.h | 19 ++- types/xdg_shell/wlr_xdg_popup.c | 35 +++-- types/xdg_shell/wlr_xdg_shell.c | 2 +- types/xdg_shell/wlr_xdg_surface.c | 203 ++++++++++++++++++----------- types/xdg_shell/wlr_xdg_toplevel.c | 44 ++----- 6 files changed, 174 insertions(+), 140 deletions(-) diff --git a/include/types/wlr_xdg_shell.h b/include/types/wlr_xdg_shell.h index 96cc117fa..34dec2adb 100644 --- a/include/types/wlr_xdg_shell.h +++ b/include/types/wlr_xdg_shell.h @@ -5,16 +5,13 @@ #include #include "xdg-shell-protocol.h" -extern const struct wlr_surface_role xdg_toplevel_surface_role; -extern const struct wlr_surface_role xdg_popup_surface_role; - void create_xdg_surface(struct wlr_xdg_client *client, struct wlr_surface *wlr_surface, uint32_t id); void destroy_xdg_surface(struct wlr_xdg_surface *surface); -void destroy_xdg_surface_role_object(struct wlr_xdg_surface *surface); -void xdg_surface_role_commit(struct wlr_surface *wlr_surface); -void xdg_surface_role_unmap(struct wlr_surface *wlr_surface); -void xdg_surface_role_destroy(struct wlr_surface *wlr_surface); + +bool set_xdg_surface_role(struct wlr_xdg_surface *surface, enum wlr_xdg_surface_role role); +void set_xdg_surface_role_object(struct wlr_xdg_surface *surface, + struct wl_resource *role_resource); void create_xdg_positioner(struct wlr_xdg_client *client, uint32_t id); diff --git a/include/wlr/types/wlr_xdg_shell.h b/include/wlr/types/wlr_xdg_shell.h index 9cae226cf..1b15bfe43 100644 --- a/include/wlr/types/wlr_xdg_shell.h +++ b/include/wlr/types/wlr_xdg_shell.h @@ -231,8 +231,18 @@ struct wlr_xdg_surface { struct wl_resource *resource; struct wlr_surface *surface; struct wl_list link; // wlr_xdg_client.surfaces - enum wlr_xdg_surface_role role; + /** + * The lifetime-bound role of the xdg_surface. WLR_XDG_SURFACE_ROLE_NONE + * if the role was never set. + */ + enum wlr_xdg_surface_role role; + /** + * The role object representing the role. NULL if the object was destroyed. + */ + struct wl_resource *role_resource; + + // NULL if the role resource is inert union { struct wlr_xdg_toplevel *toplevel; struct wlr_xdg_popup *popup; @@ -247,8 +257,6 @@ struct wlr_xdg_surface { struct wlr_xdg_surface_state current, pending; - struct wl_listener surface_commit; - struct { struct wl_signal destroy; struct wl_signal ping_timeout; @@ -260,6 +268,11 @@ struct wlr_xdg_surface { } events; void *data; + + // private state + + bool client_mapped; + struct wl_listener role_resource_destroy; }; struct wlr_xdg_toplevel_move_event { diff --git a/types/xdg_shell/wlr_xdg_popup.c b/types/xdg_shell/wlr_xdg_popup.c index 3d9b31a67..99f45d596 100644 --- a/types/xdg_shell/wlr_xdg_popup.c +++ b/types/xdg_shell/wlr_xdg_popup.c @@ -361,13 +361,6 @@ static void xdg_popup_handle_resource_destroy(struct wl_resource *resource) { wlr_xdg_popup_destroy(popup); } -const struct wlr_surface_role xdg_popup_surface_role = { - .name = "xdg_popup", - .commit = xdg_surface_role_commit, - .unmap = xdg_surface_role_unmap, - .destroy = xdg_surface_role_destroy, -}; - void create_xdg_popup(struct wlr_xdg_surface *surface, struct wlr_xdg_surface *parent, struct wlr_xdg_positioner *positioner, uint32_t id) { @@ -379,15 +372,7 @@ void create_xdg_popup(struct wlr_xdg_surface *surface, return; } - if (surface->role != WLR_XDG_SURFACE_ROLE_NONE) { - wl_resource_post_error(surface->resource, - XDG_SURFACE_ERROR_ALREADY_CONSTRUCTED, - "xdg-surface has already been constructed"); - return; - } - - if (!wlr_surface_set_role(surface->surface, &xdg_popup_surface_role, - surface->resource, XDG_WM_BASE_ERROR_ROLE)) { + if (!set_xdg_surface_role(surface, WLR_XDG_SURFACE_ROLE_POPUP)) { return; } @@ -412,8 +397,6 @@ void create_xdg_popup(struct wlr_xdg_surface *surface, &xdg_popup_implementation, surface->popup, xdg_popup_handle_resource_destroy); - wlr_surface_set_role_object(surface->surface, surface->resource); - surface->role = WLR_XDG_SURFACE_ROLE_POPUP; wlr_xdg_positioner_rules_get_geometry( @@ -429,6 +412,8 @@ void create_xdg_popup(struct wlr_xdg_surface *surface, } else { wl_list_init(&surface->popup->link); } + + set_xdg_surface_role_object(surface, surface->popup->resource); } void reset_xdg_popup(struct wlr_xdg_popup *popup) { @@ -460,6 +445,17 @@ void reset_xdg_popup(struct wlr_xdg_popup *popup) { } void destroy_xdg_popup(struct wlr_xdg_popup *popup) { + wlr_surface_unmap(popup->base->surface); + reset_xdg_popup(popup); + + // TODO: improve events + if (popup->base->added) { + wl_signal_emit_mutable(&popup->base->events.destroy, NULL); + popup->base->added = false; + } + + popup->base->popup = NULL; + wl_list_remove(&popup->link); wl_resource_set_user_data(popup->resource, NULL); free(popup); @@ -476,8 +472,7 @@ void wlr_xdg_popup_destroy(struct wlr_xdg_popup *popup) { } xdg_popup_send_popup_done(popup->resource); - wl_resource_set_user_data(popup->resource, NULL); - destroy_xdg_surface_role_object(popup->base); + destroy_xdg_popup(popup); } void wlr_xdg_popup_get_toplevel_coords(struct wlr_xdg_popup *popup, diff --git a/types/xdg_shell/wlr_xdg_shell.c b/types/xdg_shell/wlr_xdg_shell.c index 3e8985d36..157896099 100644 --- a/types/xdg_shell/wlr_xdg_shell.c +++ b/types/xdg_shell/wlr_xdg_shell.c @@ -67,7 +67,7 @@ static void xdg_client_handle_resource_destroy(struct wl_resource *resource) { struct wlr_xdg_surface *surface, *tmp = NULL; wl_list_for_each_safe(surface, tmp, &client->surfaces, link) { - wlr_surface_destroy_role_object(surface->surface); + destroy_xdg_surface(surface); } if (client->ping_timer != NULL) { diff --git a/types/xdg_shell/wlr_xdg_surface.c b/types/xdg_shell/wlr_xdg_surface.c index 38280ad57..da3afa571 100644 --- a/types/xdg_shell/wlr_xdg_surface.c +++ b/types/xdg_shell/wlr_xdg_surface.c @@ -5,18 +5,6 @@ #include #include "types/wlr_xdg_shell.h" -struct wlr_xdg_surface *wlr_xdg_surface_try_from_wlr_surface( - struct wlr_surface *surface) { - if (surface->role != &xdg_toplevel_surface_role && - surface->role != &xdg_popup_surface_role) { - return NULL; - } - if (surface->role_resource == NULL) { - return NULL; - } - return wlr_xdg_surface_from_resource(surface->role_resource); -} - static void xdg_surface_configure_destroy( struct wlr_xdg_surface_configure *configure) { if (configure == NULL) { @@ -27,7 +15,16 @@ static void xdg_surface_configure_destroy( free(configure); } +// An xdg_surface implementation is reset, when: +// 1) a surface is unmapped due to a commit with NULL buffer, or +// 2) the xdg_surface role object is destroyed, or +// 3) wlr_xdg_surface is destroyed +// An xdg_surface role object implementation is reset, when: +// 1) a surface is unmapped due to a commit with NULL buffer, or +// 2) the xdg_surface role object implementation is destroyed + static void reset_xdg_surface(struct wlr_xdg_surface *surface) { + surface->client_mapped = false; surface->configured = false; struct wlr_xdg_popup *popup, *popup_tmp; @@ -35,6 +32,18 @@ static void reset_xdg_surface(struct wlr_xdg_surface *surface) { wlr_xdg_popup_destroy(popup); } + struct wlr_xdg_surface_configure *configure, *tmp; + wl_list_for_each_safe(configure, tmp, &surface->configure_list, link) { + xdg_surface_configure_destroy(configure); + } + + if (surface->configure_idle) { + wl_event_source_remove(surface->configure_idle); + surface->configure_idle = NULL; + } +} + +static void reset_xdg_surface_role_object(struct wlr_xdg_surface *surface) { switch (surface->role) { case WLR_XDG_SURFACE_ROLE_TOPLEVEL: if (surface->toplevel != NULL) { @@ -49,16 +58,6 @@ static void reset_xdg_surface(struct wlr_xdg_surface *surface) { case WLR_XDG_SURFACE_ROLE_NONE: break; } - - struct wlr_xdg_surface_configure *configure, *tmp; - wl_list_for_each_safe(configure, tmp, &surface->configure_list, link) { - xdg_surface_configure_destroy(configure); - } - - if (surface->configure_idle) { - wl_event_source_remove(surface->configure_idle); - surface->configure_idle = NULL; - } } static void xdg_surface_handle_ack_configure(struct wl_client *client, @@ -241,7 +240,7 @@ static void xdg_surface_handle_destroy(struct wl_client *client, return; } - if (surface->role != WLR_XDG_SURFACE_ROLE_NONE) { + if (surface->role_resource != NULL) { wl_resource_post_error(resource, XDG_SURFACE_ERROR_DEFUNCT_ROLE_OBJECT, "surface was destroyed before its role object"); @@ -259,54 +258,48 @@ static const struct xdg_surface_interface xdg_surface_implementation = { .set_window_geometry = xdg_surface_handle_set_window_geometry, }; -static void xdg_surface_handle_resource_destroy(struct wl_resource *resource) { - struct wlr_xdg_surface *surface = - wlr_xdg_surface_from_resource(resource); - if (surface != NULL) { - wlr_surface_destroy_role_object(surface->surface); - } -} +static void xdg_surface_role_commit(struct wlr_surface *wlr_surface) { + struct wlr_xdg_surface *surface = wlr_xdg_surface_try_from_wlr_surface(wlr_surface); + assert(surface != NULL); -static void xdg_surface_handle_surface_commit(struct wl_listener *listener, - void *data) { - struct wlr_xdg_surface *surface = - wl_container_of(listener, surface, surface_commit); - - if (wlr_surface_has_buffer(surface->surface) && !surface->configured) { + if (wlr_surface_has_buffer(wlr_surface) && !surface->configured) { wl_resource_post_error(surface->resource, XDG_SURFACE_ERROR_UNCONFIGURED_BUFFER, "xdg_surface has never been configured"); return; } - // surface->role might be NONE for inert popups - // So we check surface->surface->role - if (surface->surface->role == NULL) { + if (surface->role_resource == NULL) { wl_resource_post_error(surface->resource, XDG_SURFACE_ERROR_NOT_CONSTRUCTED, - "xdg_surface must have a role"); + "xdg_surface must have a role object"); return; } -} -void xdg_surface_role_commit(struct wlr_surface *wlr_surface) { - struct wlr_xdg_surface *surface = wlr_xdg_surface_try_from_wlr_surface(wlr_surface); - assert(surface != NULL); + if (surface->client_mapped && !wlr_surface_has_buffer(wlr_surface)) { + // This commit has unmapped the surface + reset_xdg_surface_role_object(surface); + reset_xdg_surface(surface); + } surface->current = surface->pending; switch (surface->role) { case WLR_XDG_SURFACE_ROLE_NONE: - // inert toplevel or popup + assert(0 && "not reached"); return; case WLR_XDG_SURFACE_ROLE_TOPLEVEL: if (surface->toplevel != NULL) { handle_xdg_toplevel_committed(surface->toplevel); + } else { + return; } break; case WLR_XDG_SURFACE_ROLE_POPUP: if (surface->popup != NULL) { handle_xdg_popup_committed(surface->popup); + } else { + return; } break; } @@ -317,33 +310,43 @@ void xdg_surface_role_commit(struct wlr_surface *wlr_surface) { surface); } - if (surface->configured && wlr_surface_has_buffer(wlr_surface)) { + if (wlr_surface_has_buffer(wlr_surface)) { + surface->client_mapped = true; wlr_surface_map(wlr_surface); } } -void xdg_surface_role_unmap(struct wlr_surface *wlr_surface) { +static void xdg_surface_role_destroy(struct wlr_surface *wlr_surface) { struct wlr_xdg_surface *surface = wlr_xdg_surface_try_from_wlr_surface(wlr_surface); - assert(surface != NULL); + if (surface == NULL) { + // This is the only time xdg_surface can be inert + return; + } - reset_xdg_surface(surface); + destroy_xdg_surface(surface); } -void xdg_surface_role_destroy(struct wlr_surface *wlr_surface) { - struct wlr_xdg_surface *surface = wlr_xdg_surface_try_from_wlr_surface(wlr_surface); - assert(surface != NULL); +static struct wlr_surface_role xdg_surface_role = { + .name = "xdg_surface", + .commit = xdg_surface_role_commit, + .destroy = xdg_surface_role_destroy, +}; - destroy_xdg_surface_role_object(surface); - - wl_list_remove(&surface->link); - wl_list_remove(&surface->surface_commit.link); - - wl_resource_set_user_data(surface->resource, NULL); - free(surface); +struct wlr_xdg_surface *wlr_xdg_surface_try_from_wlr_surface( + struct wlr_surface *surface) { + if (surface->role != &xdg_surface_role || surface->role_resource == NULL) { + return NULL; + } + return wlr_xdg_surface_from_resource(surface->role_resource); } void create_xdg_surface(struct wlr_xdg_client *client, struct wlr_surface *wlr_surface, uint32_t id) { + if (!wlr_surface_set_role(wlr_surface, &xdg_surface_role, client->resource, + XDG_WM_BASE_ERROR_ROLE)) { + return; + } + struct wlr_xdg_surface *surface = calloc(1, sizeof(struct wlr_xdg_surface)); if (surface == NULL) { @@ -381,49 +384,91 @@ void create_xdg_surface(struct wlr_xdg_client *client, struct wlr_surface *wlr_s wl_signal_init(&surface->events.configure); wl_signal_init(&surface->events.ack_configure); - wl_signal_add(&surface->surface->events.commit, - &surface->surface_commit); - surface->surface_commit.notify = xdg_surface_handle_surface_commit; - wlr_log(WLR_DEBUG, "new xdg_surface %p (res %p)", surface, surface->resource); wl_resource_set_implementation(surface->resource, - &xdg_surface_implementation, surface, - xdg_surface_handle_resource_destroy); + &xdg_surface_implementation, surface, NULL); wl_list_insert(&client->surfaces, &surface->link); + + wlr_surface_set_role_object(wlr_surface, surface->resource); } -void destroy_xdg_surface_role_object(struct wlr_xdg_surface *surface) { - if (surface->surface->mapped) { - wlr_surface_unmap(surface->surface); - } else { - reset_xdg_surface(surface); +bool set_xdg_surface_role(struct wlr_xdg_surface *surface, enum wlr_xdg_surface_role role) { + assert(role != WLR_XDG_SURFACE_ROLE_NONE); + + static const char *role_names[] = { + [WLR_XDG_SURFACE_ROLE_TOPLEVEL] = "xdg_toplevel", + [WLR_XDG_SURFACE_ROLE_POPUP] = "xdg_popup", + }; + + if (surface->role != WLR_XDG_SURFACE_ROLE_NONE && surface->role != role) { + wl_resource_post_error(surface->client->resource, XDG_WM_BASE_ERROR_ROLE, + "Cannot assign role %s to xdg_surface@%" PRIu32 ", already has role %s", + role_names[role], wl_resource_get_id(surface->resource), + role_names[surface->role]); + return false; + } + if (surface->role_resource != NULL) { + wl_resource_post_error(surface->client->resource, XDG_WM_BASE_ERROR_ROLE, + "Cannot reassign role %s to xdg_surface@%" PRIu32 ", role object still exists", + role_names[role], wl_resource_get_id(surface->resource)); + return false; } - if (surface->added) { - wl_signal_emit_mutable(&surface->events.destroy, NULL); - surface->added = false; + surface->role = role; + return true; +} + +static void destroy_xdg_surface_role_object(struct wlr_xdg_surface *surface) { + if (surface->role_resource == NULL) { + return; } switch (surface->role) { + case WLR_XDG_SURFACE_ROLE_NONE: + assert(0 && "not reached"); + break; case WLR_XDG_SURFACE_ROLE_TOPLEVEL: if (surface->toplevel != NULL) { destroy_xdg_toplevel(surface->toplevel); - surface->toplevel = NULL; } break; case WLR_XDG_SURFACE_ROLE_POPUP: if (surface->popup != NULL) { destroy_xdg_popup(surface->popup); - surface->popup = NULL; } break; - case WLR_XDG_SURFACE_ROLE_NONE: - // This space is intentionally left blank - break; } - surface->role = WLR_XDG_SURFACE_ROLE_NONE; + surface->role_resource = NULL; + wl_list_remove(&surface->role_resource_destroy.link); + wl_list_init(&surface->role_resource_destroy.link); +} + +static void xdg_surface_handle_role_resource_destroy(struct wl_listener *listener, void *data) { + struct wlr_xdg_surface *surface = wl_container_of(listener, surface, role_resource_destroy); + destroy_xdg_surface_role_object(surface); + reset_xdg_surface(surface); +} + +void set_xdg_surface_role_object(struct wlr_xdg_surface *surface, + struct wl_resource *role_resource) { + assert(surface->role != WLR_XDG_SURFACE_ROLE_NONE); + assert(surface->role_resource == NULL); + assert(role_resource != NULL); + surface->role_resource = role_resource; + surface->role_resource_destroy.notify = xdg_surface_handle_role_resource_destroy; + wl_resource_add_destroy_listener(role_resource, &surface->role_resource_destroy); +} + +void destroy_xdg_surface(struct wlr_xdg_surface *surface) { + destroy_xdg_surface_role_object(surface); + reset_xdg_surface(surface); + + wl_list_remove(&surface->link); + + wl_resource_set_user_data(surface->resource, NULL); + free(surface); } struct wlr_xdg_surface *wlr_xdg_surface_from_resource( diff --git a/types/xdg_shell/wlr_xdg_toplevel.c b/types/xdg_shell/wlr_xdg_toplevel.c index 1e5617a7d..08d316d95 100644 --- a/types/xdg_shell/wlr_xdg_toplevel.c +++ b/types/xdg_shell/wlr_xdg_toplevel.c @@ -469,33 +469,9 @@ static const struct xdg_toplevel_interface xdg_toplevel_implementation = { .set_minimized = xdg_toplevel_handle_set_minimized, }; -static void xdg_toplevel_handle_resource_destroy(struct wl_resource *resource) { - struct wlr_xdg_toplevel *toplevel = - wlr_xdg_toplevel_from_resource(resource); - if (toplevel == NULL) { - return; - } - destroy_xdg_surface_role_object(toplevel->base); -} - -const struct wlr_surface_role xdg_toplevel_surface_role = { - .name = "xdg_toplevel", - .commit = xdg_surface_role_commit, - .unmap = xdg_surface_role_unmap, - .destroy = xdg_surface_role_destroy, -}; - void create_xdg_toplevel(struct wlr_xdg_surface *surface, uint32_t id) { - if (!wlr_surface_set_role(surface->surface, &xdg_toplevel_surface_role, - surface->resource, XDG_WM_BASE_ERROR_ROLE)) { - return; - } - - if (surface->role != WLR_XDG_SURFACE_ROLE_NONE) { - wl_resource_post_error(surface->resource, - XDG_SURFACE_ERROR_ALREADY_CONSTRUCTED, - "xdg-surface has already been constructed"); + if (!set_xdg_surface_role(surface, WLR_XDG_SURFACE_ROLE_TOPLEVEL)) { return; } @@ -527,12 +503,9 @@ void create_xdg_toplevel(struct wlr_xdg_surface *surface, return; } wl_resource_set_implementation(surface->toplevel->resource, - &xdg_toplevel_implementation, surface->toplevel, - xdg_toplevel_handle_resource_destroy); + &xdg_toplevel_implementation, surface->toplevel, NULL); - wlr_surface_set_role_object(surface->surface, surface->resource); - - surface->role = WLR_XDG_SURFACE_ROLE_TOPLEVEL; + set_xdg_surface_role_object(surface, surface->toplevel->resource); } void reset_xdg_toplevel(struct wlr_xdg_toplevel *toplevel) { @@ -557,6 +530,17 @@ void reset_xdg_toplevel(struct wlr_xdg_toplevel *toplevel) { } void destroy_xdg_toplevel(struct wlr_xdg_toplevel *toplevel) { + wlr_surface_unmap(toplevel->base->surface); + + reset_xdg_toplevel(toplevel); + + // TODO: improve events + if (toplevel->base->added) { + wl_signal_emit_mutable(&toplevel->base->events.destroy, NULL); + toplevel->base->added = false; + } + + toplevel->base->toplevel = NULL; wl_resource_set_user_data(toplevel->resource, NULL); free(toplevel); }