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;
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel