RFC: API redesign

[bugaevc]

High-level ideas:

• Now that async/.await is stable, wcrs should be natively async
• Event loop integration: it should not fork or spawn threads behind your back
• Less allocations, less RefCells

Watching the socket requires a reactor; there should be implementations for Tokio and async-std, togglable with a cfg flag.

I imagine the high-level API could look somewhat like this:

struct ConnectResult {
    connection: Connection,
    // Spawn this task on your executor.
    task: impl Future<Output = ()>,
}

impl Connection {
    /// Establish a new connection to the compositor.
    ///
    /// Returns the connection and a task to spawn, the task
    /// takes care of watching and dispatching Wayland events
    /// on the connection's internal socket.
    pub async fn connect() -> Result<ConnectResult>;

    /// Wrap an existing Wayland connection, which should be
    /// dispatched by someone else. Use this if you already use
    /// Wayland in your program for something else.
    pub async fn with_display(display: &wayland_client::Display) -> Result<Connection>;

    // Convenience wrapper over `.set_source(Source::simple())`
    pub fn set(&mut self, content: impl AsyncRead, mime_type: MimeType);
    pub fn set_source(&mut self, source: Source);

    // Convenience wrapper over `current_offer().read()`
    pub fn get(&mut self, mime_type: MimeType) -> impl AsyncRead;

    pub fn current_offer(&self) -> Option<Offer>;
    pub fn offers(&self) -> impl Stream<Item = Offer>;
}

impl Source {
    pub fn simple(content: impl AsyncRead, mime_type: MimeType) -> Source;
    pub fn new(content_options: &[(MimeType, impl AsyncWrite)]) -> Source;
}

impl Offer {
    pub fn available_types(&self) -> impl Iterator<Item = MimeType>;
    pub fn read(&mut self, mime_type: MimeType) -> impl AsyncRead;
}

There would be probably some "options" args here and there to set things like which clipboard and seats to use.

Naming and details are of course subject to bikeshedding.

[ghost]

Note that upgrading wayland-rs from 0.22 to 0.24 will already require some (heavy?) refactoring, that should for example reduce the RefCell usage already. It should also make implementing the event loop easier.

[YaLTeR]

wcrs should be natively async

There should still be a blocking sync API though for use-cases which are simple enough to not need the full async stack (like wl-copy, etc.).

it should not fork or spawn threads behind your back

I think it could still be worthwhile to still provide the option to spawn a thread for easy I-don't-care foreground like the current API (not fork, that one should be replaced asap).

pub async fn connect() -> Result<ConnectResult>;

An async function returning a separate Future feels weird. I imagine this should not be async, but rather the returned Future should "contain" all the connection set-up? Alternatively, an async function that returns a Result<Connection> with async methods? Actually, I can't say I understand what should be happening in this function and in the returned Future yet. One thing to consider is that wayland-rs is not async (at least thus far).

pub fn set(&mut self, content: impl AsyncRead, mime_type: MimeType);

Doesn't AsyncRead only allow reading the contents once? Whereas we'd want to read the bytes any number of times.

pub fn get(&mut self, mime_type: MimeType) -> impl AsyncRead;

Should return an Option<>?

pub fn new(content_options: &[(MimeType, impl AsyncWrite)]) -> Source;

This AsyncWrite is probably wrong?

[bugaevc]

There should still be a blocking sync API though for use-cases which are simple enough to not need the full async stack (like wl-copy, etc.).

That's what thread::block_on() is for, but maybe it'd make sense to provide yet another layer of convenience APIs that call that for you.

I think it could still be worthwhile to still provide the option to spawn a thread for easy I-don't-care foreground like the current API (not fork, that one should be replaced asap).

Again, they can do something like

thread::spawn(||
    thread::block_on(task)
);

explicitly, and then it's their fault that there's now a separate thread 🙃, and it's up to them to remember not to exit the process while it's running, etc.

An async function returning a separate Future feels weird.

It kinda does, yeah, but that is what any async fn that returns a type that has its own async methods does if you think about it. In this case the caller should .await the outer future and spawn the inner one (it never returns unless an error happens).

I imagine this should not be async, but rather the returned Future should "contain" all the connection set-up?

Well I was thinking the async-ness in connect() comes from having to roundtrip to the server fetching globals from the registry and returning an error — from connect() — if there's no wlr-data-control or something.

One thing to consider is that wayland-rs is not async (at least thus far).

It doesn't have to be. You register the socket with a reactor; when there's new data on it you ask wayland-rs to decode and dispatch it; when handling events you wake() the futures that wait for them. Actually wayland-rs not being async becomes of the major reasons to use wcrs over using it directly.

