|
View:
New views
8 Messages
—
Rating Filter:
Alert me
|
|
|
XKEYBOARD protocol definitionI'm preparing to push the GSOC protocol definition for XKEYBOARD. Please
take a look at git://anongit.freedesktop.org/~peterh/xkbproto I currently plan to squash all the changes, both mine and Mariusz's, into a pair of commits. One to update the docs and .xsd to allow the new elements, and one that simply adds the final xkb.xml. Review welcome. My changes since the last time we discussed xkb.xml are: - Remove 'type' from 'sumof' This attribute seems unnecessary. - Resolve name collisions in xkb Names inside switches were colliding with names outside switches (while still depending on the value from outside the switch). - List length is in list elements, not bytes Most lists had their length calculated incorrectly. - Remove <if> construct; it is unnecessary This change depends on BOOL always being xTrue (1) or xFalse (0). - Clean up comments - Improve definition of 'SetMap' Cases where a list of CARD8 was being used instead of the appropriate struct inside the SetMap request. Note that I have not reviewed the GSOC changes to libxcb. These changes only address the protocol definition. Thanks, Peter Harris -- Open Text Connectivity Solutions Group Peter Harris http://connectivity.opentext.com/ Research and Development Phone: +1 905 762 6001 pharris@... Toll Free: 1 877 359 4866 _______________________________________________ Xcb mailing list Xcb@... http://lists.freedesktop.org/mailman/listinfo/xcb |
|
|
Re: XKEYBOARD protocol definitionNice! Thanks much.
Bart In message <4AF9D964.2050204@...> you wrote: > I'm preparing to push the GSOC protocol definition for XKEYBOARD. Please > take a look at git://anongit.freedesktop.org/~peterh/xkbproto > > I currently plan to squash all the changes, both mine and Mariusz's, > into a pair of commits. One to update the docs and .xsd to allow the new > elements, and one that simply adds the final xkb.xml. > > Review welcome. > > My changes since the last time we discussed xkb.xml are: > > - Remove 'type' from 'sumof' > This attribute seems unnecessary. > > - Resolve name collisions in xkb > Names inside switches were colliding with names outside switches (while > still depending on the value from outside the switch). > > - List length is in list elements, not bytes > Most lists had their length calculated incorrectly. > > - Remove <if> construct; it is unnecessary > This change depends on BOOL always being xTrue (1) or xFalse (0). > > - Clean up comments > > - Improve definition of 'SetMap' > Cases where a list of CARD8 was being used instead of the appropriate > struct inside the SetMap request. > > Note that I have not reviewed the GSOC changes to libxcb. These changes > only address the protocol definition. > > Thanks, > Peter Harris > -- > Open Text Connectivity Solutions Group > Peter Harris http://connectivity.opentext.com/ > Research and Development Phone: +1 905 762 6001 > pharris@... Toll Free: 1 877 359 4866 > _______________________________________________ > Xcb mailing list > Xcb@... > http://lists.freedesktop.org/mailman/listinfo/xcb Xcb mailing list Xcb@... http://lists.freedesktop.org/mailman/listinfo/xcb |
|
|
Re: XKEYBOARD protocol definitionPeter,
Some review below. I haven't seen the earlier work so this will be zero-based. There are some changes upstream that aren't in your repo. I needed to pull from xcb/proto to get recent changes. Obligatory whitespace comments: the indentation of the XML files is a little erratic. There is some flagged whitespace I can see with git diff --color, some of which I mention below but not all. --Eamon W. diff --git a/doc/xml-xcb.txt b/doc/xml-xcb.txt index feb9984..db86159 100644 --- a/doc/xml-xcb.txt +++ b/doc/xml-xcb.txt @@ -227,6 +227,17 @@ enum; the value is restricted to one of the constants named in the enum. CARD32. value-mask-name gives the field name of the bitmask, and value-list-name gives the field name of the list of values. +<switch name="identifier"> switch expression <bitcase> bitcase expression, fields </bitcase> +...</switch> + + This element represents conditional inclusion of fields. It can be viewed as + sequence of multiple if's: if ( switch expression & bitcase + expression ) is equal to bitcase expression, bitcase fields are included in structure. + It can be used only as the last field of structure. + I don't see any new xcbgen Python code to handle this new element. The only Python changes I see are to expr.py, adding new list length calculators. Are there additional Python changes forthcoming, perhaps in the libxcb changes? +<replyof request="identifier" name="identifier" /> + + This element makes reply of request a field of structure. This does not appear anywhere else and should be removed. Expressions ----------- @@ -256,3 +267,21 @@ Expressions The bit element represents a literal bitmask value in an expression. The integer must be in the range 0..31, expanding to (1<<n) in C. + +<enumref ref="identifier">enum item identifier</enumref> + + This elements represents reference to item of enum. s/elements/element/ + +<unop op="operator">expression</unop> + + This element represents unary operator, with the op attribute specifying + which operator. Whitespace at the end of this line. + +<sumof ref="identifier" /> + + This element represents sumation of elements of referenced list. + +<popcount>expression</popcount> + + This element represents number of bits set. + Going off on a tangent here: if popcount goes in, then we don't need the <valueparam> element in the schema anymore. The reason is that the following two snippets are identical: <valueparam value-mask-type="CARD32" value-mask-name="value-mask" value-list-name="value-list" /> <field type="CARD32" name="value-mask" /> <list name="value-list" type="CARD32"> <popcount><fieldref>value-mask</fieldref></popcount> </list> diff --git a/src/xcb.xsd b/src/xcb.xsd index f3fcb6f..d94ba6e 100644 --- a/src/xcb.xsd +++ b/src/xcb.xsd @@ -46,6 +46,9 @@ authorization from the authors. </xsd:complexType> </xsd:element> + <!-- Alignment = pad(expr) --> + <xsd:element name="align" /> + This appears to be unused in the XML and should be removed. diff --git a/src/xinput2.xml b/src/xinput2.xml new file mode 100644 index 0000000..b962620 --- /dev/null +++ b/src/xinput2.xml @@ -0,0 +1,700 @@ [snip] + <!-- Types --> + + <!-- BUTTONMASK --> + <typedef oldname="CARD32" newname="BUTTONMASK" /> + + <!-- DEVICEID --> + <typedef oldname="CARD16" newname="DEVICEID" /> + + <enum name="Devices"> + <item name="All"> <value>0</value> </item> Spaces before tabs here. + <item name="AllMaster"> <value>1</value> </item> + </enum> + + <!-- DEVICE --> + <typedef oldname="DEVICEID" newname="DEVICE" /> Do we really need both DEVICE and DEVICEID aliases for CARD16? Why can't we just use one or the other? + + <!-- DEVICEUSE --> + <enum name="DeviceUse"> + <item name="MasterPointer"> <value>1</value> </item> + <item name="MasterKeyboard"> <value>2</value> </item> + <item name="SlavePointer"> <value>3</value> </item> + <item name="SlaveKeyboard"> <value>4</value> </item> + <item name="FloatingSlave"> <value>5</value> </item> + </enum> + + <!-- FP1616 --> + <typedef oldname="INT32" newname="FP1616" /> + + <!-- FP3232 --> + <struct name="FP3232"> + <field name="integral" type="INT32" /> + <field name="frac" type="CARD32" /> + </struct> Spaces before tab here. Tangent: we will probably need some utility functions to convert these to floats and vice versa. + + <!-- CHAR8 --> + <typedef oldname="CARD8" newname="CHAR8" /> This is only used in two places, both of which are string names. The convention in the other files is to use the predefined type "char" for this. [snip] + + <!-- WhatProperty --> + <enum name="WhatProperty"> + <item name="Created"> <value>1</value> </item> + <item name="Deleted"> <value>0</value> </item> + <item name="Modified"> <value>2</value> </item> + </enum> This is kind of an odd name. Maybe PropertyWhat, PropertyChange, or PropertyEventType? /me looks at the size of the xkb.xml file and decides to skip the rest of the XML for now. Will do some more review when the libxcb stuff comes out and I can compile it. [snip] diff --git a/xcbgen/expr.py b/xcbgen/expr.py index 522e17d..f479041 100644 --- a/xcbgen/expr.py +++ b/xcbgen/expr.py @@ -57,6 +57,22 @@ class Expression(object): # Standard list with a fieldref self.lenfield_name = elt.text + elif elt.tag == 'enumref': + self.lenfield_name = elt.text + self.lenfield_type = elt.get('ref') + + elif elt.tag == 'bitcount': + self.op = 'bitcount' + self.lhs = Expression(list(elt)[0], parent) + self.rhs = int(elt.get('value')) + self.lenfield_name = self.lhs.lenfield_name + self.lenfield_type = 'CARD32' + + elif elt.tag == 'sumof': + self.op = 'sumof' + self.lenfield_name = elt.get('ref') + self.lenfield_type = elt.get('type') + There is extra whitespace on the two blank lines here, and some whitespace at the end of the third added line. Also I mentioned this above but there are a bunch of new schema elements that aren't added to the Python parser in this patchset. elif elt.tag == 'valueparam': # Value-mask. The length bitmask is described by attributes. self.lenfield_name = elt.get('value-mask-name') @@ -81,7 +97,7 @@ class Expression(object): else: # Notreached - raise Exception('XXX') + raise Exception('Unknown element: %s' % elt.tag) What, you don't like getting Python tracebacks ending with "Exception:XXX"? :-) _______________________________________________ Xcb mailing list Xcb@... http://lists.freedesktop.org/mailman/listinfo/xcb |
|
|
Re: XKEYBOARD protocol definitionPeter,
Some review below. I haven't seen the earlier work so this will be zero-based. There are some changes upstream that aren't in your repo. I needed to pull from xcb/proto to get recent changes. Obligatory whitespace comments: the indentation of the XML files is a little erratic. There is some flagged whitespace I can see with git diff --color, some of which I mention below but not all. --Eamon W. diff --git a/doc/xml-xcb.txt b/doc/xml-xcb.txt index feb9984..db86159 100644 --- a/doc/xml-xcb.txt +++ b/doc/xml-xcb.txt @@ -227,6 +227,17 @@ enum; the value is restricted to one of the constants named in the enum. CARD32. value-mask-name gives the field name of the bitmask, and value-list-name gives the field name of the list of values. +<switch name="identifier"> switch expression <bitcase> bitcase expression, fields </bitcase> +...</switch> + + This element represents conditional inclusion of fields. It can be viewed as + sequence of multiple if's: if ( switch expression & bitcase + expression ) is equal to bitcase expression, bitcase fields are included in structure. + It can be used only as the last field of structure. + I don't see any new xcbgen Python code to handle this new element. The only Python changes I see are to expr.py, adding new list length calculators. Are there additional Python changes forthcoming, perhaps in the libxcb changes? +<replyof request="identifier" name="identifier" /> + + This element makes reply of request a field of structure. This does not appear anywhere else and should be removed. Expressions ----------- @@ -256,3 +267,21 @@ Expressions The bit element represents a literal bitmask value in an expression. The integer must be in the range 0..31, expanding to (1<<n) in C. + +<enumref ref="identifier">enum item identifier</enumref> + + This elements represents reference to item of enum. s/elements/element/ + +<unop op="operator">expression</unop> + + This element represents unary operator, with the op attribute specifying + which operator. Whitespace at the end of this line. + +<sumof ref="identifier" /> + + This element represents sumation of elements of referenced list. + +<popcount>expression</popcount> + + This element represents number of bits set. + Going off on a tangent here: if popcount goes in, then we don't need the <valueparam> element in the schema anymore. The reason is that the following two snippets are identical: <valueparam value-mask-type="CARD32" value-mask-name="value-mask" value-list-name="value-list" /> <field type="CARD32" name="value-mask" /> <list name="value-list" type="CARD32"> <popcount><fieldref>value-mask</fieldref></popcount> </list> diff --git a/src/xcb.xsd b/src/xcb.xsd index f3fcb6f..d94ba6e 100644 --- a/src/xcb.xsd +++ b/src/xcb.xsd @@ -46,6 +46,9 @@ authorization from the authors. </xsd:complexType> </xsd:element> + <!-- Alignment = pad(expr) --> + <xsd:element name="align" /> + This appears to be unused in the XML and should be removed. diff --git a/src/xinput2.xml b/src/xinput2.xml new file mode 100644 index 0000000..b962620 --- /dev/null +++ b/src/xinput2.xml @@ -0,0 +1,700 @@ [snip] + <!-- Types --> + + <!-- BUTTONMASK --> + <typedef oldname="CARD32" newname="BUTTONMASK" /> + + <!-- DEVICEID --> + <typedef oldname="CARD16" newname="DEVICEID" /> + + <enum name="Devices"> + <item name="All"> <value>0</value> </item> Spaces before tabs here. + <item name="AllMaster"> <value>1</value> </item> + </enum> + + <!-- DEVICE --> + <typedef oldname="DEVICEID" newname="DEVICE" /> Do we really need both DEVICE and DEVICEID aliases for CARD16? Why can't we just use one or the other? + + <!-- DEVICEUSE --> + <enum name="DeviceUse"> + <item name="MasterPointer"> <value>1</value> </item> + <item name="MasterKeyboard"> <value>2</value> </item> + <item name="SlavePointer"> <value>3</value> </item> + <item name="SlaveKeyboard"> <value>4</value> </item> + <item name="FloatingSlave"> <value>5</value> </item> + </enum> + + <!-- FP1616 --> + <typedef oldname="INT32" newname="FP1616" /> + + <!-- FP3232 --> + <struct name="FP3232"> + <field name="integral" type="INT32" /> + <field name="frac" type="CARD32" /> + </struct> Spaces before tab here. Tangent: we will probably need some utility functions to convert these to floats and vice versa. + + <!-- CHAR8 --> + <typedef oldname="CARD8" newname="CHAR8" /> This is only used in two places, both of which are string names. The convention in the other files is to use the predefined type "char" for this. [snip] + + <!-- WhatProperty --> + <enum name="WhatProperty"> + <item name="Created"> <value>1</value> </item> + <item name="Deleted"> <value>0</value> </item> + <item name="Modified"> <value>2</value> </item> + </enum> This is kind of an odd name. Maybe PropertyWhat, PropertyChange, or PropertyEventType? /me looks at the size of the xkb.xml file and decides to skip the rest of the XML for now. Will do some more review when the libxcb stuff comes out and I can compile it. [snip] diff --git a/xcbgen/expr.py b/xcbgen/expr.py index 522e17d..f479041 100644 --- a/xcbgen/expr.py +++ b/xcbgen/expr.py @@ -57,6 +57,22 @@ class Expression(object): # Standard list with a fieldref self.lenfield_name = elt.text + elif elt.tag == 'enumref': + self.lenfield_name = elt.text + self.lenfield_type = elt.get('ref') + + elif elt.tag == 'bitcount': + self.op = 'bitcount' + self.lhs = Expression(list(elt)[0], parent) + self.rhs = int(elt.get('value')) + self.lenfield_name = self.lhs.lenfield_name + self.lenfield_type = 'CARD32' + + elif elt.tag == 'sumof': + self.op = 'sumof' + self.lenfield_name = elt.get('ref') + self.lenfield_type = elt.get('type') + There is extra whitespace on the two blank lines here, and some whitespace at the end of the third added line. Also I mentioned this above but there are a bunch of new schema elements that aren't added to the Python parser in this patchset. elif elt.tag == 'valueparam': # Value-mask. The length bitmask is described by attributes. self.lenfield_name = elt.get('value-mask-name') @@ -81,7 +97,7 @@ class Expression(object): else: # Notreached - raise Exception('XXX') + raise Exception('Unknown element: %s' % elt.tag) What, you don't like getting Python tracebacks ending with "Exception:XXX"? :-) _______________________________________________ Xcb mailing list Xcb@... http://lists.freedesktop.org/mailman/listinfo/xcb |
|
|
Re: XKEYBOARD protocol definitionEamon Walsh wrote:
> Some review below. I haven't seen the earlier work so this will be > zero-based. Thanks! This is a great help. > There are some changes upstream that aren't in your repo. I needed to > pull from xcb/proto to get recent changes. Of course. You were looking at the "messy history" version that builds upon the GSOC tree. That's my fault. I think I wasn't quite clear enough in my proposal. For what I'm actually planning to commit, please see git://anongit.freedesktop.org/~peterh/xkbproto proposed (ie the proposed branch), which I've just created. I will be rewriting this branch frequently, so you probably don't want to base anything on top of it. The master branch will continue to contain incremental commits, if that's easier for somebody to review. > Obligatory whitespace comments: the indentation of the XML files is a > little erratic. There is some flagged whitespace I can see with git > diff --color, some of which I mention below but not all. Fixed. Thanks. > +<switch name="identifier"> switch expression <bitcase> bitcase expression, fields </bitcase> > +...</switch> > + > + This element represents conditional inclusion of fields. It can be viewed as > + sequence of multiple if's: if ( switch expression & bitcase > + expression ) is equal to bitcase expression, bitcase fields are included in structure. > + It can be used only as the last field of structure. > + > > I don't see any new xcbgen Python code to handle this new element. The > only Python changes I see are to expr.py, adding new list length > calculators. Are there additional Python changes forthcoming, perhaps > in the libxcb changes? Quite the reverse. I don't plan to commit any python changes. My arm could possibly be twisted far enough[1] that I'd look at libxcb (and python code in general), but I haven't done that yet. > +<replyof request="identifier" name="identifier" /> > + > + This element makes reply of request a field of structure. > > This does not appear anywhere else and should be removed. Done. > +<enumref ref="identifier">enum item identifier</enumref> > + > + This elements represents reference to item of enum. > > s/elements/element/ Done (along with a number of other grammatical, style, and word-wrap issues). > +<sumof ref="identifier" /> > + > + This element represents sumation of elements of referenced list. > + > +<popcount>expression</popcount> > + > + This element represents number of bits set. > > > Going off on a tangent here: if popcount goes in, then we don't need the > <valueparam> element in the schema anymore. The reason is that the > following two snippets are identical: > > <valueparam value-mask-type="CARD32" > value-mask-name="value-mask" > value-list-name="value-list" /> > > <field type="CARD32" name="value-mask" /> > <list name="value-list" type="CARD32"> > <popcount><fieldref>value-mask</fieldref></popcount> > </list> All true. Even better would be to use <switch>(<bitcase></bitcase)+</switch> to replace valueparam, to allow generation of code that replaces xcb/util/aux. > diff --git a/src/xcb.xsd b/src/xcb.xsd > + <!-- Alignment = pad(expr) --> > + <xsd:element name="align" /> > > This appears to be unused in the XML and should be removed. Done. > diff --git a/src/xinput2.xml b/src/xinput2.xml Not XKEYBOARD. I have not yet reviewed xinput2, nor do I intend to check it in. Thanks for looking, though. I will keep this handy for when I do get around to xinput2. > Tangent: we will probably need some utility functions to convert these > to floats and vice versa. The same utility functions could be used for RENDER's fixed point values, too. > /me looks at the size of the xkb.xml file and decides to skip the rest > of the XML for now. Will do some more review when the libxcb stuff > comes out and I can compile it. That... could take a while. Thanks again for the review. Peter Harris [1] I haven't spent enough time with python to really be comfortable with it yet. Every time I look at python, it either feels like perl with most of the expressiveness taken away, or lua with piles of cruft heaped on top. It could be worse, though. It could be C... -- Open Text Connectivity Solutions Group Peter Harris http://connectivity.opentext.com/ Research and Development Phone: +1 905 762 6001 pharris@... Toll Free: 1 877 359 4866 _______________________________________________ Xcb mailing list Xcb@... http://lists.freedesktop.org/mailman/listinfo/xcb |
|
|
Re: XKEYBOARD protocol definitionOn Wed, Nov 11, 2009 at 04:41:06PM -0500, Peter Harris wrote:
> Eamon Walsh wrote: > > +<sumof ref="identifier" /> > > + > > + This element represents sumation of elements of referenced list. > > + > > +<popcount>expression</popcount> > > + > > + This element represents number of bits set. > > > > > > Going off on a tangent here: if popcount goes in, then we don't need the > > <valueparam> element in the schema anymore. The reason is that the > > following two snippets are identical: > > > > <valueparam value-mask-type="CARD32" > > value-mask-name="value-mask" > > value-list-name="value-list" /> > > > > <field type="CARD32" name="value-mask" /> > > <list name="value-list" type="CARD32"> > > <popcount><fieldref>value-mask</fieldref></popcount> > > </list> Yes, *please*. I'd love to see valueparam die in favor of something simpler and more general. > All true. Even better would be to use > <switch>(<bitcase></bitcase)+</switch> to replace valueparam, to allow > generation of code that replaces xcb/util/aux. Both seem useful, I think, depending on the situation. If you truly just have a set of identical values and a bitmask indicating which ones you've supplied, a list with length given by a <popcount> seems perfect. If you want to supply slightly different values for different cases, or otherwise do anything out of the ordinary, <bitcase> seems preferable. Can you give an example of replacing a standard <valueparam> with <bitcase>? - Josh Triplett _______________________________________________ Xcb mailing list Xcb@... http://lists.freedesktop.org/mailman/listinfo/xcb |
|
|
Re: XKEYBOARD protocol definitionJosh Triplett wrote:
> On Wed, Nov 11, 2009 at 04:41:06PM -0500, Peter Harris wrote: >> Eamon Walsh wrote: >>> Going off on a tangent here: if popcount goes in, then we don't need the >>> <valueparam> element in the schema anymore. The reason is that the >>> following two snippets are identical: >>> >>> <valueparam value-mask-type="CARD32" >>> value-mask-name="value-mask" >>> value-list-name="value-list" /> >>> >>> <field type="CARD32" name="value-mask" /> >>> <list name="value-list" type="CARD32"> >>> <popcount><fieldref>value-mask</fieldref></popcount> >>> </list> > > Yes, *please*. I'd love to see valueparam die in favor of something > simpler and more general. > >> All true. Even better would be to use >> <switch>(<bitcase></bitcase)+</switch> to replace valueparam, to allow >> generation of code that replaces xcb/util/aux. > > Both seem useful, I think, depending on the situation. If you truly > just have a set of identical values and a bitmask indicating which ones > you've supplied, a list with length given by a <popcount> seems perfect. Agreed. > If you want to supply slightly different values for different cases, or > otherwise do anything out of the ordinary, <bitcase> seems preferable. > > Can you give an example of replacing a standard <valueparam> with > <bitcase>? By using <bitcase>, you can vary the type of the data used. In a hypothetical future server-side XCB, you could auto-generate boilerplate that checks to make sure that CreateWindow/BorderPixmap is, in fact, a valid Pixmap XID. This may also be useful for type-checking bindings (perhaps Haskell - Antoine?). For a slightly less hypothetical example, you can link the appropriate enums to individual fields. This will let the Wireshark dissector pretty-print RENDER/ChangePicture/repeat as "RepeatReflect" instead of "3". Once I update the wireshark dissector generator, that is. (RENDER example used instead of CreateWindow, since core X11 is still dissected by the legacy X11 dissector in Wireshark.) Peter Harris -- Open Text Connectivity Solutions Group Peter Harris http://connectivity.opentext.com/ Research and Development Phone: +1 905 762 6001 pharris@... Toll Free: 1 877 359 4866 _______________________________________________ Xcb mailing list Xcb@... http://lists.freedesktop.org/mailman/listinfo/xcb |
|
|
Re: XKEYBOARD protocol definitionOn Wed, Nov 11, 2009 at 5:45 PM, Peter Harris <pharris@...> wrote:
> By using <bitcase>, you can vary the type of the data used. In a > hypothetical future server-side XCB, you could auto-generate boilerplate > that checks to make sure that CreateWindow/BorderPixmap is, in fact, a > valid Pixmap XID. This may also be useful for type-checking bindings > (perhaps Haskell - Antoine?). > > For a slightly less hypothetical example, you can link the appropriate > enums to individual fields. This will let the Wireshark dissector > pretty-print RENDER/ChangePicture/repeat as "RepeatReflect" instead of > "3". Once I update the wireshark dissector generator, that is. (RENDER > example used instead of CreateWindow, since core X11 is still dissected > by the legacy X11 dissector in Wireshark.) If I could link the allowed fields in a value-param to their type at compile-time that would be pretty special, and that sounds like what you're aiming for. I'm still not sure what that would look like with the new bitcase syntax - I haven't been following too closely. At minimum, the list+popcount approach lets us get away from the value-mask bitwidth hack. Antoine _______________________________________________ Xcb mailing list Xcb@... http://lists.freedesktop.org/mailman/listinfo/xcb |
| Free embeddable forum powered by Nabble | Forum Help |