rust-x-bindings / rust-xcb

Rust bindings and wrapper for XCB.
MIT License
161 stars 63 forks source link

Add `poll_for_reply` and `poll_for_reply_unchecked` functions #245

Closed Antikyth closed 9 months ago

Antikyth commented 10 months ago

This PR adds poll_for_reply and poll_for_reply_unchecked functions - the async equivalents of the blocking wait_for_reply and wait_for_reply_unchecked. The poll API is based on the difference between wait_for_event and poll_for_event.

Where wait_for_reply and wait_for_reply_unchecked are the Rust bindings to xcb_wait_for_reply64, poll_for_reply and poll_for_reply_unchecked are the Rust bindings to xcb_poll_for_reply64.

Closes #244.

Antikyth commented 10 months ago

The changes to wait_for_reply and wait_for_reply_unchecked are just:

sprocklem commented 9 months ago

Hello @Antikyth, poll_for_reply_unchecked still seems to have the wrong behaviour. The documentation in the header is a little misleading here, but xcb_poll_for_reply* return 1 if XCB has received a corresponding reply or error, or 0 if no response of either type has been received. A return value of 1 with a null reply (where the current code panics) is perfectly valid: it is returned if an error was received by the client even though the error is not returned by the function.

This is more of a personal opinion thing, but it also seems to me that the Options are inverted: it seems like a request that is still waiting on a response should return Ok(None), while a request that has received an error (and, therefore, has not reply) should return Ok(Some(None)).

Edited to add: xcb_poll_for_reply* also returns 1 with both reply and error set to null if there is a connection error. You should insert a self.has_error()? after getting a return value of 1 in both functions, because the current code panics on connection error.

The return value is really a "would not block" boolean, and should be treated as such.

rtbo commented 9 months ago

@sprocklem is right, the return value of xcb_poll_for_reply indicates whether the request is finished. If request is finished we have to check if it is a reply or an error.

Therefore the return value of Connection::poll_for_reply should be Option<Result<C::Reply>> rather than Result<Option<Reply>>. And to be consistent with Connection::wait_for_reply_unchecked, Connection::poll_for_reply_unchecked should return Option<ConnResult<Option<C::Reply>>>.

To summarize

checked unchecked
blocking wait_for_reply -> Result<C::Reply> wait_for_reply_unchecked -> ConnResult<Option<C::Reply>>
non-blocking poll_for_reply -> Option<Result<C::Reply>> poll_for_reply_unchecked -> Option<ConnResult<Option<C::Reply>>>
rtbo commented 9 months ago

Closed in favor of #248. Thank you @Antikyth for bringing this up!