[Xcb] [PATCH resend 2/4] xkb: Fix key type map entry field order

[Ran Benita]

On Sun, Aug 11, 2013 at 10:49:02AM +0200, Daniel Martin wrote:
> On Tue, Aug 06, 2013 at 02:12:01PM +0300, Ran Benita wrote:
> > In fact, unlike the deleted comment says, both Xlib and the server use
> > the order as specified in the protocol spec:
> > http://www.x.org/releases/current/doc/kbproto/xkbproto.html#appD::Requests
> > (Search for KB_KTMAPENTRY).
> > 
> > Also see struct xkbKTMapEntryWireDesc in
> > /usr/include/X11/extensions/XKBproto.h
> > 
> > Reviewed-by: Peter Harris <pharris at opentext.com>
> > Signed-off-by: Ran Benita <ran234 at gmail.com>
> > ---
> >  src/xkb.xml | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/src/xkb.xml b/src/xkb.xml
> > index 15b3368..6de634e 100644
> > --- a/src/xkb.xml
> > +++ b/src/xkb.xml
> > @@ -410,12 +410,8 @@ authorization from the authors.
> >  
> >  	<struct name="KTMapEntry">
> >  		<field name="active" type="BOOL" />
> > -		<!-- Xlib uses a different arrangement of fields
> >  		<field name="mods_mask" type="CARD8" mask="ModMask" />
> >  		<field name="level" type="CARD8" />
> > -		-->
> > -		<field name="level" type="CARD8" />
> > -		<field name="mods_mask" type="CARD8" mask="ModMask" />
> >  		<field name="mods_mods" type="CARD8" mask="ModMask" />
> >  		<field name="mods_vmods" type="CARD16" mask="VMod" />
> >  		<pad bytes="2" />
> > -- 
> > 1.8.3.4
> 
> Your patch is
> Reviewed-by: Daniel Martin <consume.noise at gmail.com>
> 
> For the field names (i.e. mods_mods), which have been there before:
> NAK. It's the code generators job to choose strange names. That
> shouldn't be done in advance. ;)
> 
> Just scrolled through the xml and it looks like the other field names
> have been choosen based upon XKBproto.h and those names are much better
> (realMods vs. mods_mods, virtualMods vs. mods_vmods).
> 
> The Debian code search didn't hit anything but wireshark when searching
> for mods_mods and the struct was broken anyways. So, I would definitly
> sign it off, if you would change their names.

Yes these are kind of ugly. There is also the 'map_' prefix in the
{Get,Set}NamedIndicator request a bit below. I think the intention
behind this naming is to group them logically, like Xlib and the spec
do, while avoiding embeding the actual struct in the reply (why?). For
example with the indicator reqs, the spec says:

    1     CARD8          opcode
    1     15          xkb-opcode (    -- GetNamedIndicator)
    [...]

    1     1          Reply
    [...]
    1     BOOL          realIndicator
    1     KB_INDICATOR          ndx
    1     SETofKB_IMFLAGS          map.flags
    1     SETofKB_IMGROUPSWHICH          map.whichGroups
    1     SETofKB_GROUPS          map.groups
    1     SETofKB_IMMODSWHICH          map.whichMods
    1     SETofKEYMASK          map.mods
    1     SETofKEYMASK          map.realMods
    2     SETofKB_VMOD          map.vmods
    4     SETofKB_BOOLCTRL          map.ctrls
    1     BOOL          supported
    3               unused

Which is just a KB_INDICATORMAP (= xcb_xkb_indicator_map_t), or
KB_MODDEF (= xcb_xkb_mod_def_t) in the case you mentioned.

So if you still think the XKBproto.h names are better (and I do
agree..), I can send a patch for that.

Btw, while looking at this I found a few unrelated fixes, I'll send them
out now :)

Ran