[PATCH] xwayland: Fix input coordinates of shadowless CSD windows

[Scott Moreau]

On Thu, May 18, 2017 at 6:26 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> On Tue,  2 May 2017 10:24:17 -0600
> Scott Moreau <oreaus at gmail.com> wrote:
>
> > This fixes wrong input coordinates when using clients like steam,
> > that draw their own decorations and have no shadows.
> > ---
> >  xwayland/window-manager.c | 30 +++++++++++++++++-------------
> >  1 file changed, 17 insertions(+), 13 deletions(-)
> >
> > diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
> > index 2608075..7cb60d1 100644
> > --- a/xwayland/window-manager.c
> > +++ b/xwayland/window-manager.c
> > @@ -610,15 +610,17 @@ weston_wm_window_get_frame_size(struct
> weston_wm_window *window,
> >  {
> >       struct theme *t = window->wm->theme;
> >
> > -     if (window->fullscreen) {
> > +     if (window->decorate) {
> > +             if (window->frame) {
> > +                     *width = frame_width(window->frame);
> > +                     *height = frame_height(window->frame);
> > +             } else {
> > +                     *width = window->width + t->margin * 2;
> > +                     *height = window->height + t->margin * 2;
> > +             }
> > +     } else {
> >               *width = window->width;
> >               *height = window->height;
> > -     } else if (window->decorate && window->frame) {
> > -             *width = frame_width(window->frame);
> > -             *height = frame_height(window->frame);
> > -     } else {
> > -             *width = window->width + t->margin * 2;
> > -             *height = window->height + t->margin * 2;
> >       }
> >  }
> >
> > @@ -628,14 +630,16 @@ weston_wm_window_get_child_position(struct
> weston_wm_window *window,
> >  {
> >       struct theme *t = window->wm->theme;
> >
> > -     if (window->fullscreen) {
> > +     if (window->decorate) {
> > +             if (window->frame)
> > +                     frame_interior(window->frame, x, y, NULL, NULL);
> > +             else {
> > +                     *x = t->margin;
> > +                     *y = t->margin;
> > +             }
> > +     } else {
> >               *x = 0;
> >               *y = 0;
> > -     } else if (window->decorate && window->frame) {
> > -             frame_interior(window->frame, x, y, NULL, NULL);
> > -     } else {
> > -             *x = t->margin;
> > -             *y = t->margin;
> >       }
> >  }
> >
>
> Hi Scott,
>
> thanks for this, the handling of non-decorated, non-fullscreen, non-OR
> windows has probably been broken for a good while if not forever.
>

Hi Pekka,

Thanks for the review.


>
> I tested with google-chrome-browser as that's what I had at hand, both
> with and without system decorations. Without system decorations, this
> patch indeed fixes both the input region (the shadow area was not
> click-through as it should have been) and removes shadows (that I
> assume should not have been there).
>

I'm glad you mentioned this because I hadn't noticed, but this patch wasn't
intended to remove decorations. I have a few other patches I'll submit soon
to fix the input offset problem as well as a couple others I noticed.
Hopefully they fix the input region for things like google-chrome shadow
area problem as you describe.


>
> Is the above exactly what you wanted to say in the commit message, or
> did Steam really get wrong input coordinates, i.e. clicks were off also
> inside the window?
>

The commit message is poor at best; I've updated commit messages in the new
patches.


>
> Does this patch remove unwanted shadows from Steam or was Steam without
> shadows to begin with?
>

It's not meant to remove any shadows. The new patch doesn't adjust size at
all.


>
> The commit message is a little ambiguous on these terms, so I'd like
> (someone) to re-word it.
>
> When using system decorations in Chrome, the decorations look and work
> just fine also after the patch.
>

The thing that doesn't work is toggling the system decoration switch
without having to restart chrome. Also as a side note, moving a window from
its original starting position causes override redirect windows to be
placed as if the window were never moved.


>
> Maximizing xterm works ok.
>
> I'm a little worried about removing the checks for window->fullscreen,
> because e.g. weston_wm_window_draw_decoration() looks at
> window->fullscreen and does nothing. It would feel more logical for
> draw_decoration to use the same conditions as get_frame_size and
> get_child_position. What do you think? Which conditions should those be?
>

Yes, I see what you mean. I left fullscreen logic alone in the new patch.


>
> weston_wm_window_configure(), call arranged from send_configure(), is
> unconditionally using get_frame_size and get_child_position as well.
>
> Also weston_wm_window_set_pending_state() and send_configure() are
> looking at both window->decorate and window->fullscreen. It all
> seems to imply that if _NET_WM_STATE_FULLSCREEN is in effect, then
> decorations should be avoided regardless of the _MOTIF_WM_HINTS
> MWM_DECOR_* flags. Does that make sense?
>
> Looking at weston_wm_window_draw_decoration(), it seems that shadows
> for undecorated non-fullscreen surfaces were actually intentional. If
> we remove that intention, the drawing of the shadow should also be
> removed, and the commit message should explain why that was wrong.
>
> In summary, I would suggest changing
>         if (window->decorate)
> into
>         if (window->decorate && !window->fullscreen)
>
> In fact, testing with e.g. geany, this patch regresses fullscreening: a
> fullscreen X11 window is shifted down with a black bar on top and the
> bottom going out of screen. The same can be seen with xterm.
>
>
> Thanks,
> pq
>

Hopefully you can test the new series and see if there are any regressions
or concerns.


Thanks again,

- Scott
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20170624/44e64d79/attachment.html>