[1/2] Clarify the enum argument type

Submitted by Bill Spitzak on April 23, 2015, 2:40 a.m.

Details

Message ID 1429756845-16856-2-git-send-email-spitzak@gmail.com
State Superseded
Delegated to: Bryce Harrington
Headers show

Not browsing as part of any series.

Commit Message

Bill Spitzak April 23, 2015, 2:40 a.m.
This improvement to the protocol allows you to refer to the kind of enum you are expecting.
It also introduces a distinction between enums that are bitfields, ie
that can be OR'ed together.
---
 protocol/wayland.dtd |    2 ++
 protocol/wayland.xml |   32 ++++++++++++++++----------------
 2 files changed, 18 insertions(+), 16 deletions(-)

Patch hide | download patch | download mbox

diff --git a/protocol/wayland.dtd b/protocol/wayland.dtd
index b8b1573..3b67ca8 100644
--- a/protocol/wayland.dtd
+++ b/protocol/wayland.dtd
@@ -14,6 +14,7 @@ 
 <!ELEMENT enum (description?,entry*)>
   <!ATTLIST enum name CDATA #REQUIRED>
   <!ATTLIST enum since CDATA #IMPLIED>
+  <!ATTLIST enum bitfield CDATA #IMPLIED>
 <!ELEMENT entry (description?)>
   <!ATTLIST entry name CDATA #REQUIRED>
   <!ATTLIST entry value CDATA #REQUIRED>
@@ -25,5 +26,6 @@ 
   <!ATTLIST arg summary CDATA #IMPLIED>
   <!ATTLIST arg interface CDATA #IMPLIED>
   <!ATTLIST arg allow-null CDATA #IMPLIED>
