rust-x-bindings / rust-xcb

Rust bindings and wrapper for XCB.
MIT License
165 stars 64 forks source link

Don't panic for unknown events in xproto #142

Closed pr2502 closed 2 years ago

pr2502 commented 2 years ago

This changes the ResolveWireEvent::resolve_wire_event to return an Option. If we assume the X server is not misbihaving it's only possible for the base xproto to get an unparseable event (if the extension the event is from is not currently enabled) so I left the unreachable! in the other ones (and used unwrap on the options in src/event.rs:resolve_event).

Closes #141

rtbo commented 2 years ago

Thank for the PR. Everything looks good to me except that Event::Unknown should carry the mut pointer to xcb_generic_event for letting the possibility to treat the event in a custom way. Event should also have a custom Drop implementation to free this unresolved event and avoid memory leak. Event::Unresolved would also work BTW.

pr2502 commented 2 years ago

Embedding the pointer directly into the enum had the undesired side-effect that the Drop impl prevented users moving out of the whole enum (this was even triggered within the xcb codebase), which I think would make the API less ergonomic to use.

Instead I've added a wrapper type which implements BaseEvent and Drop.

pr2502 commented 2 years ago

Alternative would be to not implement the BaseEvent trait for UnresolvedEvent and only provide an as_raw and into_raw methods on the type itself.

rtbo commented 2 years ago

Alternative would be to not implement the BaseEvent trait for UnresolvedEvent and only provide an as_raw and into_raw methods on the type itself.

This would work too, but the x::SendEvent request expects an event implementing BaseEvent, so it would disable the possibility for someone to craft an event from scratch and send it to the server.

In this regards, Unknown is better than Unresolved. (the user crafts a C event, cast it to *mut xcb_generic_event_t and wraps it into a UnknownEvent to send to x::SendEvent) Sorry for going back and forth. Naming things is hard and I'm targetting v1.0 very soon, so I can't go back after.

pr2502 commented 2 years ago

Ok, that's no problem.

rtbo commented 2 years ago

All good thanks