[v2] xwm: Update input region on resize

Submitted by Scott Moreau on March 14, 2018, 3:22 a.m.

Details

Message ID 1520997724-30337-1-git-send-email-oreaus@gmail.com
State New
Headers show
Series "xwm: Update input region on resize" ( rev: 2 ) in Wayland

Not browsing as part of any series.

Commit Message

Scott Moreau March 14, 2018, 3:22 a.m.
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(-)

Patch hide | download patch | download mbox

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);
 }
 
 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);
 }
 
 static void

Comments

On Tue, 13 Mar 2018 21:22:04 -0600
Scott Moreau <oreaus@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.

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.


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?

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. 

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?

That leaves the XWM originated resizes, which boils down to...
send_configure(), or actually weston_wm_window_configure()?

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?

> 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?

>  }
>  
>  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?

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
Hi Pekka,

On Fri, Mar 16, 2018 at 9:20 AM, Pekka Paalanen <ppaalanen@gmail.com> wrote:

> On Tue, 13 Mar 2018 21:22:04 -0600
> Scott Moreau <oreaus@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.
>
>
> 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.


>
> >  }
> >
> >  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
On 2018-03-16 06:42 PM, Scott Moreau wrote:
> Hi Pekka,
> 
> On Fri, Mar 16, 2018 at 9:20 AM, Pekka Paalanen <ppaalanen@gmail.com 
> <mailto:ppaalanen@gmail.com>> wrote:
> 
>     On Tue, 13 Mar 2018 21:22:04 -0600
>     Scott Moreau <oreaus@gmail.com <mailto:oreaus@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
>
On Mon, Mar 19, 2018 at 1:13 PM, Derek Foreman <derekf@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@gmail.com
>> <mailto:ppaalanen@gmail.com>> wrote:
>>
>>     On Tue, 13 Mar 2018 21:22:04 -0600
>>     Scott Moreau <oreaus@gmail.com <mailto:oreaus@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 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
>>
>>
>
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).

Thanks,
Derek

On 2018-03-19 03:20 PM, Scott Moreau wrote:
> 
> 
> On Mon, Mar 19, 2018 at 1:13 PM, Derek Foreman <derekf@osg.samsung.com
> <mailto:derekf@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@gmail.com <mailto:ppaalanen@gmail.com>
>         <mailto:ppaalanen@gmail.com <mailto:ppaalanen@gmail.com>>> wrote:
> 
>             On Tue, 13 Mar 2018 21:22:04 -0600
>             Scott Moreau <oreaus@gmail.com <mailto:oreaus@gmail.com>
>         <mailto:oreaus@gmail.com <mailto:oreaus@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 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
>         <mailto:wayland-devel@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>