array and enum attributes (Was: [PATCH] xdg-shell: add draw states for xdg_surface)

Thu Jun 2 07:39:47 UTC 2016

On Thu, Jun 02, 2016 at 10:24:19AM +0300, Pekka Paalanen wrote:
> On Wed, 1 Jun 2016 13:16:31 -0500
> Yong Bakos <junk at humanoriented.com> wrote:
> 
> > On May 30, 2016, at 3:54 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> > > 
> > > On Sat, 28 May 2016 08:39:59 -0500
> > > Yong Bakos <junk at humanoriented.com> wrote:
> > >   
> > >> Hi Mike,
> > >> Regarding the combination of type="array" enum="foo"...
> > >>   
> > >>> On May 27, 2016, at 12:30 PM, Mike Blumenkrantz <michael.blumenkrantz at gmail.com> wrote:
> > >>> 
> > >>> I've inlined some replies below.
> > >>> 
> > >>> On Fri, May 27, 2016 at 1:13 PM Yong Bakos <junk at humanoriented.com <mailto:junk at humanoriented.com>> wrote:
> > >>> On May 27, 2016, at 10:29 AM, Mike Blumenkrantz <zmike at osg.samsung.com <mailto:zmike at osg.samsung.com>> wrote:  
> > >>>> 
> > >>>> this adds a method for compositors to change various draw attributes
> > >>>> for a surface
> > >>>> 
> > >>>> Signed-off-by: Mike Blumenkrantz <zmike at osg.samsung.com <mailto:zmike at osg.samsung.com>>
> > >>>> Signed-off-by: Jonas Ådahl <jadahl at gmail.com <mailto:jadahl at gmail.com>>  
> > >>> 
> > >>> Hi Mike & Jonas,
> > >>> A question about communicating default state, and some
> > >>> minor nits you can certainly ignore, inline below.
> > >>> 
> > >>>   
> > >>>> ---
> > >>>> unstable/xdg-shell/xdg-shell-unstable-v6.xml | 69 ++++++++++++++++++++++++++++
> > >>>> 1 file changed, 69 insertions(+)
> > >>>> 
> > >>>> diff --git a/unstable/xdg-shell/xdg-shell-unstable-v6.xml b/unstable/xdg-shell/xdg-shell-unstable-v6.xml
> > >>>> index dfd7e84..0fa76d4 100644
> > >>>> --- a/unstable/xdg-shell/xdg-shell-unstable-v6.xml
> > >>>> +++ b/unstable/xdg-shell/xdg-shell-unstable-v6.xml  
> > >   
> > >>>> +
> > >>>> +        Calling this after an xdg_toplevel's first commit will raise a client error.
> > >>>> +      </description>
> > >>>> +      <arg name="states" type="array" enum="draw_state"/>  
> > >>> 
> > >>> Just a sanity check, since I haven't seen it in a protocol spec yet. Does scanner handle
> > >>> this combination of array and enum correctly?
> > >>> 
> > >>> Good catch. This also affects the event above it.  
> > >> 
> > >> As we discussed via IRC (27 May), the scanner will choke on this. While we talked about
> > >> making a change to the scanner to allow this, perhaps such a change doesn't make sense.
> > >> 
> > >> Given a type="array", scanner will generate a parameter of type wl_array.
> > >> 
> > >> Perhaps the short story here is to just remove the enum from this arg, and the similar
> > >> arg in the configure_draw_states event above. What do you think?
> > >> 
> > >> (I wonder if it's the DTD that can change, so the scanner's validation step
> > >> will catch the unsupported combination of type="array" enum="foo". My gut tells me that
> > >> DTDs don't support this logic, but I'll dig into this.)  
> > > 
> > > Hi,
> > > 
> > > here is some background.
> > > 
> > > A type="array" argument is really just a binary blob of data. The XML
> > > description, human documentation aside, does not specify anything about
> > > the blob contents. Therefore adding an XML attribute pointing to an
> > > enum definition is half-useless. Generators could use it for creating
> > > automatic links in documentation, but it cannot be used for code
> > > generation, because you don't know the types contained in the blob.
> > > 
> > > We also do not want to add blob content type definitions to the XML
> > > language, because you might want to have everything C is able to
> > > express, including nested structs. There is also no requirement that
> > > the "array" is really an array - every "element" could be a different
> > > thing. It could be bitstream and whatnot. Only the use of
> > > wl_array_for_each() implies it is an array of similar elements,
> > > wl_array_add() does not.
> > > 
> > > The big point in adding enum annotations was that language bindings
> > > generators (other than wayland-scanner) could use the annotation for
> > > code generation. I don't think it is possible with the array type.
> > > 
> > > If we allow enum annotation with the array type, it will only be usable
> > > for doc links, unlike the other enum annotations.
> > > 
> > > OTOH, we have lots and lots of places in the documentation texts that
> > > refer to some request, event, interface, etc. that would be useful as a
> > > hyperlink in the generated docs. Enums could fall into that as well, so
> > > we would not need the attribute for only documentation.
> > > 
> > > Auke, Nils, what's your take on this matter?
> > > 
> > > We do have some documentation about enums in
> > > https://wayland.freedesktop.org/docs/html/ch04.html#sect-Protocol-Basic-Principles
> > > 
> > > Thanks,
> > > pq  
> > 
> > Pekka,
> > Thank you for the info. Just so I understand your points correctly, let
> > me assert that /just/ making a minor change to scanner to not error on
> > the presence of both array and enum together does not have any major
> > drawbacks.
> 
> None other than it does not make sense now and I cannot see how it
> could make sense in the future. I see it similar to allowing
> "interface" attribute on <arg> tags whose type is not "object" or
> "new_id".
> 
> Therefore I am against the idea unless you can point out something I
> didn't already take into account.

Is this that much different from adding a enum attribute to a uint or
int entry? It adds additional assumptions about the content of the low
level data type. An "array" is implemented as a series of bytes, but it
could be interpreted as a series of enum values as well. A language
binding could use this information in a similar way as to how it would
interpret enum on an int in that it can expose the low level array as
for example a list of enum values.

That is my take on it anyhow; I'm not a language binding author so take
it with a grain of salt.

Jonas

> 
> If it is only about doc links, I'd really prefer to improve the doc
> generation to be able to produce links from text properly if it doesn't
> work already.
> 
> 
> Thanks,
> pq