On Tue, 2018-11-13 at 11:56 +0100, Lukáš Hrázký wrote:
> On Mon, 2018-11-12 at 14:44 -0600, Jonathon Jongsma wrote:
> > On Mon, 2018-11-12 at 11:01 +0100, Lukáš Hrázký wrote:
> > > On Thu, 2018-11-08 at 11:36 -0600, Jonathon Jongsma wrote:
> > > > On Wed, 2018-11-07 at 18:23 +0100, Lukáš Hrázký wrote:
> > > > > The message serves for passing the device address and device
> > > > > display
> > > > > ID
> > > >
> > > > "serves for passing..." is a little strange to my ear. "serves
> > > > to
> > > > pass..." sounds OK. Or "is used for passing..."
> > >
> > > Ok, thanks :)
> > >
> > > > > information for all display channels from SPICE server to the
> > > > > vd_agent.
> > > > >
> > > > > Signed-off-by: Lukáš Hrázký <lhrazky@xxxxxxxxxx>
> > > > > ---
> > > > > spice/vd_agent.h | 15 +++++++++++++++
> > > > > 1 file changed, 15 insertions(+)
> > > > >
> > > > > diff --git a/spice/vd_agent.h b/spice/vd_agent.h
> > > > > index dda7044..a35f827 100644
> > > > > --- a/spice/vd_agent.h
> > > > > +++ b/spice/vd_agent.h
> > > > > @@ -91,6 +91,7 @@ enum {
> > > > > VD_AGENT_CLIENT_DISCONNECTED,
> > > > > VD_AGENT_MAX_CLIPBOARD,
> > > > > VD_AGENT_AUDIO_VOLUME_SYNC,
> > > > > + VD_AGENT_GRAPHICS_DEVICE_INFO,
> > > > > VD_AGENT_END_MESSAGE,
> > > > > };
> > > > >
> > > > > @@ -248,6 +249,19 @@ typedef struct SPICE_ATTR_PACKED
> > > > > VDAgentAudioVolumeSync {
> > > > > uint16_t volume[0];
> > > > > } VDAgentAudioVolumeSync;
> > > > >
> > > > > +typedef struct SPICE_ATTR_PACKED {
> > > > > + uint32_t channel_id;
> > > > > + uint32_t monitor_id;
> > > > > + uint32_t device_display_id;
> > > > > + uint32_t device_address_len;
> > > > > + uint8_t device_address[0]; // a zero-terminated string
> > > > > +} VDAgentGraphicsDeviceInfo;
> > > > > +
> > > > > +typedef struct SPICE_ATTR_PACKED VDAgentGraphicsDeviceInfos
> > > > > {
> > > > > + uint32_t count;
> > > > > + VDAgentGraphicsDeviceInfo graphics_device_infos[0];
> > > > > +} VDAgentGraphicsDeviceInfos;
> > > > > +
> > > >
> > > > I'm not a big fan of this name. "Information" is slightly
> > > > irregular
> > > > word which doesn't really have singular and plural forms. So
> > > > "Informations" or "Infos" sounds wrong to me.
> > >
> > > Yes, I know it's incorrect in english. I just followed the same
> > > convention I always use, name a struct and then use the plural to
> > > name
> > > the container for it... What am I supposed to do if it doesn't
> > > work
> > > for
> > > "Info"? :) I need to name the inner struct and then the one
> > > containing
> > > the list, I prefer this to some arbitrary plays on the name...
> > >
> > > > In addition, the VDAgentGraphicsDeviceInfo name implies to me
> > > > that
> > > > this
> > > > struct represents information about a single device. But in
> > > > reality, it
> > > > represents information about a single spice display --
> > > > specifically
> > > > which device display is associated with the spice display. So a
> > > > single
> > > > device with multiple displays would be represented by multiple
> > > > info
> > > > structs. So maybe a better name might be... (just brainstorming
> > > > here):
> > > > VDAgentSpiceDisplayInfo
> > > > VDAgentChannelDisplayInfo
> > > > VDAgentSpiceDisplayDeviceInfo
> > > >
> > > > None of them are very nice, but I feel like the 'focus' of the
> > > > name
> > > > should be the on the spice display rather than the device...
> > >
> > > What I did was take the name we used for the QEMU interface
> > > method:
> > > spice_qxl_set_device_info. I thought "device_info" is too generic
> > > outside a QXL interface context so I added the "graphics" to the
> > > "device".
> >
> > Right, except the data we're passing is not exactly the same thing
> > that
> > qemu passed to us. Qemu passed us a device address and a list of
> > displays associated with a QXL instance. We took that information
> > and
> > essentially created a table which maps from a spice protocol
> > display id
> > to some information that describes a guest device display. That
> > *mapping* is the important concept that we're passing down to the
> > guest. After all, the device information is already essentially
> > known
> > to the guest. So it feels slightly misleading to call it
> > VDAgentGraphicsDeviceInfo. Maybe something like
> > VDAgentDeviceDisplayMapping??
>
> Hmm. One thing I was aiming for was keeping the name similar to
> what's
> used in the QEMU interface, makes it easier for others reading the
> code
> to associate I think.
>
> You're right it is passing the mapping, but I think it is a common
> case
> when you're sending some data that there are some IDs that go with
> them
> and it's clear the data belong (map) to the IDs. But yeah, in this
> case
> the data are used for identification in the guest, so calling it
> mapping makes more sense.
>
> Still, not sure how much it helps and keeping the name similar seems
> more important to me.
OK, feel free to keep the original name, I don't feel that strongly
after discussing it a bit.
>
> > Note that I'm not trying to make a big deal of this. If we can't
> > come
> > up with a better name, I'm OK with the original one.
>
> As petty as these discussions seem to be, I think it's good to have
> them... bad names are bad :)
>
> > > I don't find your suggestions particularly better :) Especially
> > > those
> > > containing the word "Spice", I think that's redundant?
> > >
> > > The outer struct with the array kind of does contain all the
> > > device
> > > info, while the inner one is per display/monitor, so what about:
> > >
> > > struct VDAgentGraphicsDeviceInfo {
> > > ...
> > > VDAgentGraphicsDeviceDisplayInfo
> > > graphics_device_display_info[0];
> > > };
>
> So the above doesn't work for you?
Hmm, in theory it works, but in practice it's a bit confusing because
if you see either of them in isolation it's not as clear which is the
container struct and which is the individual struct. I'd prefer one
that was more immediately obvious, so I would probably prefer the
awkward *Infos name over this.
> Is VDAgentDisplayDeviceInfo and
> VDAgentDisplayDeviceDisplayInfo too much? :)
Heh, I don't think that works for me.
>
> If we went with your "mapping" name, how would you name the inner
> struct?
I think the inner struct is the mapping one. The outer one could be
pluralized:
struct VDAgentDeviceDisplayMappings {
...
VDAgentDeviceDisplayMapping displays[0];
};
?
(The name of the 'displays' field is obviously up for debate)
Jonathon
>
> Cheers,
> Lukas
>
> > > We can still discuss the "GraphicsDevice" part (I'm not too happy
> > > about
> > > it myself), but I still can't think of a better one...
> > >
> > > > Also, I was comparing this message to the monitors config
> > > > message
> > > > and
> > > > noticed that that message has a 'flags' field to accomodate
> > > > different
> > > > behavior that was added over time. If we add a 'flags' field,
> > > > it
> > > > would
> > > > give us some future-proofing that may allow us to interpret the
> > > > data
> > > > slightly differently based on the existence of different flags.
> > > > Worthwhile?
> > >
> > > I don't know. The flags add the possibility to pass boolean
> > > values,
> > > which is a very limited set of cases you are preparing for. Since
> > > we
> > > can't predict the future... I'd lean towards not adding it. We
> > > can
> > > also
> > > get into the (non)extesibility debate and perhaps add some
> > > generic
> > > "padding" to the structs which could be used for anything later,
> > > but...
> >
> >
> > yeah, I was mostly pondering aloud. I'm not opposed to moving
> > forward
> > without.
> >
> >
> >
> > > Thanks,
> > > Lukas
> > >
> > > > > enum {
> > > > > VD_AGENT_CAP_MOUSE_STATE = 0,
> > > > > VD_AGENT_CAP_MONITORS_CONFIG,
> > > > > @@ -264,6 +278,7 @@ enum {
> > > > > VD_AGENT_CAP_MONITORS_CONFIG_POSITION,
> > > > > VD_AGENT_CAP_FILE_XFER_DISABLED,
> > > > > VD_AGENT_CAP_FILE_XFER_DETAILED_ERRORS,
> > > > > + VD_AGENT_CAP_GRAPHICS_DEVICE_INFO,
> > > > > VD_AGENT_END_CAP,
> > > > > };
> >
> >
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel