Rust implementation #2

Copy link

ifreund commented 2026年03月10日 09:27:53 +01:00

Is it too awkward to store the xkb_bindings and pointer_bindings in the Seat struct instead? Or is there some other reason these are global state currently?

Is it too awkward to store the xkb_bindings and pointer_bindings in the Seat struct instead? Or is there some other reason these are global state currently?

jandrews271 commented 2026年03月11日 12:33:52 +01:00

It's a little awkward, but we could do it.

The issue is that on binding events we need to lookup the binding using the proxy id without having the seat itself. There are two alternatives I can think of:

1. Keep the bindings on the seats, and do a linear scan of all seats, or
2. Use user data to pass the seat with the binding (like the C implementation does)

(1) would work fine, but if we're always accessing the bindings from the window manager, there's not much point to putting it on the seat

(2) is a little awkward in Rust because of the ownership model. We'd need to use something like an Rc<Refcell<Seat>>. I almost did this, but felt like it would add complexity for not much advantage.

I can see the argument for (2). On the one hand it makes the code a little harder to follow, which isn't ideal for a simple example. On the other hand, it would show a Rust pattern for using user data, which might be good to have in the example.

Let me know what you think makes sense. I honestly went back and forth on this question half a dozen times before deciding to just do the easier version.

It's a little awkward, but we could do it. The issue is that on binding events we need to lookup the binding using the proxy id without having the seat itself. There are two alternatives I can think of: 1. Keep the bindings on the seats, and do a linear scan of all seats, or 2. Use user data to pass the seat with the binding (like the C implementation does) (1) would work fine, but if we're always accessing the bindings from the window manager, there's not much point to putting it on the seat (2) is a little awkward in Rust because of the ownership model. We'd need to use something like an `Rc<Refcell<Seat>>`. I almost did this, but felt like it would add complexity for not much advantage. I can see the argument for (2). On the one hand it makes the code a little harder to follow, which isn't ideal for a simple example. On the other hand, it would show a Rust pattern for using user data, which might be good to have in the example. Let me know what you think makes sense. I honestly went back and forth on this question half a dozen times before deciding to just do the easier version.

ifreund commented 2026年03月11日 12:43:40 +01:00

I see, that is indeed more awkward than I realized.

What if we set the user data of the bindings to the seat's ObjectId rather than a Rc/RefCell wrapped direct reference to the seat?

If that's also too awkward I'm fine with keeping the status quo.

I see, that is indeed more awkward than I realized. What if we set the user data of the bindings to the seat's `ObjectId` rather than a Rc/RefCell wrapped direct reference to the seat? If that's also too awkward I'm fine with keeping the status quo.

jandrews271 commented 2026年03月11日 15:26:37 +01:00

Passing the ObjectId is actually perfect. This is a huge improvement, and I think the pattern of passing around ObjectIds as user data is probably the right default for Rust since it bypasses ownership issues neatly, so having that in the example is great.

Passing the `ObjectId` is actually perfect. This is a huge improvement, and I think the pattern of passing around `ObjectId`s as user data is probably the right default for Rust since it bypasses ownership issues neatly, so having that in the example is great.

ifreund commented 2026年03月11日 18:22:06 +01:00

Perfect :)

Perfect :)

ifreund commented 2026年03月10日 09:20:33 +01:00

The Go implementation of tinyrwm uses an interface for seat operations and encapsulates the operation-specific state, which I think worked out quite nicely.

Perhaps it would be more idiomatic Rust to have a SeatOp trait with implementations for move and resize operations?

The Go implementation of tinyrwm uses an interface for seat operations and encapsulates the operation-specific state, which I think worked out quite nicely. Perhaps it would be more idiomatic Rust to have a `SeatOp` trait with implementations for move and resize operations?

jandrews271 commented 2026年03月11日 14:23:18 +01:00

Updated! I didn't follow the Go implementation exactly. For Rust at least, keeping the proxy in the enum with the rest of the operation-specific state is cleaner, and it didn't seem worth having a separate struct just for op_dx and op_dy.

