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

[Lukáš Hrázký]

On Tue, 2018-06-19 at 17:25 +0200, Christophe de Dinechin wrote:
> 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.

Ok.

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

Yeah, I can add that.

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

I'm reusing the current protocol here and basically abusing the fact
that the motion event expects zero-based sequence of IDs (it's actually
in effect used as the xrandr output ID) and that's what my output_id
is. Except I shifted it (by +1) to one-based in the streaming agent, so
that I can have 0 mean the output_id is unset.

Yeah, I agree this should be changed, too many assumptions around this
too.

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

:) our case proves it's real!

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