Bug 51295

Summary: various problems with the xkb protocol bindings
Product: XCB Reporter: Uli Schlachter <psychon>
Component: ProtocolAssignee: xcb mailing list dummy <xcb>
Status: RESOLVED MOVED QA Contact: xcb mailing list dummy <xcb>
Severity: minor    
Priority: medium CC: consume.noise, gatis.paeglis, me
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: xkb: Rename enum EventType to EventTypeMask, add EventType

Description Uli Schlachter 2012-06-21 06:41:48 UTC
While looking at http://lists.freedesktop.org/archives/xcb/2012-June/007800.html, I noticed some more problems with the XKB bindings. Since they won't be fixed any time soon, let's make a meta-bug "xkb is ugly" to make sure that these issues don't get forgetten. So here it is.

The XKB extension has only a single event number. These events contain a xkbType field which then says which kind of event this actually is.
The current xkb.xml describes these events as seperate events, but from the POV of the generator, they are just a single event.

<psychon> daniels: <event name="BellNotify" number="8"> <- doesn't the "8" here mean that the event is xkb-event-base + 8?
[...]
<daniels> so technically having them as separate <event>s is wrong i think
<daniels> but i'm not sure how to cover loads of totally disparate events all having the same event type
<pharris> daniels: One event, with a giant <switch>.
[...]
<pharris> daniels: Or possibly a <union>, but I dislike <union> as it doesn't represent well in non-C languages.

The second issue is:

<enum name="EventType"> [...] <item name="BellNotify"> <bit>8</bit> </item>

This causes a #define XCB_XKB_EVENT_TYPE_BELL_NOTIFY 256 to be generated, but this actually wants a define with value 8.

The above was noticed while looking at this for 2 minutes and comparing it with Xlib's src/xkb/XKBUse.c, function wire_to_event() and proto/kbproto's X11/extension/XKBproto.h, struct XkbStateNotifyEvent. I bet that a closer inspection would find more problems.
Comment 1 Daniel Martin 2013-08-06 20:36:54 UTC
(In reply to comment #0)
> The XKB extension has only a single event number. These events contain a
> xkbType field which then says which kind of event this actually is.
> The current xkb.xml describes these events as seperate events, but from the
> POV of the generator, they are just a single event.
> 
> <psychon> daniels: <event name="BellNotify" number="8"> <- doesn't the "8"
> here mean that the event is xkb-event-base + 8?
> [...]
> <daniels> so technically having them as separate <event>s is wrong i think
> <daniels> but i'm not sure how to cover loads of totally disparate events
> all having the same event type
> <pharris> daniels: One event, with a giant <switch>.
> [...]
> <pharris> daniels: Or possibly a <union>, but I dislike <union> as it
> doesn't represent well in non-C languages.

That would be a switch/case. Which is no fun to add into the existing code generator.

I could think of a workaround/hack:
- transform the existing events into structures
- add one event with the structure like xkbAnyEvent
With that one could cast the incoming sub events into the appropriate structures and we would just have one event.

But, one day we'll have a switch/case (I promise, just a matter of [spare] time). If we're going to replace the workaround with the switch/case we'll have an API break.


> The second issue is:
> 
> <enum name="EventType"> [...] <item name="BellNotify"> <bit>8</bit> </item>
> 
> This causes a #define XCB_XKB_EVENT_TYPE_BELL_NOTIFY 256 to be generated,
> but this actually wants a define with value 8.

That's easy to fix, i.e. with the following attachment. Well "easy to fix" with the assumption: The generated event structures are incorrect and therefor the SelectEvents request hasn't been used by anyone. Because, that patch replaces XCB_XKB_EVENT_TYPE_BELL_NOTIFY, which potentially has been used to select events. The old one is renamed to XCB_XKB_EVENT_TYPE_MASK_BELL_NOTIFY.
So, if the assumption is wrong, then this is an API break and another name for the enum with the BellNotify=8 has to be choosen.
Comment 2 Daniel Martin 2013-08-06 20:37:26 UTC
Created attachment 83738 [details] [review]
xkb: Rename enum EventType to EventTypeMask, add EventType
Comment 3 GitLab Migration User 2019-02-16 19:39:54 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/xorg/proto/xcbproto/issues/4.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.