[user202729]

Fix #206 .

Diff of generated code looks like this. (Though it also kind of show up in test/generator/.)

--- a/xcffib/xinput.py
+++ b/xcffib/xinput.py
@@ -2825,6 +2843,7 @@
     KeyRepeat = 1 << 16
 class KeyPressEvent(xcffib.Event):
     xge = True
+    event_number = 2
     def __init__(self, unpacker):
         if isinstance(unpacker, xcffib.Protobj):
             unpacker = xcffib.MemoryUnpacker(unpacker.pack())
@@ -2880,6 +2899,7 @@
 _events[2] = KeyPressEvent
 class KeyReleaseEvent(xcffib.Event):
     xge = True
+    event_number = 3
     def __init__(self, unpacker):
         if isinstance(unpacker, xcffib.Protobj):
             unpacker = xcffib.MemoryUnpacker(unpacker.pack())
--- a/xcffib/xproto.py
+++ b/xcffib/xproto.py
@@ -387,6 +387,7 @@
     _None = 0
 class KeyPressEvent(xcffib.Event):
     xge = False
+    event_number = 2
     def __init__(self, unpacker):
         if isinstance(unpacker, xcffib.Protobj):
             unpacker = xcffib.MemoryUnpacker(unpacker.pack())
@@ -420,6 +421,7 @@
 _events[2] = KeyPressEvent
 class KeyReleaseEvent(xcffib.Event):
     xge = False
+    event_number = 3
     def __init__(self, unpacker):
         if isinstance(unpacker, xcffib.Protobj):
             unpacker = xcffib.MemoryUnpacker(unpacker.pack())

The name event_number instead of say event_code was chosen to match XML number="..." attribute. Its caller call it opcode though.

Possible tech debts created/bad design choices made:

• the pair of functions mkXClass and mkXEventClass sound like the former is a more general case of the latter, but this is in fact false, since the former does not support event_number attribute.
  • → rename mkXClass to mkXNonEventClass? This name sounds clumsy though.
• all call sites of mkXClass pass xge = False now.
  • → refactor?
• the event_number does not clash with anything preexisting, so no backward compatibility concern, but it means different things:
  • core event e.g. xcffib.xproto.KeyPressEvent: response_type = event_number. Add 0x80 if received an event from the X server generated from a SendEvent request.
  • regular extension event e.g. xcffib.xfixes.SelectionNotifyEvent: response_type = first_event + event_number. May add 0x80 as above.
  • XGE event e.g. xcffib.xinput.KeyPressEvent: response_type = 35, event_type = event_number.

Need discussion on the above.

Point 1 and 2 are easy (but leads to slightly more diff to review).

For point 3, if we don't want one homogeneous name, a possible solution is

• core event (xproto): base_response_type (contrast with response_type which may have the 0x80)
• regular extension event: response_type_offset (offset from first_event)
• XGE event: event_type (reuse, no concern of clash)

[tych0]

why isn't this event_number?

[tych0]

Oh, I see, viz. your discussion above...

[tych0]

core event (xproto): base_response_type (contrast with response_type which may have the 0x80)

I think we should use the same name for xproto and extension events, otherwise you have to keep track of a list of which events are in core to figure out how to decode things. The wire format will have the types as offset + event_number, so I would lean toward using that as the event_number, so that people don't have to do the decoding on their own.

Of course, that means your code here will need to change, since the extension's offset is determined at runtime by the x server.

XGE event e.g. xcffib.xinput.KeyPressEvent: response_type = 35, event_type = event_number.

Sounds reasonable.

[user202729]

I think we should use the same name for xproto and extension events

It's already using the same name (event_number), just that that thing has different meaning.

I would lean toward using [offset + event_number] as the event_number

In theory this sounds nice, but if I understood correctly, the offset is not known at generation time, and you have to ask the X server for that. For all I know, it may be different for each connection object?

people […] do the decoding on their own

Technically when people receive a message, the library already do the decoding and put the message into the correct Python type. So they shouldn't need to “figure out how to decode things”.

These constants are only needed when people want to access the raw underlying value for whatever reason, such as the FakeInput function. We expect that to be uncommon.

[tych0]

In theory this sounds nice, but if I understood correctly, the offset is not known at generation time, and you have to ask the X server for that. For all I know, it may be different for each connection object?

It depends on which extensions the server has enabled, so it should be stable for connections to the same server.

These constants are only needed when people want to access the raw underlying value for whatever reason, such as the FakeInput function. We expect that to be uncommon.

Right, but don't you have to pass the offset + event_number to FakeEvent? Or is it just event_number?

[tych0]

(Could probably use a test explicitly for your use case.)

[user202729]

https://gitlab.freedesktop.org/xorg/proto/xcbproto/-/blob/master/src/xtest.xml?ref_type=heads#L58-L81

Apparently this specific function only takes 5 distinct possibilities, all of them are fixed. This pull request adds event_number for other event classes because they just happen to use the same code path in the Haskell generator.

[tych0]

Yeah, so if you want to export those hard coded values that's fine too. But I do think a round-trip test would be good to include.

[user202729]

Added a test. Hope this is what you mean.

[tych0]

This is good enough, I was really thinking of one where you send an event through the X server as you're going to do in your code, to make sure our encoding matches what they expect. But assuming you've tested that in your work, I can land this.

[user202729]

This new feature doesn't really have anything to do with the X server. It literally just define constants event_number on various classes equal to some integer constants.

The code change is just replacing magic constants with slightly-less-magic constants. https://github.com/opensteno/plover/compare/d7a1dd1cdc3faa7cf5ec03047fc703f2c3b7f19c..525110a6a0c7270adc58200f3021d7fe204af774 (But yes, I tested this one by hand.)

[user202729]

Cool. Can you make a new release as well?

[tych0]

Yeah, let me land this memory leak fix and then I'll do one. Hopefully tomorrow night.

[user202729]

Sure, no rush. Just in case you forget it.

[tych0]

Just tagged v1.13.0 with both of your patches in it.