[Spice-devel] [RFC PATCH spice-common v2 02/20] A version 2 of the MousePosition message

[Lukáš Hrázký]

On Wed, 2018-08-22 at 16:11 -0500, Jonathon Jongsma wrote:
> On Thu, 2018-08-16 at 18:26 +0200, Lukáš Hrázký wrote:
> > The version 2 is using a (channel_id, monitor_id) pair to uniquely
> > identify the display on which the event occured, instead of the
> > ambiguous display_id.
> > 
> > Signed-off-by: Lukáš Hrázký <lhrazky at redhat.com>
> > ---
> >  common/messages.h | 8 ++++++++
> >  spice.proto       | 8 ++++++++
> >  2 files changed, 16 insertions(+)
> > 
> > diff --git a/common/messages.h b/common/messages.h
> > index 942ba07..9b05cee 100644
> > --- a/common/messages.h
> > +++ b/common/messages.h
> > @@ -460,6 +460,14 @@ typedef struct SpiceMsgcMousePosition {
> >      uint8_t display_id;
> >  } SpiceMsgcMousePosition;
> >  
> > +typedef struct SpiceMsgcMousePositionV2 {
> > +    uint32_t x;
> > +    uint32_t y;
> > +    uint32_t buttons_state;
> > +    uint32_t channel_id;
> > +    uint32_t monitor_id;
> > +} SpiceMsgcMousePositionV2;
> > +
> >  typedef struct SpiceMsgcMousePress {
> >      int32_t button;
> >      int32_t buttons_state;
> > diff --git a/spice.proto b/spice.proto
> > index 80976d4..14475fc 100644
> > --- a/spice.proto
> > +++ b/spice.proto
> > @@ -1092,6 +1092,14 @@ channel InputsChannel : BaseChannel {
> >          uint8 display_id;
> >      } @ctype(SpiceMsgcMousePosition) mouse_position;
> >  
> > +    message {
> > +        uint32 x;
> > +        uint32 y;
> > +        mouse_button_mask buttons_state;
> > +        uint32 channel_id;
> > +        uint32 monitor_id;
> > +    } @ctype(SpiceMsgcMousePositionV2) mouse_position_v2;
> > +
> >      message {
> >          mouse_button button;
> >          mouse_button_mask buttons_state;
> 
> 
> This protocol change is clearly necessary if we're going to support any
> configurations other than those that we've supported in the past. 
> 
> One thing I just realized is that a channel id is not enough by itself
> to uniquely identify a channel. You need a channel type and a channel
> id. You could argue that I'm being pedantic and that it's obvious that
> we're talking about display channels here. And maybe you'd be right.
> But maybe it would be better to be more explicit? You could do that
> either by using the existing SpiceChannelId type (which is a pair of
> (type, id)), or by simply using a more descriptive name (e.g.
> display_channel, or display_channel_id).

Well, it's only display channels that currently have any monitors.
You're technically right, if we ever introduce another channel type
similar to a display channel that will also have monitors, we would
need this distinction. (I don't think we'll ever want to add monitors
to any of the existing channels)

I feel like it's very unlikely and am inclined not to add the type, but
yeah...

> Or if nobody else cares, we could leave it like this.
> 
> Reviewed-by: Jonathon Jongsma <jjongsma at redhat.com>