On 2018-03-16 06:42 PM, Scott Moreau wrote:
Hi Pekka,
On Fri, Mar 16, 2018 at 9:20 AM, Pekka Paalanen <ppaala...@gmail.com
<mailto:ppaala...@gmail.com>> wrote:
On Tue, 13 Mar 2018 21:22:04 -0600
Scott Moreau <ore...@gmail.com <mailto:ore...@gmail.com>> wrote:
> Commit 332d1892 introduced a bug because the window was
> shaped only when the frame was created, leaving the input
> region unchanged regardless if the window was resized.
> This patch updates the input region shape on resize,
> fixing the problem.
>
> ---
>
> Changed in v2:
>
> - Bail in shape function if (window->override_redirect || !window->frame)
> - Call shape function from send_configure()
>
> xwayland/window-manager.c | 53
+++++++++++++++++++++++++++++------------------
> 1 file changed, 33 insertions(+), 20 deletions(-)
Hi,
while trying to wrap my head around this, I started feeling dizzy. For
real. So I have to keep this short.
I think this is what happens when trying to sync two display servers.
The first decision we should make is, do we want a quick patch for an
immediate issue at hand, or do we want to make things better in the long
run. Taking in this patch as is seems to be the former to me, and given
the phase of the release cycle can be justified.
The following mind flow is for a long term solution, and the comments
inlined with the code below are for the quick patch.
FWIW, I'd be open to landing something quick at this point. The bug it
fixes seems hugely annoying. I resize an xterm, and I can't click in
the new area.
Alternately, I'm wondering if we should consider reverting the patch
that introduced this bug? I guess it wasn't tested well enough, and
this behaviour seems more painful than what it was intended to fix?
Taking a step back, the aim to keep the input shape up-to-date whenever
something happens.
If we have a frame window with decorations, then we call
frame_resize_inside() to adjust its size. Would it not be logical to
set the input shape in at least all those cases?
Yes, maybe there can be a function that calls frame_resize_inside and
the shape function to replace calls to frame_resize_inside.
Except it looks like we can have decorated O-R windows, and those
should be exempt? Oh, I'm told decorated O-R windows don't make sense,
but I see some code in weston that would only apply in such case...
if (window->override_redirect) { ... if (window->frame)
frame_resize_inside(...)
All windows that go through map_request handler will get the frame
window (window->frame_id) and the frame (window->frame) created. The
only windows that don't get this treatment are therefore windows that
are O-R at the time of mapping them. It's somewhat complicated by the
fact that XWM does not support dynamically changing O-R flag... or
maybe it makes it easier.
Now, we have configure_request and configure_notify handlers. O-R
windows will not hit configure_request handler, and the X server
processes XWM's configure commands immediately. This sounds like
configure_request handler would be a good place to update the input
shape.
Yes
configure_notify handler gets called for O-R as well, and it happens
after the fact, so updating there would be a tiny bit late. Would you
agree?
I was thinking there might be some change that comes in configure notify
that we don't know about until the event happens.
That leaves the XWM originated resizes, which boils down to...
send_configure(), or actually weston_wm_window_configure()?
Yes
It looks like configure_request handler is open-coding all of
weston_wm_window_configure() but it also adds some bits specific to the
configure request.
Am I making sense?
Yes, and thanks for taking the time to try and help unravel this.
> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
> index c307e19..cd72a59 100644
> --- a/xwayland/window-manager.c
> +++ b/xwayland/window-manager.c
> @@ -659,6 +659,33 @@ weston_wm_window_get_input_rect(struct
weston_wm_window *window,
> }
>
> static void
> +weston_wm_window_shape(struct weston_wm_window *window)
> +{
> + struct weston_wm *wm = window->wm;
> + xcb_rectangle_t rect;
> + int x, y, width, height;
> +
> + if (window->override_redirect || !window->frame)
> + return;
> +
> + weston_wm_window_get_input_rect(window, &x, &y, &width,
&height);
> +
> + rect.x = x;
> + rect.y = y;
> + rect.width = width;
> + rect.height = height;
> +
> + /* The window frame was created with position and size
which include
> + * an offset for margins and shadow. Set the input region
to ignore
> + * shadow. */
> + xcb_shape_rectangles(wm->conn,
> + XCB_SHAPE_SO_SET,
> + XCB_SHAPE_SK_INPUT,
> + 0, window->frame_id,
> + 0, 0, 1, &rect);
> +}
> +
> +static void
> weston_wm_window_send_configure_notify(struct weston_wm_window
*window)
> {
> xcb_configure_notify_event_t configure_notify;
> @@ -789,6 +816,8 @@ weston_wm_handle_configure_notify(struct
weston_wm *wm, xcb_generic_event_t *eve
> xwayland_api->set_xwayland(window->shsurf,
> window->x,
window->y);
> }
> +
> + weston_wm_window_shape(window);
From Daniel I understood that there would have been a better place to
call this?
I must have misunderstood.
Since this thread's been quiet for a couple of days, I'll ask:
where should this have gone? :)
What needs to be resolved to move forward on this? Would rather not get
to a weston final with this bug present.
Thanks,
Derek
> }
>
> static void
> @@ -983,7 +1012,6 @@ weston_wm_window_create_frame(struct
weston_wm_window *window)
> {
> struct weston_wm *wm = window->wm;
> uint32_t values[3];
> - xcb_rectangle_t rect;
> int x, y, width, height;
> int buttons = FRAME_BUTTON_CLOSE;
>
> @@ -1040,26 +1068,9 @@ weston_wm_window_create_frame(struct
weston_wm_window *window)
>
&wm->format_rgba,
> width,
height);
>
> - weston_wm_window_get_input_rect(window, &x, &y, &width,
&height);
> - rect.x = x;
> - rect.y = y;
> - rect.width = width;
> - rect.height = height;
> -
> - /* The window frame was created with position and size
which include
> - * an offset for margins and shadow. Set the input region
to ignore
> - * shadow. */
> - xcb_shape_rectangles(wm->conn,
> - XCB_SHAPE_SO_SET,
> - XCB_SHAPE_SK_INPUT,
> - 0,
> - window->frame_id,
> - 0,
> - 0,
> - 1,
> - &rect);
> -
> hash_table_insert(wm->window_hash, window->frame_id, window);
> +
> + weston_wm_window_shape(window);
> }
>
> /*
> @@ -2726,6 +2737,8 @@ send_configure(struct weston_surface
*surface, int32_t width, int32_t height)
> window->configure_source =
> wl_event_loop_add_idle(wm->server->loop,
> weston_wm_window_configure,
window);
> +
> + weston_wm_window_shape(window);
This means we do the shaping immediately, but the configure is postponed
to the idle handler. Shouldn't those go together?
Yes but what are you proposing to do to make them go together, call the
shape function from weston_wm_window_configure() instead?
OTOH, weston_wm_window_configure() is also called from
weston_mw_window_set_toplevel(). To me it seems it would be appropriate
to call weston_wm_window_shape() from weston_wm_window_configure().
Is it?
> }
>
> static void
Thanks,
pq
Thanks,
Scott
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
