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

[Christophe de Dinechin]

I thought I had already commented on this one, but can’t find my comments anywhere. Sorry if duplicate.


> On 5 Jun 2018, at 17:30, Lukáš Hrázký <lhrazky at redhat.com> 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.

I would avoid using “wacky”

Probably want to add a comment explaining what other pieces of the series deal with the mouse motion event, and why you don’t need that in server mode.

> ---
> 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.
> + */

If this is really the input precondition, I would add an assertion to that effect.

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

Don’t like that much, to be honest, even with a comment. I find it all the more confusing that it’s called a “display_id” in the function name and argument name. Why would motion events expect a zero-based ID if you are in the “new” case?

Meta: Is there such a thing as iditis, i.e. a disease that happens when you have too many ids? I suggest we call some of them “universal” IDs to solve the problem ;-)


> +    }
> +
> +    return get_display_id(display);
> +}
> +
> 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(motion->state));
>         }
>         break;
> -- 
> 2.17.1
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel