[Spice-devel] [RFC/POC PATCH spice-gtk 12/16] Use the new output_id as display ID for the mouse motion event

[Jonathon Jongsma]

On Tue, 2018-06-05 at 17:30 +0200, Lukáš Hrázký wrote:
> If the output_id is set, it is the true xrandr guest ID to use for
> the
> mouse motion event. If it's not present, keep relying on the wacky
> sequence of IDs of channel_id + monitor_id.
> ---
>  src/spice-widget.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/src/spice-widget.c b/src/spice-widget.c
> index 72f5334..cdda8db 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -220,6 +220,12 @@ static void update_keyboard_focus(SpiceDisplay
> *display, gboolean state)
>      spice_gtk_session_request_auto_usbredir(d->gtk_session, state);
>  }
>  
> +/*
> + * This function assumes either channel_id or monitor_id is always
> 0. Under
> + * this assumption, the code is equal to channel_id + monitor_id.
> Therefore the
> + * result can be used with
> spice_main_channel_update_display{,_enabled}()
> + * functions.
> + */
>  static gint get_display_id(SpiceDisplay *display)
>  {
>      SpiceDisplayPrivate *d = display->priv;
> @@ -233,6 +239,28 @@ static gint get_display_id(SpiceDisplay
> *display)
>      return d->channel_id;
>  }
>  
> +/*
> + * Use output ID for mouse input if it is present in the monitor
> config. It is
> + * the correct ID from the guest, set by the streaming agent for
> streaming
> + * channels. If we don't find it, fall back to the old way of
> channel_id +
> + * monitor_id, which, under the assumptions we make, should be a
> sequence
> + * starting from 0 and with no collisions.
> + */
> +static gint get_mouse_input_display_id(SpiceDisplay *display)
> +{
> +    SpiceDisplayPrivate *d = display->priv;
> +
> +    SpiceMonitorConfig *mc = spice_session_find_monitor_config(d-
> >session, d->channel_id, d->monitor_id);
> +
> +    if (mc && mc->output_id) {
> +        // output IDs are a sequence starting from 1 (0 meaning it
> is unset),
> +        // but the mouse motion event expects zero-based IDs
> +        return mc->output_id - 1;
> +    }
> +
> +    return get_display_id(display);
> +}

So, this function here seems to leak implementation details into the
client. When we receive a monitors config message from the server, we
will receive an output_id. It seems to me that this output id should be
opaque to the client. The client should be able to use the id that it
received from the server without modification. It should not need to
know that the implementation of server requires us to subtract 1 from
this value to get the actual ID. 

It also introduces an inconsistency between mouse input and monitor
configuration. In a later patch (11/16), you use the output_id
unmodified in the monitors_config message. But here you subtract 1 from
it before sending it on. So an ID of N in a MousePosition message is
equal to an ID of N+1 in a MonitorsConfig message. That seems
confusing.

Jonathon

> +
>  static bool egl_enabled(SpiceDisplayPrivate *d)
>  {
>  #if HAVE_EGL
> @@ -1980,7 +2008,7 @@ static gboolean motion_event(GtkWidget *widget,
> GdkEventMotion *motion)
>      case SPICE_MOUSE_MODE_CLIENT:
>          if (x >= 0 && x < d->area.width &&
>              y >= 0 && y < d->area.height) {
> -            spice_inputs_channel_position(d->inputs, x, y,
> get_display_id(display),
> +            spice_inputs_channel_position(d->inputs, x, y,
> get_mouse_input_display_id(display),
>                                            button_mask_gdk_to_spice(m
> otion->state));
>          }
>          break;