jezek / xgb

The X Go Binding is a low-level API to communicate with the X server. It is modeled on XCB and supports many X extensions.
Other
130 stars 13 forks source link

KeyReleaseEvent.Bytes() doesn't write type #6

Closed tesselslate closed 2 years ago

tesselslate commented 2 years ago

I believe that KeyReleaseEvent.Bytes() should set the first byte of the event to 3, which is the KeyRelease event type. The current behavior with just returning the value of KeyPressEvent.Bytes() returns the bytes for a key press event (which has type 2), not a key release event.

https://github.com/jezek/xgb/blob/7a4fade77cf2a8406e63409d11ac844dfb150679/xproto/xproto.go#L3414-L3416

Manually setting the 0th byte to 3 after creates a proper KeyRelease event to be sent with SendEvent. (excerpt from my code)

    bytes := evt.Bytes()
    if press.State == KeyUp {
        bytes[0] = 3
    }

This same issue seems to be present with other event types as well, such as ButtonPressEvent and ButtonReleaseEvent.

https://github.com/jezek/xgb/blob/7a4fade77cf2a8406e63409d11ac844dfb150679/xproto/xproto.go#L528-L530

jezek commented 2 years ago

Yes, you're definitely right. I've created test programs in c (c++?) using xcb and go using xgb and the xcb version gives different first byte for key release.

$ ./x11KeyPressReleaseBytes_in_c
KeyPress  : 18
KeyPressBytes  : 02 18 02 00 0d f6 21 07 96 04 00 00 00 00 20 01 00 00 00 00 01 00 76 02 cf ff 05 02 00 00 01 00 
KeyRelease: 18
KeyReleaseBytes: 03 18 02 00 99 f6 21 07 96 04 00 00 00 00 20 01 00 00 00 00 01 00 76 02 cf ff 05 02 00 00 01 00 
KeyPress  : 9
KeyPressBytes  : 02 09 02 00 5a fa 21 07 96 04 00 00 00 00 20 01 00 00 00 00 01 00 76 02 cf ff 05 02 00 00 01 00 

$ ./x11KeyPressReleaseBytes_in_go
KeyPress  : 18
KeyPressBytes  :02 18 00 00 e7 1e 22 07 96 04 00 00 01 00 20 01 00 00 00 00 01 00 76 02 cf ff 05 02 00 00 01 00 
KeyRelease: 18
KeyReleaseBytes:02 18 00 00 87 1f 22 07 96 04 00 00 01 00 20 01 00 00 00 00 01 00 76 02 cf ff 05 02 00 00 01 00 
KeyPress  : 9
KeyPressBytes  :02 09 00 00 cf 24 22 07 96 04 00 00 01 00 20 01 00 00 00 00 01 00 76 02 cf ff 05 02 00 00 01 00 

Not that I've doubted you, but now there is a proof.

The thing is that the ./xproto/xproto.go file is generated by ./xgbgen (see MAKEFILE). The command for generation is xgbgen/xgbgen --proto-path /usr/share/xcb /usr/share/xcb/xproto.xml > xproto/xproto.go, or using make command make xproto.xml . The current xproto.go file and other were generated a long time ago on some previous version of xcb xml's (I think xcb-proto 1.10) and with my current version (1.14) it is not working (it panics).

What I want to say is that the solution to simply edit the xproto.go file would not be the proper solution. The proper solution would be to patch the xgbgen and then regenerate for the working xcb-proto version (1.10), or better for current xcb-proto version (1.14).

Help would be appreciated. Can you try to make the patches?

Note: I've seen @aarzilli has a branch with changes to xgbgen for xcb-proto 1.12, but I don't know if it works for 1.14. But it could be a good start.

tesselslate commented 2 years ago

Sure, I'll take a look at getting xgbgen running and patching it this weekend.

tesselslate commented 2 years ago

The actual fix for the initial issue is pretty simple but 1.14 support is more problematic. I've managed to get it running with 1.14 on my fork, but I think aarzilli's version might be better. My code has a few pretty bad workarounds for things I didn't understand within xgbgen.

All of the tests in xgb and xproto pass for me, but I'm far less confident about the extensions. I will have to write a few tests for those to see if they work or not. I'll post again when I have those working.

jezek commented 2 years ago

I see, the patch for the pre-1.14 version is really simple. Maybe before the bigger patch to 1.14, the code could be regenerated with your patch on the original xcb-proto version (1.10, 1.11?), so there will be only minimal changes fixing only this issue. After that the transition to newer xcb-proto versions can be done in new issue. Do you agree?

tesselslate commented 2 years ago

Sure, sounds good. I can make a PR with just the 3 line change if you want, or you can push it yourself since it's pretty simple.

jezek commented 2 years ago

Can you make a PR? Ideal would be 2 commits.

This way it should be easy to review the changes, because only the release events should change.

Thank you.