[PATCH v2] xwm: Update input region on resize
Derek Foreman
derekf at osg.samsung.com
Wed Mar 28 14:49:33 UTC 2018
On 2018-03-28 12:55 AM, Ray, Ian (GE Healthcare) wrote:
> On 27/03/2018, 21.50, "wayland-devel on behalf of Derek Foreman" <wayland-devel-bounces at lists.freedesktop.org on behalf of derekf at osg.samsung.com> wrote:
>>
>> Hey everyone,
>>
>> I've added Ian Ray to CC as the author of commit 332d1892bbb (xwm: do
>> not include shadow in input region)
>>
>> I think at this point reverting that patch for release is our most
>> sensible move. We don't seem to have much forward progress on the
>> horrible bug it's introduced. (Resizing an xwayland client doesn't
>> update input region)
>>
>> If there are no fixes forthcoming I'd like to revert commit 332d1892bbb
>> before RC1 (which is still scheduled for release on Monday, April 2).
>
> Agreed.
>
> (FWIW, my submission* was marked RFC, and as such was not as thoroughly
> tested as it should have been.)
I can see why it landed, it looks trivially correct to me too. :/
> [*] https://lists.freedesktop.org/archives/wayland-devel/2017-December/036402.html
Thanks for this - I couldn't find the thread because the commit landed
with a revised name. I wasn't sure how to produce the bug it was
intended to fix.
I've reverted the patch for now. Hopefully it can be revisited after
release with input regions handled on window resize (as the patch in
this thread intends to do).
Thanks,
Derek
>
>> Thanks,
>> Derek
>>
>> On 2018-03-19 03:20 PM, Scott Moreau wrote:
>>>
>>>
>>> On Mon, Mar 19, 2018 at 1:13 PM, Derek Foreman <derekf at osg.samsung.com
>>> <mailto:derekf at osg.samsung.com>> wrote:
>>>
>>> On 2018-03-16 06:42 PM, Scott Moreau wrote:
>>>
>>> Hi Pekka,
>>>
>>> On Fri, Mar 16, 2018 at 9:20 AM, Pekka Paalanen
>>> <ppaalanen at gmail.com <mailto:ppaalanen at gmail.com>
>>> <mailto:ppaalanen at gmail.com <mailto:ppaalanen at gmail.com>>> wrote:
>>>
>>> On Tue, 13 Mar 2018 21:22:04 -0600
>>> Scott Moreau <oreaus at gmail.com <mailto:oreaus at gmail.com>
>>> <mailto:oreaus at gmail.com <mailto:oreaus at 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?
>>>
>>>
>>> I think a revert would be a good place to start. The patch also has
>>> whitespace that does not match surrounding code.
>>>
>>> To clarify the bug, start an xwayland window and resize it to a larger
>>> dimension. Mouse input will only work for the size of the window before
>>> resize. The rest of the window does not respond to input.
>>>
>>> Thanks,
>>> Scott
>>>
>>>
>>>
>>>
>>>
>>> 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 w edo 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 at lists.freedesktop.org
>>> <mailto:wayland-devel at lists.freedesktop.org>
>>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>>> <https://lists.freedesktop.org/mailman/listinfo/wayland-devel>
>>>
>>>
>>>
>>>
>>>
>>> _______________________________________________
>>> wayland-devel mailing list
>>> wayland-devel at lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>>>
>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>>
>
>
>
More information about the wayland-devel
mailing list