There is an argument for putting op_dx and op_dy into the enum too. It would require updating the OpDelta handler to use a match statement and handle move and resize separately. Arguably that's more correct since in principle different operations might want to do different things on an OpDelta event, but it does add a bunch of lines of code for the same behavior.

I'm on the fence, so let me know if you have a preference!

Updated! I didn't follow the Go implementation exactly. For Rust at least, keeping the proxy in the enum with the rest of the operation-specific state is cleaner, and it didn't seem worth having a separate struct just for `op_dx` and `op_dy`. There is an argument for putting `op_dx` and `op_dy` into the enum too. It would require updating the `OpDelta` handler to use a `match` statement and handle move and resize separately. Arguably that's more correct since in principle different operations might want to do different things on an `OpDelta` event, but it does add a bunch of lines of code for the same behavior. I'm on the fence, so let me know if you have a preference!

ifreund commented 2026年03月11日 18:29:58 +01:00

I think this is great as is, I don't think putting op_dx and op_dy into the enum would be an improvement :)

I think this is great as is, I don't think putting `op_dx` and `op_dy` into the enum would be an improvement :)

ifreund commented 2026年03月10日 09:15:57 +01:00

This should only end the seat operation if the closed window is the target.

This should only end the seat operation if the closed window is the target.

jandrews271 commented 2026年03月11日 14:34:13 +01:00

Whoops! Should be fixed now.

Whoops! Should be fixed now.

ifreund commented 2026年03月10日 09:17:03 +01:00

This removed_ids hash set is unnecessary. I think it would be simpler to nest the xkb_bindings.retain() and pointer_bindings.retain() calls in the seats.retain() call.

This `removed_ids` hash set is unnecessary. I think it would be simpler to nest the `xkb_bindings.retain()` and `pointer_bindings.retain()` calls in the `seats.retain()` call.

jandrews271 commented 2026年03月11日 15:30:39 +01:00

This should be fixed with the bindings -> seat refactor. I originally had this in place to deal with some borrow checker complaints, but the new data model fixed that entirely.

This should be fixed with the bindings -> seat refactor. I originally had this in place to deal with some borrow checker complaints, but the new data model fixed that entirely.

ifreund commented 2026年03月10日 10:13:26 +01:00

Interesting, this differs from the other tinyrwm implementations which call seat.focus() directly here. I do kindof like this though...

Interesting, this differs from the other tinyrwm implementations which call `seat.focus()` directly here. I do kindof like this though...

jandrews271 commented 2026年03月11日 17:22:55 +01:00

So, this is definitely one source of differences from the C implementation.

In the C implementation, focusing each window in turn means if there are multiple seats doing simultaneous operations, we'll shuffle around the window list for each one, keeping a record of all that. This implementation only reorders based on the last operation, so the order of windows might be different.

Also, if somehow we've got a seat operation going on, but seat.interacted points at a different window (maybe multiple WindowInteraction events in the same manage cycle?), then the C implementation will prefer whatever last interaction happened, but this version will prefer the last move or resize.

I wasn't really sure how specific the tinyrwm focus spec is. I like the simplicity of this code, but if we want identical focus behavior, this definitely isn't it!

Let me know what you prefer on this front. Implementing an exact match for the C behavior will definitely be a little uglier because the borrow checker doesn't like me passing a mutable reference to self.windows to seat.pointer_move() while I'm iterating over the windows, so that's what I was trying to avoid, but maybe a little ugliness is the right approach here.

So, this is definitely one source of differences from the C implementation. In the C implementation, focusing each window in turn means if there are multiple seats doing simultaneous operations, we'll shuffle around the window list for each one, keeping a record of all that. This implementation only reorders based on the last operation, so the order of windows might be different. Also, if somehow we've got a seat operation going on, but `seat.interacted` points at a different window (maybe multiple `WindowInteraction` events in the same manage cycle?), then the C implementation will prefer whatever last interaction happened, but this version will prefer the last move or resize. I wasn't really sure how specific the `tinyrwm` focus spec is. I like the simplicity of this code, but if we want identical focus behavior, this definitely isn't it! Let me know what you prefer on this front. Implementing an exact match for the C behavior will definitely be a little uglier because the borrow checker doesn't like me passing a mutable reference to `self.windows` to `seat.pointer_move()` while I'm iterating over the windows, so that's what I was trying to avoid, but maybe a little ugliness is the right approach here.

