On 2018-03-29 09:10 AM, Derek Foreman wrote: > On 2018-03-29 08:59 AM, Derek Foreman wrote: >> This reverts commit bef761796c2ada6344d227142af4a0f40b9760dd. >> This (partially) reverts commit 4d1cd36c9ea041688f92cd8981e43b5fe3b52409. >> - the frame_destroy in weston_wm_window_destroy() remains >> This reverts commit 44fc1be913ab2faad0414f50e51d58310302d065. >> This (partially) reverts commit e277276b850bad39ed6995be5a82f24aa6b17bf1. >> - only the custom icon bits are removed >> This reverts commit 6b58ea8c43ac81e519bd418efbf24687a5d731b8. >> >> The new xwm icon code has proven to be leaky and incomplete, and while >> we have patches under consideration to fix the rest of its known problems >> they still require changes and review cycles. Reverting this all for now >> for the upcoming release. >> > > Perhaps I should've been more clear as to what "incomplete" means in > this commit log. > > The current code will pick the first available icon unconditionally, > regardless as to whether this fits on the titlebar, and no scaling is done. > > So, as an example, here running terminology under xwayland will result > in picking a 128x128 icon, and drawing it as a 16 high 128 wide piece of > the icon on the title bar. when the window closes for some reason the > whole icon appears during fade out. > > It all looks pretty embarrassing. > > That said, all the known leaks have been fixed, it's just visually > disappointing.
Quentin has suggested on IRC that it might be better to just land the xwayland/window-manager.c parts of this revert and keep the rest. I'd like to see if anyone else has opinions before I resubmit half this patch for review. Thanks, Derek > Thanks, > Derek > >> Signed-off-by: Derek Foreman <[email protected]> >> --- >> clients/window.c | 4 +- >> libweston/compositor-wayland.c | 2 +- >> shared/cairo-util.h | 6 +-- >> shared/frame.c | 76 +++++++++--------------------------- >> xwayland/window-manager.c | 88 >> +----------------------------------------- >> 5 files changed, 24 insertions(+), 152 deletions(-) >> >> diff --git a/clients/window.c b/clients/window.c >> index bcf2b017..82fbd572 100644 >> --- a/clients/window.c >> +++ b/clients/window.c >> @@ -2523,7 +2523,7 @@ window_frame_create(struct window *window, void *data) >> >> frame = xzalloc(sizeof *frame); >> frame->frame = frame_create(window->display->theme, 0, 0, >> - buttons, window->title, NULL); >> + buttons, window->title); >> >> frame->widget = window_add_widget(window, frame); >> frame->child = widget_add_widget(frame->widget, data); >> @@ -5398,7 +5398,7 @@ create_menu(struct display *display, >> menu->user_data = user_data; >> menu->widget = window_add_widget(menu->window, menu); >> menu->frame = frame_create(window->display->theme, 0, 0, >> - FRAME_BUTTON_NONE, NULL, NULL); >> + FRAME_BUTTON_NONE, NULL); >> fail_on_null(menu->frame, 0, __FILE__, __LINE__); >> menu->entries = entries; >> menu->count = count; >> diff --git a/libweston/compositor-wayland.c b/libweston/compositor-wayland.c >> index 111c4c09..5629b7f4 100644 >> --- a/libweston/compositor-wayland.c >> +++ b/libweston/compositor-wayland.c >> @@ -869,7 +869,7 @@ wayland_output_set_windowed(struct wayland_output >> *output) >> return -1; >> } >> output->frame = frame_create(b->theme, 100, 100, >> - FRAME_BUTTON_CLOSE, output->title, NULL); >> + FRAME_BUTTON_CLOSE, output->title); >> if (!output->frame) >> return -1; >> >> diff --git a/shared/cairo-util.h b/shared/cairo-util.h >> index 6fd11f6b..9481e58c 100644 >> --- a/shared/cairo-util.h >> +++ b/shared/cairo-util.h >> @@ -126,7 +126,7 @@ enum { >> >> struct frame * >> frame_create(struct theme *t, int32_t width, int32_t height, uint32_t >> buttons, >> - const char *title, cairo_surface_t *icon); >> + const char *title); >> >> void >> frame_destroy(struct frame *frame); >> @@ -135,10 +135,6 @@ frame_destroy(struct frame *frame); >> int >> frame_set_title(struct frame *frame, const char *title); >> >> -/* May set FRAME_STATUS_REPAINT */ >> -void >> -frame_set_icon(struct frame *frame, cairo_surface_t *icon); >> - >> /* May set FRAME_STATUS_REPAINT */ >> void >> frame_set_flag(struct frame *frame, enum frame_flag flag); >> diff --git a/shared/frame.c b/shared/frame.c >> index e8a5cad6..83bd7922 100644 >> --- a/shared/frame.c >> +++ b/shared/frame.c >> @@ -109,9 +109,9 @@ struct frame { >> }; >> >> static struct frame_button * >> -frame_button_create_from_surface(struct frame *frame, cairo_surface_t *icon, >> - enum frame_status status_effect, >> - enum frame_button_flags flags) >> +frame_button_create(struct frame *frame, const char *icon, >> + enum frame_status status_effect, >> + enum frame_button_flags flags) >> { >> struct frame_button *button; >> >> @@ -119,7 +119,12 @@ frame_button_create_from_surface(struct frame *frame, >> cairo_surface_t *icon, >> if (!button) >> return NULL; >> >> - button->icon = icon; >> + button->icon = cairo_image_surface_create_from_png(icon); >> + if (!button->icon) { >> + free(button); >> + return NULL; >> + } >> + >> button->frame = frame; >> button->flags = flags; >> button->status_effect = status_effect; >> @@ -129,30 +134,6 @@ frame_button_create_from_surface(struct frame *frame, >> cairo_surface_t *icon, >> return button; >> } >> >> -static struct frame_button * >> -frame_button_create(struct frame *frame, const char *icon_name, >> - enum frame_status status_effect, >> - enum frame_button_flags flags) >> -{ >> - struct frame_button *button; >> - cairo_surface_t *icon; >> - >> - icon = cairo_image_surface_create_from_png(icon_name); >> - if (cairo_surface_status(icon) != CAIRO_STATUS_SUCCESS) >> - goto error; >> - >> - button = frame_button_create_from_surface(frame, icon, status_effect, >> - flags); >> - if (!button) >> - goto error; >> - >> - return button; >> - >> -error: >> - cairo_surface_destroy(icon); >> - return NULL; >> -} >> - >> static void >> frame_button_destroy(struct frame_button *button) >> { >> @@ -325,7 +306,7 @@ frame_destroy(struct frame *frame) >> >> struct frame * >> frame_create(struct theme *t, int32_t width, int32_t height, uint32_t >> buttons, >> - const char *title, cairo_surface_t *icon) >> + const char *title) >> { >> struct frame *frame; >> struct frame_button *button; >> @@ -352,23 +333,16 @@ frame_create(struct theme *t, int32_t width, int32_t >> height, uint32_t buttons, >> } >> >> if (title) { >> - if (icon) { >> - button = frame_button_create_from_surface(frame, >> - icon, >> - >> FRAME_STATUS_MENU, >> - >> FRAME_BUTTON_CLICK_DOWN); >> - } else { >> - char *name = file_name_with_datadir("icon_window.png"); >> + char *name = file_name_with_datadir("icon_window.png"); >> + if (!name) >> + goto free_frame; >> >> - if (!name) >> - goto free_frame; >> + button = frame_button_create(frame, >> + name, >> + FRAME_STATUS_MENU, >> + FRAME_BUTTON_CLICK_DOWN); >> + free(name); >> >> - button = frame_button_create(frame, >> - name, >> - FRAME_STATUS_MENU, >> - FRAME_BUTTON_CLICK_DOWN); >> - free(name); >> - } >> if (!button) >> goto free_frame; >> } >> @@ -448,20 +422,6 @@ frame_set_title(struct frame *frame, const char *title) >> return 0; >> } >> >> -void >> -frame_set_icon(struct frame *frame, cairo_surface_t *icon) >> -{ >> - struct frame_button *button; >> - wl_list_for_each(button, &frame->buttons, link) { >> - if (button->status_effect != FRAME_STATUS_MENU) >> - continue; >> - if (button->icon) >> - cairo_surface_destroy(button->icon); >> - button->icon = icon; >> - frame->status |= FRAME_STATUS_REPAINT; >> - } >> -} >> - >> void >> frame_set_flag(struct frame *frame, enum frame_flag flag) >> { >> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c >> index 62087941..6f5c3443 100644 >> --- a/xwayland/window-manager.c >> +++ b/xwayland/window-manager.c >> @@ -138,8 +138,6 @@ struct weston_wm_window { >> xcb_window_t frame_id; >> struct frame *frame; >> cairo_surface_t *cairo_surface; >> - int icon; >> - cairo_surface_t *icon_surface; /* A temporary slot, to be passed to >> frame on creation */ >> uint32_t surface_id; >> struct weston_surface *surface; >> struct weston_desktop_xwayland_surface *shsurf; >> @@ -475,7 +473,6 @@ weston_wm_window_read_properties(struct weston_wm_window >> *window) >> { wm->atom.net_wm_state, TYPE_NET_WM_STATE, NULL >> }, >> { wm->atom.net_wm_window_type, XCB_ATOM_ATOM, >> F(type) }, >> { wm->atom.net_wm_name, XCB_ATOM_STRING, >> F(name) }, >> - { wm->atom.net_wm_icon, XCB_ATOM_CARDINAL, >> F(icon) }, >> { wm->atom.net_wm_pid, XCB_ATOM_CARDINAL, >> F(pid) }, >> { wm->atom.motif_wm_hints, TYPE_MOTIF_WM_HINTS, NULL >> }, >> { wm->atom.wm_client_machine, XCB_ATOM_WM_CLIENT_MACHINE, >> F(machine) }, >> @@ -976,10 +973,8 @@ weston_wm_window_create_frame(struct weston_wm_window >> *window) >> buttons |= FRAME_BUTTON_MAXIMIZE; >> >> window->frame = frame_create(window->wm->theme, >> - window->width, window->height, >> - buttons, window->name, >> - window->icon_surface); >> - window->icon_surface = NULL; >> + window->width, window->height, >> + buttons, window->name); >> frame_resize_inside(window->frame, window->width, window->height); >> >> weston_wm_window_get_frame_size(window, &width, &height); >> @@ -1318,70 +1313,6 @@ weston_wm_window_schedule_repaint(struct >> weston_wm_window *window) >> weston_wm_window_do_repaint, window); >> } >> >> -static void >> -handle_icon_surface_destroy(void *data) >> -{ >> - free(data); >> -} >> - >> -static void >> -weston_wm_handle_icon(struct weston_wm *wm, struct weston_wm_window *window) >> -{ >> - xcb_get_property_reply_t *reply; >> - xcb_get_property_cookie_t cookie; >> - uint32_t length; >> - uint32_t *data, width, height; >> - cairo_surface_t *new_surface; >> - >> - /* TODO: icons don’t have any specified order, we should pick the >> - * closest one to the target dimension instead of the first one. */ >> - >> - cookie = xcb_get_property(wm->conn, 0, window->id, >> - wm->atom.net_wm_icon, XCB_ATOM_ANY, 0, >> - UINT32_MAX); >> - reply = xcb_get_property_reply(wm->conn, cookie, NULL); >> - length = xcb_get_property_value_length(reply); >> - >> - /* This is in 32-bit words, not in bytes. */ >> - if (length < 2) { >> - free(reply); >> - return; >> - } >> - >> - data = xcb_get_property_value(reply); >> - width = *data++; >> - height = *data++; >> - >> - /* Some checks against malformed input. */ >> - if (width == 0 || height == 0 || length < 2 + width * height) { >> - free(reply); >> - return; >> - } >> - >> - new_surface = >> - cairo_image_surface_create_for_data((unsigned char *)data, >> - CAIRO_FORMAT_ARGB32, >> - width, height, width * 4); >> - >> - /* Bail out in case anything wrong happened during surface creation. */ >> - if (cairo_surface_status(new_surface) != CAIRO_STATUS_SUCCESS) { >> - cairo_surface_destroy(new_surface); >> - free(reply); >> - return; >> - } >> - >> - if (window->icon_surface) >> - cairo_surface_destroy(window->icon_surface); >> - >> - cairo_surface_set_user_data(new_surface, NULL, reply, >> - &handle_icon_surface_destroy); >> - >> - if (window->frame) >> - frame_set_icon(window->frame, new_surface); >> - else /* We don’t have a frame yet */ >> - window->icon_surface = new_surface; >> -} >> - >> static void >> weston_wm_handle_property_notify(struct weston_wm *wm, xcb_generic_event_t >> *event) >> { >> @@ -1402,19 +1333,6 @@ weston_wm_handle_property_notify(struct weston_wm >> *wm, xcb_generic_event_t *even >> read_and_dump_property(wm, property_notify->window, >> property_notify->atom); >> >> - if (property_notify->atom == wm->atom.net_wm_icon) { >> - if (property_notify->state != XCB_PROPERTY_DELETE) { >> - weston_wm_handle_icon(wm, window); >> - } else { >> - if (window->frame) >> - frame_set_icon(window->frame, NULL); >> - if (window->icon_surface) >> - cairo_surface_destroy(window->icon_surface); >> - window->icon_surface = NULL; >> - } >> - weston_wm_window_schedule_repaint(window); >> - } >> - >> if (property_notify->atom == wm->atom.net_wm_name || >> property_notify->atom == XCB_ATOM_WM_NAME) >> weston_wm_window_schedule_repaint(window); >> @@ -1475,8 +1393,6 @@ weston_wm_window_destroy(struct weston_wm_window >> *window) >> wl_event_source_remove(window->repaint_source); >> if (window->cairo_surface) >> cairo_surface_destroy(window->cairo_surface); >> - if (window->icon_surface) >> - cairo_surface_destroy(window->icon_surface); >> >> if (window->frame_id) { >> xcb_reparent_window(wm->conn, window->id, wm->wm_window, 0, 0); >> > > _______________________________________________ > wayland-devel mailing list > [email protected] > https://lists.freedesktop.org/mailman/listinfo/wayland-devel > _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
