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