[PATCH weston v2] xwm: use always a valid 'primary view' for an X window
Derek Foreman
derekf at osg.samsung.com
Tue Sep 29 14:51:58 PDT 2015
On 23/09/15 12:15 PM, Giulio Camuffo wrote:
> 2015-09-23 19:46 GMT+03:00 Derek Foreman <derekf at osg.samsung.com>:
>> On 01/02/15 08:18 AM, Giulio Camuffo wrote:
>>> The xwm gets a primary view for a X window using the get_primary_view
>>> vfunc of the shell_interface struct. Storing it is dangerous though because
>>> it doesn't listen for its destruction so it may end up using the old stored
>>> view pointer after that view was freed, or after the primary view for that
>>> window was changed to another one.
>>> Fetch the primary view just before using it every time and try to not
>>> abuse this 'primary view' concept which may map badly to some shells:
>>> iterate over all the views instead when it makes sense.
>>> ---
>>
>> Good commit log, makes it obvious that we should be doing this...
>>
>>> v2: check window->shsurf
>>>
>>> xwayland/window-manager.c | 39 +++++++++++++++++++++------------------
>>> 1 file changed, 21 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
>>> index 07c2ef3..be56ffc 100644
>>> --- a/xwayland/window-manager.c
>>> +++ b/xwayland/window-manager.c
>>> @@ -124,7 +124,6 @@ struct weston_wm_window {
>>> uint32_t surface_id;
>>> struct weston_surface *surface;
>>> struct shell_surface *shsurf;
>>> - struct weston_view *view;
>>> struct wl_listener surface_destroy_listener;
>>> struct wl_event_source *repaint_source;
>>> struct wl_event_source *configure_source;
>>> @@ -750,18 +749,27 @@ weston_wm_window_transform(struct wl_listener *listener, void *data)
>>> struct weston_wm_window *window = get_wm_window(surface);
>>> struct weston_wm *wm =
>>> container_of(listener, struct weston_wm, transform_listener);
>>> + struct weston_view *view;
>>> + struct weston_shell_interface *shell_interface =
>>> + &wm->server->compositor->shell_interface;
>>> uint32_t mask, values[2];
>>>
>>> - if (!window || !wm)
>>> + if (!window || !wm || !window->shsurf)
>>> return;
>>>
>>> - if (!window->view || !weston_view_is_mapped(window->view))
>>> + if (!shell_interface->get_primary_view)
>>>
>>> return;
>>>
>>> - if (window->x != window->view->geometry.x ||
>>> - window->y != window->view->geometry.y) {
>>> - values[0] = window->view->geometry.x;
>>> - values[1] = window->view->geometry.y;
>>> + view = shell_interface->get_primary_view(shell_interface->shell,
>>> + window->shsurf);
>>> +
>>> + if (!view || !weston_view_is_mapped(view))
>>> + return;
>>> +
>>> + if (window->x != view->geometry.x ||
>>> + window->y != view->geometry.y) {
>>> + values[0] = view->geometry.x;
>>> + values[1] = view->geometry.y;
>>> mask = XCB_CONFIG_WINDOW_X | XCB_CONFIG_WINDOW_Y;
>>>
>>> xcb_configure_window(wm->conn, window->frame_id, mask, values);
>>> @@ -972,7 +980,6 @@ weston_wm_handle_unmap_notify(struct weston_wm *wm, xcb_generic_event_t *event)
>>> wl_list_remove(&window->surface_destroy_listener.link);
>>> window->surface = NULL;
>>> window->shsurf = NULL;
>>> - window->view = NULL;
>>>
>>> weston_wm_window_set_wm_state(window, ICCCM_WITHDRAWN_STATE);
>>> weston_wm_window_set_virtual_desktop(window, -1);
>>> @@ -992,6 +999,7 @@ weston_wm_window_draw_decoration(void *data)
>>> struct weston_shell_interface *shell_interface =
>>> &wm->server->compositor->shell_interface;
>>> uint32_t flags = 0;
>>> + struct weston_view *view;
>>>
>>> weston_wm_window_read_properties(window);
>>>
>>> @@ -1033,8 +1041,8 @@ weston_wm_window_draw_decoration(void *data)
>>> window->width + 2,
>>> window->height + 2);
>>> }
>>> - if (window->view)
>>> - weston_view_geometry_dirty(window->view);
>>> + wl_list_for_each(view, &window->surface->views, surface_link)
>>> + weston_view_geometry_dirty(view);
>>
>> Is this an unrelated functional change? Previously we only dirtied the
>> primary view, now we dirty all views?
>>
>>>
>>> pixman_region32_fini(&window->surface->pending.input);
>>>
>>> @@ -1060,6 +1068,7 @@ static void
>>> weston_wm_window_schedule_repaint(struct weston_wm_window *window)
>>> {
>>> struct weston_wm *wm = window->wm;
>>> + struct weston_view *view;
>>> int width, height;
>>>
>>> if (window->frame_id == XCB_WINDOW_NONE) {
>>> @@ -1072,8 +1081,8 @@ weston_wm_window_schedule_repaint(struct weston_wm_window *window)
>>> pixman_region32_init_rect(&window->surface->pending.opaque, 0, 0,
>>> width, height);
>>> }
>>> - if (window->view)
>>> - weston_view_geometry_dirty(window->view);
>>> + wl_list_for_each(view, &window->surface->views, surface_link)
>>> + weston_view_geometry_dirty(view);
>>
>> Same as above...
>>
>> It seems like these two hunks are actually a separate bug fix? Or is
>> this because some shells might not have a get_primary_view? Either way,
>> it's a functional change from before?
>
> That's the "and try to not abuse this 'primary view' concept which may
> map badly to some shells: iterate over all the views instead when it
> makes sense." part of the commit message.
> I guess it may be a bug fix in some cases but the main reason was to
> reduce the usage of the primary view. I can split it out though.
Maybe I'm just being silly, it's not like landing the two parts
separately would ever let us back out just the functional change, since
the other bit depends on it.
Reviewed-by: Derek Foreman <derekf at osg.samsung.com>
> Thanks,
> Giulio
>
>>
>> I'd like to see this as two patches, the one that's removing the view
>> caching is already
>> Reviewed-by: Derek Foreman <derekf at osg.samsung.com>
>>
>> (CC me and I'll review the other quickly, I just want to see a commit
>> log and understand the change)
>>
>>> }
>>> return;
>>> }
>>> @@ -1430,7 +1439,6 @@ surface_destroy(struct wl_listener *listener, void *data)
>>> * Don't try to use it later. */
>>> window->shsurf = NULL;
>>> window->surface = NULL;
>>> - window->view = NULL;
>>> }
>>>
>>> static void
>>> @@ -2409,9 +2417,6 @@ xserver_map_shell_surface(struct weston_wm_window *window,
>>> if (!shell_interface->create_shell_surface)
>>> return;
>>>
>>> - if (!shell_interface->get_primary_view)
>>> - return;
>>> -
>>> if (window->surface->configure) {
>>> weston_log("warning, unexpected in %s: "
>>> "surface's configure hook is already set.\n",
>>> @@ -2423,8 +2428,6 @@ xserver_map_shell_surface(struct weston_wm_window *window,
>>> shell_interface->create_shell_surface(shell_interface->shell,
>>> window->surface,
>>> &shell_client);
>>> - window->view = shell_interface->get_primary_view(shell_interface->shell,
>>> - window->shsurf);
>>>
>>> if (window->name)
>>> shell_interface->set_title(window->shsurf, window->name);
>>>
>>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
More information about the wayland-devel
mailing list