arminfriedl / xcb-wm

Rust implementation of xcb-wm - icccm and ewmh extensions for xcb
MIT License
5 stars 3 forks source link

Consider optionally owning the xcb connection #2

Open vikigenius opened 2 years ago

vikigenius commented 2 years ago

This is a follow up to my post in the Rust Forum

Perhaps it's a good idea to store either Cow<'a, xcb::Connection> instead of &'a xcb::Connection in the ewmh connection Struct or even be generic over AsRef<xcb::Connection> so that it can optionally own the connection ?

I can contribute a PR if you think this is a good idea but don't have time to do this yourself.

arminfriedl commented 2 years ago

Hi @vikigenius. Thanks for the suggestion. I think this is definitely an idea worth exploring. I have to think a bit about the implications still.

vikigenius commented 2 years ago

Or alternatively if generics is too complicated, we could just own the xcb::Connection but implement Deref so that people can easily access the underlying connection if needed

arminfriedl commented 2 years ago

I was looking a bit into your first suggestion. These bounds would need either AsRef or Clone to be implemented on the underlying xcb::Connection, which is currently not the case I believe. Other than that, AsRef seems like a good idea to me.

Regarding actually enforcing ownership of the xcb::Connection with a Deref - this was not done on purpose. Conceptually, xcb-wm is using xcb to send some additional protocol messages through it. The connection and the majority of the X11 protocols are xcb and that's the types that should be kept around (and sometimes also the types expected by other libraries). Taking ownership is too intrusive, although I understand that in situations like your example it can make more sense. Before xcb-wm I used https://github.com/meh/rust-xcb-util (which takes ownership) and this was a pain point.

Regarding your example I think the best way to go right now is to store the xcb::Connection in the struct and wrap it into an ewmh connection only temporarily when you need it. This wrapping should be cheap enough. Once the atoms are interned they aren't interned again (not in the sense that they are sent to the X11 server anyways). It's a bit of boilerplate but not too bad I hope.

vikigenius commented 2 years ago

Thanks, for the suggestion I see your point and I managed to solve the problem I have the way you suggested. But this whole issue and discussion is fascinating to me after I have spent some time reading about the Rust view on inheritance. I would like to understand better some of your thought process since I am a relative beginner to Rust, please don't take it as a criticism of your approach.

This brings me to a general problem with Rust that I have found. Composition and Delegation.

The ultimate goal of this create is to:

  1. Extend the functionality of the xcb connection by providing it the ability to send ewmh messages. Essentially extending the protocol.

The way I see it there are two major ways to achieve this.

  1. Just implement traits on the existing connection type
  2. Create a wrapper over the existing connection type (You have chosen this approach)

You can't chose 1 because you have chosen to add additional state to the connection (the atoms). But why is that needed? You are not using it anywhere except for when interning all of them initially iiuc. Maybe we can just add a trait with a setup_ewmh method and call it to intern everything ewmh and forget about the atoms struct so that a wrapper connection is not needed at all?

But now coming to the 2nd approach (wrapper) it is pretty diverse, this can be further broken down on how you plan to use the wrapper.

  1. You don't own the connection, you just take a reference to it so that you can continue using the original connection for it's original purposes.
  2. You own the original connection but expose it as a public member. The burden falls to the consumer to figure out when to use your methods vs the original. (But 1 also has the same issue)
  3. You own the original connection but do not expose it. You try to provide a convenient way to delegate all the original functionality to the original connection and the additional functionality to your methods.

When it comes to 3, Rust has no easy way of delegation so you either have to manually copy paste all the implementations of your inner type. Or use Deref, which seems like an anti-pattern, since that is not it's original purpose. But at least in this specific case it kind of makes sense, since an EMH connection is an X connection.

vikigenius commented 2 years ago

Sorry for the huge wall of text, but if you are interested I would like to get your thoughts on this, because I seem to run into similar problems a lot when designing wrapper types.

vikigenius commented 1 year ago

@arminfriedl so xcb now does implement AsRef for Connection like I suggested https://github.com/rust-x-bindings/rust-xcb/pull/206. So if this is something that you are still interested in, we can go ahead with taking generic over AsRef<Connection>