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

Fri Jul 29 20:51:28 UTC 2016

Hi!

On Fri, Jul 29, 2016 at 10:52 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Fri, 22 Jul 2016 21:39:20 +0200
> "Nils Chr. Brause" <nilschrbrause at gmail.com> wrote:
>
>> HI,
>>
>> On Thu, Jun 2, 2016 at 10:30 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
>> > On Thu, 2 Jun 2016 15:39:47 +0800
>> > Jonas Ådahl <jadahl at gmail.com> wrote:
>> >
>> >> 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.
>>
>> Then that's not an 'array' than but more like a struct.
>> Therefore the structure definition should be in the XML.
>> Leaving it undocumented is a terrible option.
>
> Hi Nils,
>
> yes, things should definitely be documented. I mean human-readable
> documentation, not necessarily machine-parseable description.
>
>> >> > > > 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.
>>
>> Of course it is. If we properly define its contents, language bindings
>> could put the
>> proper structure into the argument list of the particular requests/events.
>
> I would hate to have to define the XML language for describing
> more complicated types than the three POD types we already have for
> message arguments.

I acually don't image it to be too complicated.
To declare a structure, I'd imagine something like:

<struct name="foo">
  <entry name="bar" type="string"/>
  <entry name="baz" type="uint" enum="wl_shm.format"/>
  <entry name="yada" type="array" array="other"/>
</struct>

and then later:

<arg name="stuff" type="array" array="foo"/>

This would denote an array of structures of type "foo".
Having an array of length 1 would be possible of course.

In case of real arrays, I'd imagine something like:

<arg name="reals" type="array" array="fixed"/>

And for "undescribed data" like you mention below,
the "array" attribute can just be left out.

No much more complicated than an enumeration.
You could even nest structures, if needed.

>
>> >> > > >
>> >> > > > 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?
>>
>> Sorry for my late answer. I'm am currently very busy with my PhD thesis.
>>
>> >> > > >
>> >> > > > 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.
>> >
>> > Hi,
>> >
>> > how can you turn a wl_array into a list of enums, when you do not know
>> > the element size in the wl_array? That's my major objection.
>> >
>> > Nothing says it's 32-bit in the XML, we don't have the language for
>> > that. Should we then add a new attribute "element_size" applicable only
>> > to <arg>s with type="array", giving the element size in bytes?
>> >
>> > If the attribute is not given, then the array is a single blob as
>> > before.
>> >
>> > Then the remaining ambiguity would be signed vs. unsigned.
>>
>> Reading that, I get the impression that arrays are completely unusable
>> right now. How is a programmer supposed to know how to use an array,
>> if there is no documentation about its contents, whatsoever?
>
> By the documentation. Where we do not have documentation (which we
> should have), programmers have looked at existing code using those arrays.
>
>> >
>> >> That is my take on it anyhow; I'm not a language binding author so take
>> >> it with a grain of salt.
>> >
>> > Me neither, so I would not go adding these features unless someone
>> > tells us they would really be useful for language bindings. Then we get
>> > to figure out what the ABI stability rules are for adding or changing
>> > these.
>>
>> I am a language binding developer. In my C++ bindings [1] the developer
>> currently has to know what the contents of a particular array are and do a
>> manual cast (See the bottom of my README.md). This is sub-optimal.
>> I'd rather have a std::vector<> (or whatever) with the correct type already
>> in the argument list, otherwise the user could cast the array to the wrong
>> thing accidentally.
>
> That sounds like a very good use case. It made me rethink this a bit.
>
>> > And yes, we have probably neglected to explicitly document the array
>> > element type in all interfaces so far. That's a doc bug.
>>
>> Are there any plans to fix that?
>
> I don't. Maybe if I drive by something and notice, I might fix it, but
> I don't have an intention to re-review everything.
>
> I think this is the point where "patches welcome" is a sincere suggestion.

If I had more time on my hands, I'd love to improve that.
Probably after my PhD thesis. ;)

To improve the documentation situation, you could from now on
only accept protocol additions, when they are completly and
properly documented, so that programmers don't have to look
through lots of example code. I know, programmers don't like to
write documentation, but it would certainly improve the situation.

Just a suggestion.

>
>> Once the contents of arrays are _properly_ documented (complete type),
>> language bindings could finally produce arrays with the correct content
>> instead of having the user to manually cast them (possibly wrongly).
>
> I would really prefer if you made the difference between documentation
> and machine-readable description in terms.

I'll try. But for an programmer there is really no difference between
machine-readable and "human readable" documentation, because
"machine-readable" doesn't mean it's not human readable. On the
contrary, I most of the time prefer machine-readable documentation,
because it is generally more precise and without room for interpretation.
Therefore and since a am a language binding devoloper, I often value
the machine-readable documation more.

>
> Documentation patches I would welcome and review any time, if I just
> notice them.
>
>> > I still do not think we should preclude anyone from storing a list of
>> > enums in an array of e.g. uint8_t. Maybe xdg-shell could use arrays of
>> > uint16_t for the state enums?
>>
>> Everywhere else, enums are 32 bits ('int, 'uint'). Why should that be
>> different in arrays? That doesn't make any sense and will confuse
>> people unnecessarily.
>
> I already explained that. Also I don't think C guarantees enums are
> 32-bit either, even if just historically.

You are right, they arent. But that is not my point. The protocol already
marks every enum as an "uint" (or "int" in that one unfortunate exception).
Therefore, we should just stick to that. It doesn't matter how long an
unsigned int or int in C actually is. (It's 32 bit on all suported platforms,
therefore I misleadingly used the term "32 bits" when i really just meant
"uint" or "int". I'm sorry.)

> People have been free to use arbitrary binary data in 'array'
> arguments. We cannot just break that now.

Is anyone acutally doing that anywhere?

> Adding machine-readable descriptions for the same POD types (int, uint,
> fixed) would be feasible on one hand, but then we must also have the
> escape hatch for "undescribed data, read the docs instead" since we
> cannot get rid of that. It is also needed for XML files that have not
> been fixed to have the new attribute.
>
> If we can describe the POD types, then we can also add the enum
> attribute where it is applicable.
>
> Would that be workable for you?

I don't really get what you are trying to say here. There is no need to add
machine-readable descriptions for (u)int/fixed. It's already clear what they
are. What we need are descriptions of the non-POD types: arrays.

I would however be fine with an "undescribed data" excaption if it's really
neccessary. For exmaple if the type/structure of the contents can change
from call to call (I think of pixel formats and audio right now).

>
> Would anyone like to do the work?
>
> 1. Review all arrays in wayland and wayland-protocols and add any
> missing documentation by cross-checking from well-known implementations.
>
> 2. Patch wayland-scanner to inspect the new array element type
> attribute. Document the new attribute.
>
> 3. Patch wayland-scanner to inspect the enum attribute with arrays.
> Update the XML language documentation.
>
> 4. Add array type and enum attributes to wayland and wayland-protocols
> XML files.
>
> As for API and ABI stability, there will be no changes to the C
> generator output, and generators for other languages get to solve their
> own issues just like when we added the enum attribute. Ok?
>
> If someone wants, one could file a Phabricator Task and copy the
> relevant bits from this email for tracking the development.
>
> Just to repeat, I don't think going beyond int, uint and fixed types is
> a good idea.

Well, arrays with arbitrary data in them are already new types.
It's just a matter of documenting them. :)

>
>
> Thanks,
> pq
>
>
>> [1] https://github.com/NilsBrause/waylandpp.
>

Cheers,
Nils