Small API change request : let make ClientBuild.proxy() accept a null value.

[bmarty]

It will simplify the caller life.

I believe the change has to be done here : https://github.com/matrix-org/matrix-rust-sdk/blob/main/bindings/matrix-sdk-ffi/src/client_builder.rs#L178, by changing String to Option<String>.

Thanks!

[bmarty]

In the same vein disable_ssl_verification() should probably take a Boolean parameter, and there are maybe other APIs to improve.

[poljar]

How would this make the life of the caller simpler? This is a builder pattern, if you don't want to set the proxy, don't call the method?

What is the point of a set_proxy() method if it accepts an Option, it stops being a set_proxy() method.

[bmarty]

If it accepts null (proxyProvider.provides() returns nullable String):

        val client = ClientBuilder()
            .basePath(baseDirectory.absolutePath)
            .homeserverUrl(sessionData.homeserverUrl)
            .username(sessionData.userId)
            .passphrase(sessionData.passphrase)
            .userAgent(userAgentProvider.provide())
            .let {
                val proxy = proxyProvider.provides()
                if (proxy != null) {
                    it.proxy(proxy)
                } else {
                    it
                }
            }
            ...

and if the method accepts null:

        val client = ClientBuilder()
            .basePath(baseDirectory.absolutePath)
            .homeserverUrl(sessionData.homeserverUrl)
            .username(sessionData.userId)
            .passphrase(sessionData.passphrase)
            .userAgent(userAgentProvider.provide())
            .proxy(proxyProvider.provides())
            ...

[bnjbvr]

It's a bit useless to have a builder pattern if we're passing all the parameters all the time. Instead, we could make it so, for the FFI layer, that all the parameters be included in a large struct, and only that one struct would be passed to the constructor.

// pseudo code
let client = new Client({
  basePath: ...,
  homeserverUrl: ...,
  proxy: ...,
});

[bmarty]

Other clients of the Rust SDK may not need to provide all the params. If it is too problematic to change this API, let's close the issue as not planned, I will not be offended :).

[bnjbvr]

I think it'd be fine in the FFI layer, in replacement of, or in addition to, the existing ClientBuilder in bindings. We're not removing the ClientBuilder in the SDK crate any time soon :-)

[bmarty]

It's not really a builder pattern, since each call to a builder method return a new builder instance at least from the application point of view. A builder should return self. See https://github.com/element-hq/element-x-android/pull/3068#discussion_r1656737483 for more details.

[bnjbvr]

So I've looked into this a bit:

• UniFFI doesn't allow to return &Self or take a Self argument by ownership, so we can't use a real Rust builder pattern here.
• We can emulate a builder pattern by having each method take and return Arc<Self>, but this means the data stored in e.g. ClientBuilder is now behind an Arc<Mutex<_>>, meaning that each clone of the ClientBuilder is shallow and refers to the same underlying ClientBuilder data. IMO it is more footgun-y than the status quo.

That being said, if most of the methods of a builder are called upon construction, we might as well have a ClientBuilderParameters public record, and a Client::new(params: ClientBuilderParameters) method or something like this.

[poljar]

That being said, if most of the methods of a builder are called upon construction, we might as well have a ClientBuilderParameters public record, and a Client::new(params: ClientBuilderParameters) method or something like this.

EX isn't the only one using the bindings, at least complement is using them as well. I prefer the API to be kept mostly the same as we have in the non-bindings case.