Dan Cross cross
cross commented on pull request vitrine/wmaker#22 2026-03-16 15:38:09 -04:00
Utilities for X11 image capture and acceptance testing

So this gets into some of the weird semantics between POSIX locks and flock, but do you want to lock against threads in your own process, or other processes? Or both? The use of Atomics suggests that the former is at least on your radar.

cross commented on pull request vitrine/wmaker#22 2026-03-16 15:28:31 -04:00
Utilities for X11 image capture and acceptance testing

Given that you do Not want to collide with an actual X server process (or ssh daemon simulating one), why not use these names as the lock file directly? It's not just that you have your own namespace to protect colliding with an already-running server, but also that a server started subsequently to you running could collide with you (though since it involves binding a socket, that is unlikely). But an O_EXCL create here would pretty much guarantee isolation between actual X servers and you.

cross commented on pull request vitrine/wmaker#22 2026-03-16 15:25:55 -04:00
Utilities for X11 image capture and acceptance testing

Oh! You do the first. I got confused by searching for /tmp/.X{}-lock, but that's to try and avoid racing against another process starting an X server.

The big issue I'd worry about, I suppose,…

cross commented on pull request vitrine/wmaker#22 2026-03-16 15:22:22 -04:00
Utilities for X11 image capture and acceptance testing

So...A couple of things you can do to make this a bit more robust.

cross commented on pull request vitrine/wmaker#22 2026-03-16 09:51:27 -04:00
Utilities for X11 image capture and acceptance testing

I think the return statements here are redundant: the match is the only expression in this function.

cross commented on pull request vitrine/wmaker#17 2026-02-15 11:54:53 -05:00
Simplify WINGs handlers API and reimplement in Rust

Is there a good reason not to change the type signature on the C side to make this explicitly unsigned? A negative number there feels relativistically impossible?

cross commented on pull request vitrine/wmaker#17 2026-02-15 11:54:53 -05:00
Simplify WINGs handlers API and reimplement in Rust

I don't quite get this; it appears to not just check, but consume and drop, global idle handlers. Perhaps the name could be changed to reflect that? run_global_idle_handlers or something? The repeated with_global_handlers blocks are interesting; I believe that's intended for the case where an idle handler is added during processing?

cross commented on pull request vitrine/wmaker#17 2026-02-15 11:54:53 -05:00
Simplify WINGs handlers API and reimplement in Rust

(Similarly, a comment here would be nice.)

cross commented on pull request vitrine/wmaker#17 2026-02-15 11:54:53 -05:00
Simplify WINGs handlers API and reimplement in Rust

Nit: a couple of do comments on this struct and callback would be nice.

cross commented on pull request vitrine/wmaker#10 2025-12-12 08:45:43 -05:00
Start addressing some bugs in new Rust code

I had thought at one point that it might be nice to try to make this generic over some type T, and bifurcate usage between c_void and usize, but looking at the sheer volume of places this code is used, I think that would be tedious and premature. It's a bummer that we have to do a lot of pointer casting as a result, but ... c'est la vie.

cross commented on pull request vitrine/wmaker#10 2025-12-12 07:31:51 -05:00
Start addressing some bugs in new Rust code

oh, hmm. No, not really, then. In that case, I'd recommend using the "provenance" based functions, though: .with_addr and so forth, if that's reasonable: https://doc.rust-lang.org/std/ptr/index.h…

cross approved vitrine/wmaker#9 2025-12-12 07:28:39 -05:00
Rewrite WINGs wfont.c in Rust

This is certainly an improvement! I'm a bit surprised there aren't crates providing better bindings to both Xft and pango, but that's a separate matter.

cross commented on pull request vitrine/wmaker#9 2025-12-12 07:23:08 -05:00
Rewrite WINGs wfont.c in Rust

Instead of the comments, why not give these proper names via let bindings?

cross approved vitrine/wmaker#7 2025-12-12 07:17:35 -05:00
Rewrite WUtils notification.c in Rust
cross commented on pull request vitrine/wmaker#10 2025-12-02 13:59:03 -05:00
Start addressing some bugs in new Rust code

Could you use .cast() here instead of as (and similarly elsewhere)?

cross commented on pull request vitrine/wmaker#7 2025-12-02 13:56:43 -05:00
Rewrite WUtils notification.c in Rust

The actions in the match here are repeated verbatim in all cases (I think). Perhaps delegate to a small helper function that implements just that logic?

cross commented on pull request vitrine/wmaker#7 2025-12-02 13:55:16 -05:00
Rewrite WUtils notification.c in Rust

It strikes me that if you used BTreeMap here instead of HashMap, you could make the constructor for NotificationCenter by pub const fn new() and do away with the OnceLock bits in with_global_default, instead using a static Mutex<NotificationCenter>.

cross commented on pull request vitrine/wmaker#7 2025-12-02 13:49:00 -05:00
Rewrite WUtils notification.c in Rust

"...must be guaranteed by code..." (missing the word "by").