Add must_use to the cookie types

[SUPERCILEX]

I've goofed this several times now and wasted a few hours trying to understand why nothing would happen only to realize I forgot to call check (which I believe flushes requests). This would make that bug very hard to write.

[SUPERCILEX]

Hmmm, for folks that don't call check this might be way too annoying. Not sure what the right call is here.

[psychon]

Note that "just drop the cookie" explicitly appears in the docs as an option: https://docs.rs/x11rb/latest/x11rb/cookie/index.html#handling-x11-errors

With your code changes, how would I handle errors asynchronously, i.e. without forcing a round-trip to the X11 server (which is what check() does)? Right now "just drop the cookie and the error shows up in wait_for_event()" would be the approach.

only to realize I forgot to call check (which I believe flushes requests).

For what it's worth: In "my projects", I always try to arrange for flush() to be called by the event loop before sleeping on requests. This doesn't work that well when threads are involved (since it's racy), but most of the time I was working without threads, I think.

Random example: Add conn.flush()?; before this line and then you can remove all the calls to check() everywhere (and even can be sure that there are few syscalls happening (but I guess the performance win of that would be quite small and no one cares about X11 across the internet and the interaction anyway...)): https://github.com/SUPERCILEX/clipboard-history/blob/14af39be4b1cd101df90ead1318048bab4f92ed0/x11/src/main.rs#L361

[notgull]

Maybe we add a method to VoidCookie to the tune of "ignore" (separate from ignore_error) where you don't want to consider it at all, then we make it must_drop?

[SUPERCILEX]

With your code changes, how would I handle errors asynchronously

See my second commit: you have to explicitly drop it either with drop or let _ = cookie. I agree that this is pretty annoying, hence why I'm not sure this is a good idea. I wish there was a way to know if the programmer uses flush. Then again, maybe this is how you read minds? If I see let _ = then I know I should look for a flush somewhere. So maybe this is a good idea? Dunno.

Random example [...]

Damn, digging through my projects is some A grade support lol. 😁 I'm always a fan of improved perf so yeah that's a good way to do things. Implemented in SUPERCILEX/clipboard-history@0eca6dc. I personally wouldn't mind being forced to write let _ = so I remember the request is occurring asynchronously.

---

@notgull Eyyy, glad to see you here! What's the benefit over calling drop or using an empty let binding?

[notgull]

What's the benefit over calling drop or using an empty let binding?

It gives it semantic meaning.

[SUPERCILEX]

Good point. Maybe we call it check_later since that's what's happening?

[psychon]

Damn, digging through my projects is some A grade support lol.

Your're welcome. :-)

You wrote that you had some problems with this and I wanted to find out what exactly you were doing and whether the advice I give would be helpful at all. Luckily, one of your first Rust repos was a hit (I also looked at how winit deals with this and couldn't figure it out...).

Maybe we call it check_later since that's what's happening?

If we want to go that route (@notgull seems to be on board and I am neutral), I would propose another color for that bike shed: How about error_as_event "since that's what's happening"? I'm feel like I am not really good at naming, but I also feel like this name could be more intuitive (since the "later" in check_later made me think "when is later?").

Also, in that case the table in the docs that I linked to above should be updated, I guess? Or is check_later() / error_as_event() simply implemented as fn check_later(self) { std::mem::drop(self); } and so technically the table in the docs is still correct?

[SUPERCILEX]

You wrote that you had some problems with this

Yeah my specific issue was that I called set_selecrion_owner without a check or flush and spent like an hour trying to figure it out.

fn check_later(self) { std::mem::drop(self); }

Yup, that's all it'd do.

Naming wise, maybe check_in_event?

If we want to go that route

Yeah, do you know of any other invested parties that could weigh in? This could be a pretty big change, so it'd be nice to know how annoyed people would be. 😅