+  <!ATTLIST arg enum CDATA #IMPLIED>
 <!ELEMENT description (#PCDATA)>
   <!ATTLIST description summary CDATA #REQUIRED>
diff --git a/protocol/wayland.xml b/protocol/wayland.xml
index f52677f..2b9efa1 100644
--- a/protocol/wayland.xml
+++ b/protocol/wayland.xml
@@ -367,7 +367,7 @@ 
 	can be used for buffers. Known formats include
 	argb8888 and xrgb8888.
       </description>
-      <arg name="format" type="uint"/>
+      <arg name="format" type="uint" enum="format"/>
     </event>
   </interface>
 
@@ -774,7 +774,7 @@ 
       </description>
       <arg name="seat" type="object" interface="wl_seat" summary="the wl_seat whose pointer is used"/>
       <arg name="serial" type="uint" summary="serial of the implicit grab on the pointer"/>
-      <arg name="edges" type="uint" summary="which edge or corner is being dragged"/>
+      <arg name="edges" type="uint" summary="which edge or corner is being dragged" enum="resize"/>
     </request>
 
     <request name="set_toplevel">
@@ -785,7 +785,7 @@ 
       </description>
     </request>
 
-    <enum name="transient">
+    <enum name="transient" bitfield="true">
       <description summary="details of transient behaviour">
 	These flags specify details of the expected behaviour
 	of transient surfaces. Used in the set_transient request.
@@ -807,7 +807,7 @@ 
       <arg name="parent" type="object" interface="wl_surface"/>
       <arg name="x" type="int"/>
       <arg name="y" type="int"/>
-      <arg name="flags" type="uint"/>
+      <arg name="flags" type="uint" enum="transient"/>
     </request>
 
     <enum name="fullscreen_method">
@@ -891,7 +891,7 @@ 
       <arg name="parent" type="object" interface="wl_surface"/>
       <arg name="x" type="int"/>
       <arg name="y" type="int"/>
-      <arg name="flags" type="uint"/>
+      <arg name="flags" type="uint" enum="transient"/>
     </request>
 
     <request name="set_maximized">
@@ -972,7 +972,7 @@ 
 	in surface local coordinates.
       </description>
 
-      <arg name="edges" type="uint"/>
+      <arg name="edges" type="uint" enum="resize"/>
       <arg name="width" type="int"/>
       <arg name="height" type="int"/>
     </event>
@@ -1337,7 +1337,7 @@ 
       maintains a keyboard focus and a pointer focus.
     </description>
 
-    <enum name="capability">
+    <enum name="capability" bitfield="true">
       <description summary="seat capability bitmask">
         This is a bitmask of capabilities this seat has; if a member is
         set, then it is present on the seat.
@@ -1353,7 +1353,7 @@ 
 	keyboard or touch capabilities.  The argument is a capability
 	enum containing the complete set of capabilities this seat has.
       </description>
-      <arg name="capabilities" type="uint"/>
+      <arg name="capabilities" type="uint" enum="capability"/>
     </event>
 
     <request name="get_pointer">
@@ -1521,7 +1521,7 @@ 
       <arg name="serial" type="uint"/>
       <arg name="time" type="uint" summary="timestamp with millisecond granularity"/>
       <arg name="button" type="uint"/>
-      <arg name="state" type="uint"/>
+      <arg name="state" type="uint" enum="button_state"/>
     </event>
 
     <enum name="axis">
@@ -1553,7 +1553,7 @@ 
       </description>
 
       <arg name="time" type="uint" summary="timestamp with millisecond granularity"/>
-      <arg name="axis" type="uint"/>
+      <arg name="axis" type="uint" enum="axis"/>
       <arg name="value" type="fixed"/>
     </event>
 
@@ -1593,7 +1593,7 @@ 
 	This event provides a file descriptor to the client which can be
 	memory-mapped to provide a keyboard mapping description.
       </description>
-      <arg name="format" type="uint"/>
+      <arg name="format" type="uint" enum="keymap_format"/>
       <arg name="fd" type="fd"/>
       <arg name="size" type="uint"/>
     </event>
@@ -1638,7 +1638,7 @@ 
       <arg name="serial" type="uint"/>
       <arg name="time" type="uint" summary="timestamp with millisecond granularity"/>
       <arg name="key" type="uint"/>
-      <arg name="state" type="uint"/>
+      <arg name="state" type="uint" enum="key_state"/>
     </event>
 
     <event name="modifiers">
@@ -1819,17 +1819,17 @@ 
 	   summary="width in millimeters of the output"/>
       <arg name="physical_height" type="int"
 	   summary="height in millimeters of the output"/>
-      <arg name="subpixel" type="int"
+      <arg name="subpixel" type="int" enum="subpixel"
 	   summary="subpixel orientation of the output"/>
       <arg name="make" type="string"
 	   summary="textual description of the manufacturer"/>
       <arg name="model" type="string"
 	   summary="textual description of the model"/>
-      <arg name="transform" type="int"
+      <arg name="transform" type="int" enum="transform"
 	   summary="transform that maps framebuffer to output"/>
     </event>
 
-    <enum name="mode">
+    <enum name="mode" bitfield="true">
       <description summary="mode information">
 	These flags describe properties of an output mode.
 	They are used in the flags bitfield of the mode event.
@@ -1856,7 +1856,7 @@ 
         the output may be scaled, as described in wl_output.scale,
         or transformed , as described in wl_output.transform.
       </description>
-      <arg name="flags" type="uint" summary="bitfield of mode flags"/>
+      <arg name="flags" type="uint" summary="bitfield of mode flags" enum="mode"/>
       <arg name="width" type="int" summary="width of the mode in hardware units"/>
       <arg name="height" type="int" summary="height of the mode in hardware units"/>
       <arg name="refresh" type="int" summary="vertical refresh rate in mHz"/>

Comments

On Wed, 22 Apr 2015 19:40:44 -0700
Bill Spitzak <spitzak@gmail.com> wrote:

> This improvement to the protocol allows you to refer to the kind of enum you are expecting.
> It also introduces a distinction between enums that are bitfields, ie
> that can be OR'ed together.
> ---
>  protocol/wayland.dtd |    2 ++
>  protocol/wayland.xml |   32 ++++++++++++++++----------------
>  2 files changed, 18 insertions(+), 16 deletions(-)

Hi,

I like the enum="" additions. I believe wayland-scanner should be
ignoring unknown attributes since its inception, so that should not
pose a problem.

All in all this patch series would be a very good improvement to the
docs.

However, I don't think we have a closure on the enum/bitfield attribute
discussion yet: how they can be used, what they guarantee and require, and
what to do when things change. I think we need to see that through
before taking this or the 2/2 patch, because these attributes may
affect other code generators than C. (TBF, I haven't been able to dig
into that discussion again.)

Let's keep these two patches still in the queue until the enum/bitfield
attribute design is decided.


Thanks,
pq
Hi,

I would recommend to also include the name of the interface, where the enum
is defined, in the enum attribute, e.g:
<arg name="format" type="uint" enum="wl_shm.format"/>

Also, I think you forgot some enum attributes, e.g. wl_shm_pool::
create_buffer::format is the wl_shm::format enum, but I can't find it in
your patch (I can't seem to find the e-mail with your second patch, though).
You might want to compare with my XML file:
https://github.com/NilsBrause/waylandpp/blob/master/scanner/wayland.xml

Apart from that, I like it for obvious reasons. :)

Cheers,
Nils


On 23 Apr 2015 04:41, "Bill Spitzak" <spitzak@gmail.com> wrote:
>
> This improvement to the protocol allows you to refer to the kind of enum
you are expecting.
> It also introduces a distinction between enums that are bitfields, ie
> that can be OR'ed together.
> ---
>  protocol/wayland.dtd |    2 ++
>  protocol/wayland.xml |   32 ++++++++++++++++----------------
>  2 files changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/protocol/wayland.dtd b/protocol/wayland.dtd
> index b8b1573..3b67ca8 100644
> --- a/protocol/wayland.dtd
> +++ b/protocol/wayland.dtd
> @@ -14,6 +14,7 @@
>  <!ELEMENT enum (description?,entry*)>
>    <!ATTLIST enum name CDATA #REQUIRED>
>    <!ATTLIST enum since CDATA #IMPLIED>
> +  <!ATTLIST enum bitfield CDATA #IMPLIED>
>  <!ELEMENT entry (description?)>
>    <!ATTLIST entry name CDATA #REQUIRED>
>    <!ATTLIST entry value CDATA #REQUIRED>
> @@ -25,5 +26,6 @@
>    <!ATTLIST arg summary CDATA #IMPLIED>
>    <!ATTLIST arg interface CDATA #IMPLIED>
>    <!ATTLIST arg allow-null CDATA #IMPLIED>
> +  <!ATTLIST arg enum CDATA #IMPLIED>
>  <!ELEMENT description (#PCDATA)>
>    <!ATTLIST description summary CDATA #REQUIRED>
> diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> index f52677f..2b9efa1 100644
> --- a/protocol/wayland.xml
> +++ b/protocol/wayland.xml
> @@ -367,7 +367,7 @@
>         can be used for buffers. Known formats include
>         argb8888 and xrgb8888.
>        </description>
> -      <arg name="format" type="uint"/>
> +      <arg name="format" type="uint" enum="format"/>
>      </event>
>    </interface>
>
> @@ -774,7 +774,7 @@
>        </description>
>        <arg name="seat" type="object" interface="wl_seat" summary="the
wl_seat whose pointer is used"/>
>        <arg name="serial" type="uint" summary="serial of the implicit
grab on the pointer"/>
> -      <arg name="edges" type="uint" summary="which edge or corner is
being dragged"/>
> +      <arg name="edges" type="uint" summary="which edge or corner is
being dragged" enum="resize"/>
>      </request>
>
>      <request name="set_toplevel">
> @@ -785,7 +785,7 @@
>        </description>
>      </request>
>
> -    <enum name="transient">
> +    <enum name="transient" bitfield="true">
>        <description summary="details of transient behaviour">
>         These flags specify details of the expected behaviour
>         of transient surfaces. Used in the set_transient request.
> @@ -807,7 +807,7 @@
>        <arg name="parent" type="object" interface="wl_surface"/>
>        <arg name="x" type="int"/>
>        <arg name="y" type="int"/>
> -      <arg name="flags" type="uint"/>
> +      <arg name="flags" type="uint" enum="transient"/>
>      </request>
>
>      <enum name="fullscreen_method">
> @@ -891,7 +891,7 @@
>        <arg name="parent" type="object" interface="wl_surface"/>
>        <arg name="x" type="int"/>
>        <arg name="y" type="int"/>
> -      <arg name="flags" type="uint"/>
> +      <arg name="flags" type="uint" enum="transient"/>
>      </request>
>
>      <request name="set_maximized">
> @@ -972,7 +972,7 @@
>         in surface local coordinates.
>        </description>
>
> -      <arg name="edges" type="uint"/>
> +      <arg name="edges" type="uint" enum="resize"/>
>        <arg name="width" type="int"/>
>        <arg name="height" type="int"/>
>      </event>
> @@ -1337,7 +1337,7 @@
>        maintains a keyboard focus and a pointer focus.
>      </description>
>
> -    <enum name="capability">
> +    <enum name="capability" bitfield="true">
>        <description summary="seat capability bitmask">
>          This is a bitmask of capabilities this seat has; if a member is
>          set, then it is present on the seat.
> @@ -1353,7 +1353,7 @@
>         keyboard or touch capabilities.  The argument is a capability
>         enum containing the complete set of capabilities this seat has.
>        </description>
> -      <arg name="capabilities" type="uint"/>
> +      <arg name="capabilities" type="uint" enum="capability"/>
>      </event>
>
>      <request name="get_pointer">
> @@ -1521,7 +1521,7 @@
>        <arg name="serial" type="uint"/>
>        <arg name="time" type="uint" summary="timestamp with millisecond
granularity"/>
>        <arg name="button" type="uint"/>
> -      <arg name="state" type="uint"/>
> +      <arg name="state" type="uint" enum="button_state"/>
>      </event>
>
>      <enum name="axis">
> @@ -1553,7 +1553,7 @@
>        </description>
>
>        <arg name="time" type="uint" summary="timestamp with millisecond
granularity"/>
> -      <arg name="axis" type="uint"/>
> +      <arg name="axis" type="uint" enum="axis"/>
>        <arg name="value" type="fixed"/>
>      </event>
>
> @@ -1593,7 +1593,7 @@
>         This event provides a file descriptor to the client which can be
>         memory-mapped to provide a keyboard mapping description.
>        </description>
> -      <arg name="format" type="uint"/>
> +      <arg name="format" type="uint" enum="keymap_format"/>
>        <arg name="fd" type="fd"/>
>        <arg name="size" type="uint"/>
>      </event>
> @@ -1638,7 +1638,7 @@
>        <arg name="serial" type="uint"/>
>        <arg name="time" type="uint" summary="timestamp with millisecond
granularity"/>
>        <arg name="key" type="uint"/>
> -      <arg name="state" type="uint"/>
> +      <arg name="state" type="uint" enum="key_state"/>
>      </event>
>
>      <event name="modifiers">
> @@ -1819,17 +1819,17 @@
>            summary="width in millimeters of the output"/>
>        <arg name="physical_height" type="int"
>            summary="height in millimeters of the output"/>
> -      <arg name="subpixel" type="int"
> +      <arg name="subpixel" type="int" enum="subpixel"
>            summary="subpixel orientation of the output"/>
>        <arg name="make" type="string"
>            summary="textual description of the manufacturer"/>
>        <arg name="model" type="string"
>            summary="textual description of the model"/>
> -      <arg name="transform" type="int"
> +      <arg name="transform" type="int" enum="transform"
>            summary="transform that maps framebuffer to output"/>
>      </event>
>
> -    <enum name="mode">
> +    <enum name="mode" bitfield="true">
>        <description summary="mode information">
>         These flags describe properties of an output mode.
>         They are used in the flags bitfield of the mode event.
> @@ -1856,7 +1856,7 @@
>          the output may be scaled, as described in wl_output.scale,
>          or transformed , as described in wl_output.transform.
>        </description>
> -      <arg name="flags" type="uint" summary="bitfield of mode flags"/>
> +      <arg name="flags" type="uint" summary="bitfield of mode flags"
enum="mode"/>
>        <arg name="width" type="int" summary="width of the mode in
hardware units"/>
>        <arg name="height" type="int" summary="height of the mode in
hardware units"/>
>        <arg name="refresh" type="int" summary="vertical refresh rate in
mHz"/>
> --
> 1.7.9.5
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
On Thu, Oct 1, 2015 at 8:09 AM, Nils Chr. Brause <nilschrbrause@gmail.com>
wrote:

> Hi,
>
> I would recommend to also include the name of the interface, where the
> enum is defined, in the enum attribute, e.g:
> <arg name="format" type="uint" enum="wl_shm.format"/>
>

I believe popular opinion was that a "local" enum, belonging to the same
object as the message or event, should not require this prefix. One big
reason is so that the object can be renamed without having to edit it's
contents.

And currently only such "local" enums are being used in the protocols, so
there is no need for parsers to support any such prefix yet, and no need to
figure out it's rules. Whether the ability to get enums from other objects,
or from some global space, does not have to be decided yet.
On Thu, Oct 1, 2015 at 9:00 PM, Bill Spitzak <spitzak@gmail.com> wrote:
>
>
> On Thu, Oct 1, 2015 at 8:09 AM, Nils Chr. Brause <nilschrbrause@gmail.com>
> wrote:
>>
>> Hi,
>>
>> I would recommend to also include the name of the interface, where the
>> enum is defined, in the enum attribute, e.g:
>> <arg name="format" type="uint" enum="wl_shm.format"/>
>
>
> I believe popular opinion was that a "local" enum, belonging to the same
> object as the message or event, should not require this prefix. One big
> reason is so that the object can be renamed without having to edit it's
> contents.
>
> And currently only such "local" enums are being used in the protocols, so
> there is no need for parsers to support any such prefix yet, and no need to
> figure out it's rules. Whether the ability to get enums from other objects,
> or from some global space, does not have to be decided yet.

There are "non-local" enums, e.g.
wl_surface::set_buffer_transform::transform uses the enum
wl_output::transform, or am I missing something important here?
On Thu, Oct 1, 2015 at 12:09 PM, Nils Chr. Brause <nilschrbrause@gmail.com>
wrote:

> On Thu, Oct 1, 2015 at 9:00 PM, Bill Spitzak <spitzak@gmail.com> wrote:
> >
> >
> > On Thu, Oct 1, 2015 at 8:09 AM, Nils Chr. Brause <
> nilschrbrause@gmail.com>
> > wrote:
> >>
> >> Hi,
> >>
> >> I would recommend to also include the name of the interface, where the
> >> enum is defined, in the enum attribute, e.g:
> >> <arg name="format" type="uint" enum="wl_shm.format"/>
> >
> >
> > I believe popular opinion was that a "local" enum, belonging to the same
> > object as the message or event, should not require this prefix. One big
> > reason is so that the object can be renamed without having to edit it's
> > contents.
> >
> > And currently only such "local" enums are being used in the protocols, so
> > there is no need for parsers to support any such prefix yet, and no need
> to
> > figure out it's rules. Whether the ability to get enums from other
> objects,
> > or from some global space, does not have to be decided yet.
>
> There are "non-local" enums, e.g.
> wl_surface::set_buffer_transform::transform uses the enum
> wl_output::transform, or am I missing something important here?
>

It looks like you are right. Not sure what was done, I have not looked at
it for a long time now. However whatever is in the patch seems to work and
I would use that.