[PATCH xserver v2] xwayland: rotate logical size for RRMode

[Olivier Fourdan]

Hi,

Thanks for your follow up on that!

On Fri, Jul 13, 2018 at 9:51 PM Simon Ser <contact at emersion.fr> wrote:
>
> From: emersion <contact at emersion.fr>
>
> The logical size is the size of the output in the global compositor
> space. The mode width/height should be scaled as in the logical
> size, but shouldn't be transformed. Thus we need to rotate back
> the logical size to be able to use it as the mode width/height.
>
> This fixes issues with pointer input on transformed outputs.
>
> Signed-Off-By: Simon Ser <contact at emersion.fr>
> ---
>
> Changes from v1 to v2:
> - Fixed rotation when xdg-output isn't available
>
> I've made sure this works on both rootston (with xdg-output) and
> Weston (without xdg-output).
>
>  hw/xwayland/xwayland-output.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
> index 379062549..0d2ec7890 100644
> --- a/hw/xwayland/xwayland-output.c
> +++ b/hw/xwayland/xwayland-output.c
> @@ -213,6 +213,7 @@ apply_output_change(struct xwl_output *xwl_output)
>  {
>      struct xwl_screen *xwl_screen = xwl_output->xwl_screen;
>      struct xwl_output *it;
> +    int mode_width, mode_height;
>      int width = 0, height = 0, has_this_output = 0;
>      RRModePtr randr_mode;
>      Bool need_rotate;
> @@ -224,7 +225,16 @@ apply_output_change(struct xwl_output *xwl_output)
>      /* xdg-output sends output size in compositor space. so already rotated */
>      need_rotate = (xwl_output->xdg_output == NULL);
>
> -    randr_mode = xwayland_cvt(xwl_output->width, xwl_output->height,
> +    /* We need to rotate back the logical size for the mode */
> +    if (need_rotate || xwl_output->rotation & (RR_Rotate_0 | RR_Rotate_180)) {

But is it `need_rotate` or `!need_rotate` here?

`need_rotate` being `TRUE` means we don't have xdg-output available,
in which case we shouldn't have to do this. Basically, we need to
revert to the original width/height only if we have xdg-output (which
has already applied the rotation), so I reckon it should be
`!need_rotate` here.

> +        mode_width = xwl_output->width;
> +        mode_height = xwl_output->height;
> +    } else {
> +        mode_width = xwl_output->height;
> +        mode_height = xwl_output->width;
> +    }

So if we use `(!need_rotate)`, this addition becomes completely
similar to the code found in `output_get_new_size()` it might be
interesting to move that to a small helper function, e.g.
`output_get_transform_size()` that would return the swapped
width/height depending on the output rotation, so we don't duplicate
that small portion of code. Nit picking, I know :)

> +
> +    randr_mode = xwayland_cvt(mode_width, mode_height,
>                                xwl_output->refresh / 1000.0, 0, 0);
>      RROutputSetModes(xwl_output->randr_output, &randr_mode, 1, 1);
>      RRCrtcNotify(xwl_output->randr_crtc, randr_mode,
> --
> 2.18.0

Also, this is something I noticed the hard way some time ago
(completely unrelated to your patch, though), the width/height
parameters for `output_get_new_size()` are inverted (`height, width`
instead of `width, height` as usual) which is error prone (don't ask
how I noticed...).

I can't see a reason for this everywhere else in the code we use
`width, height`, I reckon it would be great if we could fix that (in a
separate patch) as well, while at it... Because things are already
complicated enough that we don't need to add more confusion by
changing the well established order of parameters just for the sake of
it :)

Cheers,
Olivier