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

[Yong Bakos]

On Jun 2, 2016, at 4:26 AM, Auke Booij <auke at tulcod.com> wrote:
> 
> On 1 June 2016 at 20:16, 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.
>> 
>> Correct?
> 
> Technically, it might be the case that nothing will necessarily break
> *now*. But such a change would move us from very the currently
> well-defined semantics of the enum attribute, into weird
> documentation-only or half-defined specifications. This will be
> confusing more often than it will be helpful.
> 
> If you want to refer to a specific enum, because your binary data
> *could* be interpreted as doing so, in my opinion this should be done
> in the textual documentation. Any formal reference should have a
> formal technical specification, which we don't have (and don't want).

Thanks Auke and Pekka, I definitely see the point in needing to be
conservative here, despite the innocuous intent. Scanner should not
accommodate the presence of array with enum right now.

yong