[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 Wed, 2018-06-13 at 16:47 -0500, Jonathon Jongsma wrote:
> 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. 

I find this formulation confusing, though I think I figured out what
you mean :) I wouldn't call it "leak implementation details", but yeah,
it is abusing the semantics of the output_id while it shouldn't have
to.

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

You are right, but I think we are nowhere close to this level of
correctness. The current code is not abusing, it is mostly ignoring the
rules that should be in place for IDs.

I did it so that we don't have to change the protocol for this message,
if it isn't obvious... We could fix this, but it would require a
protocol change and all the jazz that goes with it :(

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

Yes yes :) that's why I put the comment there :D As I said, I did it to
avoid a protocol change that is not strictly necessary.

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