ifreund commented 2026年03月10日 10:02:05 +01:00

The focus behavior of this rust implementation differs from that of the other tinyrwm implementations.

At least one bug I see in the rust focus logic is failing to reset seat.interacted to None after reading its value during a manage sequence.

The focus behavior of this rust implementation differs from that of the other tinyrwm implementations. At least one bug I see in the rust focus logic is failing to reset `seat.interacted` to `None` after reading its value during a manage sequence.

jandrews271 commented 2026年03月11日 16:43:51 +01:00

I think this probably isn't an issue. seat.interacted.take() resets the value to None (and is the idiomatic way to clear an Option and use the contents).

I think this probably isn't an issue. `seat.interacted.take()` resets the value to `None` (and is the idiomatic way to clear an `Option` and use the contents).

ifreund commented 2026年03月11日 18:23:41 +01:00

Ah my bad, I don't read or write rust very often and didn't realize the behavior of take.

I'll take a fresh look at the focus logic soon :)

Ah my bad, I don't read or write rust very often and didn't realize the behavior of take. I'll take a fresh look at the focus logic soon :)

ifreund commented 2026年03月10日 10:14:41 +01:00

This needs to move the window to the back of the windows deque to keep it in sync with rendering order. This is also contributing to the buggy focus behavior of this window manager implementation.

This needs to move the window to the back of the `windows` deque to keep it in sync with rendering order. This is also contributing to the buggy focus behavior of this window manager implementation.

ifreund commented 2026年03月10日 09:51:46 +01:00

It doesn't seem possible for this to be None, perhaps we should simply unwrap() here?

It doesn't seem possible for this to be None, perhaps we should simply unwrap() here?

jandrews271 commented 2026年03月11日 13:00:22 +01:00

Yup! Done.

Yup! Done.

ifreund commented 2026年03月10日 09:44:09 +01:00

I'd rather set window.closed = true directly here rather than having a single line handle_closed() function. The same goes for all the other trivial handle_foo() functions that currently exist.

I'd rather set `window.closed = true` directly here rather than having a single line `handle_closed()` function. The same goes for all the other trivial `handle_foo()` functions that currently exist.

jandrews271 commented 2026年03月11日 12:41:14 +01:00

Done!

That does feel better.

Done! That does feel better.

ifreund commented 2026年03月10日 09:46:05 +01:00

In what case is proxy.id() not present in state.wm.seats? I don't see one.

I think this should be an assertion (unwrap() in rust I guess) and find_or_log should be deleted.

In what case is proxy.id() not present in state.wm.seats? I don't see one. I think this should be an assertion (`unwrap()` in rust I guess) and `find_or_log` should be deleted.

jandrews271 commented 2026年03月11日 12:07:00 +01:00

I don't think it should ever happen without a bug. I think at the time I was thinking that logging an error message was better than panicking if it did happen, but given that it really shouldn't happen panicking seems fine to me.

Switched to expect() (unwrap() but with a message on failure). I also went through and made the use of unwrap() and expect() more consistent - defaulting to expect(), and only using unwrap() where the code is locally and obviously correct.

I don't think it should ever happen without a bug. I think at the time I was thinking that logging an error message was better than panicking if it did happen, but given that it really shouldn't happen panicking seems fine to me. Switched to `expect()` (`unwrap()` but with a message on failure). I also went through and made the use of `unwrap()` and `expect()` more consistent - defaulting to `expect()`, and only using `unwrap()` where the code is locally and obviously correct.

ifreund commented 2026年03月11日 18:24:41 +01:00

That seems like a nice approach to me :)

That seems like a nice approach to me :)

Copy link