Add client transport abstraction

[madushan1000]

I added a ClientTransport to abstract connect call. All the tests still pass and egl example still works.
But I'm not sure if all the changes I did are actually correct.

[madushan1000]

One problem I'm having already with this approach is that having to mention the wayland transport type in multiple structs/function definitions when writing a client. so it's a bit hard to make a client that would work with multiple transport types or a client that can determine the transport type at runtime.

[MaxVerevkin]

One problem I'm having already with this approach is that having to mention the wayland transport type in multiple structs/function definitions when writing a client.

You may write a type alias and use it everywhere.

a client that can determine the transport type at runtime.

If I understand correctly, virtio requires a very specific way to create pipes and shm buffers, so I'm not sure how feasible this is.

But if a client supporting many transports is realistic, then either we should

• impl Transport for Box<dyn Transport> (this may be a good addition in any case), which enables clients to opt-in for dynamic dispatch
• or even use dynamic dispatch unconditionally, and remove genericts. The cost is always using dynamic dispatch, which in this case is almost zero. One downside is that .transport() would always return &dyn Transport and not a specific T (and having a specific T would be convenient for virtio pipes and buffers, I assume).

By the way, I think we can remove the T: Transport bound from struct definitions and methods which don't require it. Similar to how HashMap does it. This would, for example, make generated request functions cleaner.

[MaxVerevkin]

Note: I think most of the T: ClientTransport requirements may be omitted, and only a few methods of Connection should require it. This would make generic code which does not deal with IO cleaner, for example the wayrs-scanner-generated code, wayrs-utils code, etc. Similar to how HashMap requires K: Hash only for methods that need it, even though HashMap with non-hashable keys is a practically useless type.

So for example

impl<D, T> Connection<D, T> {
    pub fn connect() -> Result<Self> where T: ClientTransport { ... }

    #[doc(hidden)]
    pub fn send_request(&mut self, iface: &'static Interface, request: Message) { .... }

    pub fn flush(&mut self, mode: IoMode) -> io::Result<()> where T: Transport { ... }
}

[madushan1000]

I adjusted the trait bound. Does that look correct?

[MaxVerevkin]

• This function really needs some docs.
• This is very wayrs-egl specific. Maybe it should be moved to its own trait EglTransport: ClientTransport?

[madushan1000]

Sounds like a good idea, I'll refactor it and add some docs soon

[MaxVerevkin]

How often do you call Connection::transport? I'm starting to think that is is better to store the transport as a Box<dyn Transport + Any + Send> to avoid all these generics everywhere, which make it easy to accidentally write libraries that are not generic over transport, thanks to the default T. But without default T writing normal clients is painful.

The transport can still be downcasted to a specific type, just with a small runtime check.

[madushan1000]

that will be called every time a buffer is attached to query buffer metadata from the host.

[madushan1000]

I took another look at this and since the overhead of doing a ioctl to get buffer metadata is much higher than accessing a trait object, I don't think there will be a big performance impact storing the transport as a trait object. I'm fine with this approach if it makes the api better.

[MaxVerevkin]

The performance overhead is basically zero and it shouldn't matter unless you call it thousands times per frame. By "how often" I meant how frequently it appears in the code, you would have to unwrap the option, which may be a bit annoying.

I think not having a new generic would make the API generally better. However I'm not sure how to handle wayrs-egl with its fix_metadata...

It doesn't matter that much, TBH.

[madushan1000]

It should only be called when a buffer is attached to a surface. so not a lot of places.
My current application (proxy composer for android) doesn't use wayrs-egl. fix_metadata call isn't even needed since I use minigbm in android and minigbm store the correct stride etc.. as buffer metadata that gets passed to the proxy composer.

[MaxVerevkin]

Let's drop the generic then

[madushan1000]

sounds good, I'll refactor it when I get some time.

[MaxVerevkin]

It is really annoying that default generics are not allowed in functions. This means that if we want to drop the generic, we will have to have two versions of each connect function - one generic and one for UnixStream. 6 connect functions is too much.

One solution to this is to remove connect_and_collect_globals and async_connect_and_collect_globals, and instead add Connection::collect_initial_globals(&mut self), with an async variant. I don't like it that much because calling it not directly after connect would be invalid. This could be enforced at runtime, though.

[MaxVerevkin]

I've rebased this branch and applied some changes in #16. Could you take a look?

[MaxVerevkin]

It is really annoying that default generics are not allowed in functions. This means that if we want to drop the generic, we will have to have two versions of each connect function - one generic and one for UnixStream. 6 connect functions is too much.

One solution to this is to remove connect_and_collect_globals and async_connect_and_collect_globals, and instead add Connection::collect_initial_globals(&mut self), with an async variant. I don't like it that much because calling it not directly after connect would be invalid. This could be enforced at runtime, though.

I found a solution to this - having a builder type for Connection. Implemented in #17. I like this approach the most so far.

[madushan1000]

I looked over both #16 and #17. looks good so far. I will rebase and test with the virtio client as soon as I get some time. sorry about not being responsive, I've been a bit busy in the past couple of weeks :)

[MaxVerevkin]

Closing in favor of #17