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