Doesn't AsyncRead only allow reading the contents once? Whereas we'd want to read the bytes any number of times.

Right. impl Fn() -> impl AsyncRead or something like that then. Maybe a new trait with default impls,

trait Data {
    fn read(&self) -> impl AsyncRead;
}

impl Data for &[u8] {
    fn read(&self) -> impl AsyncRead { *self }
}

impl<F, R> Data for F where F: Fn() -> R, R: AsyncRead {
    fn read(&self) -> impl AsyncRead { self() }
}

Should return an Option<>?

Probably, yeah.

This AsyncWrite is probably wrong?

Yeah, it should be AsyncRead (or Data) too.

[YaLTeR]

thread::block_on()

I like the idea, but that still requires bringing in an entire async crate to use wcrs which is undesirable if you don't use async elsewhere.

[bugaevc]

thread::block_on() itself should be in std; but you indeed need a reactor.

[YaLTeR]

Another thing I want to consider is if it's possible to export a simple C API for copying and pasting so that wcrs could be used as a library from other languages. I think it shouldn't be too hard with the current implementation.

[bugaevc]

Watching the socket requires a reactor; there should be implementations for Tokio and async-std, togglable with a cfg flag.

Actually if we already "know" the user is using tokio or async-std, maybe we can just spawn tasks there instead of making the user do it? But then we lose explicitness about threads and blocking.

[YaLTeR]

Oh yeah, yet another point to consider: wl-copy needs to be able to fork-and-not-exec to support the foreground mode (does it? Can we achieve the same behavior by doing a fork-exec into wl-copy without the foreground flag set? How will this report errors to the user?), and I'm not sure if this is safe when using async runtimes (since they might spawn threads which would make what we do past fork invalid).

[bugaevc]

Yeah, it should be AsyncRead (or Data) too.

Hmm, maybe like this then:

trait ClipboardContents {
    fn types(&self) -> impl Iterator<Item = MimeType>;
    fn data(&self, type: MimeType) -> impl AsyncRead;
}

impl ClipboardContents for Offer { ... }

impl Connection {
    pub fn set(&mut self, contents: impl ClipboardContents);
}

I'm not sure if this is safe when using async runtimes (since they might spawn threads which would make what we do past fork invalid)

You can ask them to use only one thread.

[YaLTeR]

You can ask them to use only one thread.

That reads to me like it's still gonna spawn at least one thread on the pool.

[bugaevc]

My bad, it's this method that tells tokio to only use the current thread. And

If the rt-core feature is enabled and rt-threaded is not, Runtime::new will return a basic scheduler runtime by default.

[YaLTeR]

Let me write down some goals of a new API.

1. Many toolkits offer multiple formats when copying e.g. images: you get image/png, image/jpeg, and so on and so forth. It should be possible to do the same with wl-clipboard-rs without converting the image to all formats upfront.
2. With certain formats, e.g. text, it's customary to offer multiple formats for the same data: text/plain, TEXT, etc. It should be possible to offer multiple formats for the same data without copying the data multiple times.
3. It should be possible to "watch" for new selections properly (not by trying to paste on a timer).
4. There should remain a simple way of using the API if no advanced functionality is needed. Ideally simple tasks (those handled by the current API) should not be any more complicated than they currently are.
5. Repeated copying and pasting should be possible without re-connecting to the Wayland server every time.
6. If possible, when copying, there should not be any extra copies of the input data. Before the latest changes there used to be one "copy" (to a temporary file), after the latest changes there are two copies (necessary for keeping within the same API while removing forking), but I believe it's possible to do better than that.
7. The possibility of exporting a C API should be investigated. I believe it's not too hard to export a C API with the current design.

Introducing an async API, while probably not necessary for achieving the above goals, is nice for the following reasons.

• In an async application it might be possible to "copy in the background" without actually spawning any additional threads (by introducing a task which polls the Wayland socket fd and reads events/writes requests in a non-blocking fashion).
• The "watching for new selections" concept seems to present itself very nicely as a non-blocking async Stream (once again, allowing for handling this without spawning additional threads).

@bugaevc @yory8 if you have any other goals you want to see in wl-clipboard-rs, please comment, and I'll add them to the list. Then we can keep them in mind while coming up with an API and bikeshedding.

[ghost]

Seems all lovely!

Just a note about point 3: currently the naive timer-based approach is actually unusable with this library, as it crashes the whole wayland session after a little while.

Also, I recently became aware of the SAVE_TARGETS mechanism that is supposed to be used by freedesktop-compliant clipboard managers to persist the full offer (all mimetypes) after the application exits. I don't get how it's supposed to work, though, nor if it belongs to this library or its users'.