[PATCH v2] xwm: Update input region on resize
Ray, Ian (GE Healthcare)
ian.ray at ge.com
Wed Mar 28 05:55:45 UTC 2018
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.)
[*] https://lists.freedesktop.org/archives/wayland-devel/2017-December/036402.html
> 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