X11: use existing connection and window

[dhardy]

It turns out we can use the window reference on X11 too to avoid creating a fake window for reading a clipboard. After integrating into KAS for testing, this part appears to work; this may however only be because of the blocking design and event processing normally happening in the expected order...

I copied the run logic from https://github.com/quininer/x11-clipboard but using this same connection and tested in KAS, and of course the run thread eats most of the main thread's events. There's a reason x11-clipboard uses two new connections, each with a fake window (though I don't know if the run part actually requires a window or just the connection).

It seems that the correct way to make a clipboard work under X11 (and I think Wayland) is to handle these events within the application's main event loop. Making this work correctly via a standalone lib requires at least one new event loop, perhaps with a channel to send read results from the background event-loop thread to the read function.

Obviously, this implies that the "correct" solution is to integrate clipboard functionality into winit, either directly or as a plug-in attached to a sub-set of events, and that a cross-platform API must be asynchronous (and I'm not sure that futures cut it).

(All this for a broken clipboard API — it obviously requires a hack somewhere to let selections persist beyond application closure.)

CC @vberger (Wayland, https://github.com/rust-windowing/winit/issues/872): do you have thoughts on what the clipboard API should look like? Also see #2.

[dhardy]

Perhaps we shouldn't let X11/Wayland get in the way of good API design: APIs should be modular, with minimal inter-dependencies. In which case, we should be able to hack a solution here (though it probably means using the existing window connection to create a second connection to the same display/seat, then modifying the run thread to handle both reads and writes).

[elinorbgr]

CC @vberger (Wayland, rust-windowing/winit#872): do you have thoughts on what the clipboard API should look like?

I don't have a lot of thoughts about that currently, as I don't have a lot of input about what apps expect from a good clipboard API. However,

I copied the run logic from https://github.com/quininer/x11-clipboard but using this same connection and tested in KAS, and of course the run thread eats most of the main thread's events. [ ... ] It seems that the correct way to make a clipboard work under X11 (and I think Wayland) is to handle these events within the application's main event loop.

It's possibly the case for X11, but that is a non-issue for Wayland. The Wayland client libraries has multiple event queues (while AFAIK X11 has only one). This allows multiple libraries to be able to share a single Wayland connection without eating each other's events. And this is basically what smithay-clipboard already does. While it still spawns its own thread, but I have in my plans to make it leaner.

Overall though, it seems to me that there is a common question: the clipboard handling library needs to be able to regularly check if it has new events to process. I see roughly 3 ways to do that:

• spawn a thread and have it block/loop/epoll/... to process the clipboard events
  • more or less what is currently done in practice AFAIK ?
• integrate the clipboard sockets into the main app event loop
  • this requires access to the main app event loop, winit currently does not expose it
  • this is not really applicable for Wayland, as the clipboard managing library must use the same Wayland socket as the main app, and this socket is already managed by the main loop
• have the main app regularly call some function of the clipboard managing library, which checks if new messages are pending and processes them if any
  • this IMO would be the "correct" way to do that on Wayland, inducing the minimal overhead
  • cannot tell how relevant it could be for X11
  • for example when used in conjunction with winit, this function could be called whenever the MainEventsCleared event is received.

[dhardy]

Thanks @vberger.

have the main app regularly call some function of the clipboard managing library, which checks if new messages are pending and processes them if any

This option sounds attractive, allowing a modular design and simple API without a background thread. (Hopefully it doesn't result in lots of "cross platform" apps with broken clipboards on Linux.)

A related question is how this should work with multi-window apps: window_clipboard::Clipboard should have one instance per window and is created from a RawWindowHandle. Ideally we don't want a separate event loop for each window; will everything work if we just instantiate a lazy_static with the first window's connection? And is that something to handle within smithay-clipboard or in an external wrapper?

[dhardy]

Finally found a good write-up on how the X11 clipboard works: https://www.uninformativ.de/blog/postings/2017-04-02/0/POSTING-en.html

It sounds like using an unmapped window to manage the clipboard is common practice.

[elinorbgr]

A related question is how this should work with multi-window apps:

I don't know how it is for other platforms, but on Wayland you don't really need the window-specific part ofRawWindowHandle at all, so a single initialization for the whole app would work fine.

[dhardy]

No, but from what I understand one does need the connection part. I believe the same applies to X11, so this seems to be fine.

[elinorbgr]

I think my question is rather regarding this:

window_clipboard::Clipboard should have one instance per window

Why should there be one instance per window, rather than a single one for the whole app?

[dhardy]

Well, maybe there shouldn't. But the current API suggests that a multi-window app should use the current window's handle. (This may just be a documentation issue.)

[dhardy]

Anecdote: I accidentally discovered that a couple of KDE apps and Firefox block for several seconds when incorrectly setting the SELECTION. So apparently blocking APIs are common and not an issue.

[dhardy]

I implemented enough to make this API work under my testing, so this might be ready for merging (or not, according to by CI).

This currently uses the background thread approach. I'm still not certain whether which should switch to the "update" method, and if so that should be another PR.

[dhardy]

@hecrj the Ubuntu runner is failing with = note: /usr/bin/ld: cannot find -lX11-xcb. I've not used GitHub workflows so not sure how to add a dependency here (it works on my local Fedora machine).

[hecrj]

Wow, there's a lot to go through here!

I see you have implemented something similar to what the x11-clipboard already does. I wanted to try something different in this crate.

The main issue I see here is that we are spawning a thread sneakily in Clipboard::new. I believe this server idea should be an explicit concept in the public API. It should be something the user needs to deal with and keep alive (even when not dealing with X11). This is why I mentioned async in #2.

Ideally, I'd like to find a common denominator that unifies clipboard access to all platforms—even if we have to complicate the API unnecessarily for some of them—while keeping things explicit. I believe this is the main challenge we face here.

We should probably think about different designs before writing any code.

[dhardy]

@hecrj sorry to overwhelm you with a lot of discussion, but it seems you missed some of what happened. @vberger suggested adding an update or poll function to the clipboard which the user polls in MainEventsCleared; this would eliminate the need for a separate background thread and allow the two connections used here to be merged, but as I said, I think this should be a separate PR.

In the mean time, this PR adds clip-board write support (though I only tested on X11), and I don't think the addition of the background run thread is too bad (it uses connection.wait_for_event() for scheduling, and the complexity is all there for good reason).

So, if you fix the Ubuntu build dependency and merge this, I can go ahead and make a new PR adding Clipboard::update and some doc changes. Though I'm not completely convinced this is the right design: for non-Linux users the update function will do nothing and other functionality will work regardless, thus leading to potential portability issues. The nice thing about the current code is that it just works. Async alone can't solve it.

[dhardy]

Note: on my system, libX11-xcb is provided by libX11-xcb-1.6.9-2.fc31.i686 which has many reverse-dependencies, so I'm surprised it's not included by the Ubuntu test instance. This dependency may be an issue in some cases however. If we don't want it, we would lose the ability to discover the X11 display name from an existing Xlib connection. In most cases this will resolve the same display name via environment variable anyway, but there may be some corner cases.

[hecrj]

adding an update or poll function to the clipboard which the user polls in MainEventsCleared

How would this work when you use ControlFlow::Wait and another application triggers a clipboard read request? How will the main loop respond?

don't think the addition of the background run thread is too bad

I don't think spawning a thread in a constructor is an acceptable side-effect. conrod, for instance, creates a new x11-clipboard on every paste event, probably because they are unaware of the cost.

The nice thing about the current code is that it just works.

I understand, but I really believe code is the easy part. I don't see any value in copying exactly what other crates (like copypasta) are already doing. I'd like to explore type-safe, portable, hack-free, and explicit approaches here.

Async alone can't solve it.

I believe it could be part of the design we are looking for. We could have some kind of WindowManager type with an async connect method accepting a raw-window-handle. Then, the only way to obtain a Clipboard instance would be from a WindowManager itself.

[dhardy]

ControlFlow::Wait

Good point.

unaware of the cost

IMO this is a documentation issue, as limited as documentation is. And presumably the only reason conrod gets away with constructing a new clipboard on each paste event is because the cost is actually not that important, relative to what an interactive app does anyway.

Before we go any further here, do we have any actual metrics on the cost of an extra background thread and display server connection?

WindowManager

So, you're talking about another type in this lib just to make construction more explicit? Or you're talking about making winit extensible by having it accept a future (or really just a closure) which it polls periodically?

[hecrj]

IMO this is a documentation issue

Documentation is great, but it should not be used to patch or cover bad API design. The main issue here is that a constructor performing side-effects is not intuitive and not explicit in the API boundaries.

Before we go any further here, do we have any actual metrics on the cost of an extra background thread and display server connection?

The cost isn't really what worries me. It's the design.

So, you're talking about another type in this lib just to make construction more explicit?

Yes. I believe the first thing we need to do is find out which concepts need to exist in our cross-platform public API and how they relate to each other.

[dhardy]

concepts

IMO we should aim for a simple API:

• read String, blocking
• write String, blocking or offloading

along with a more comprehensive API:

• discover available formats of current entry
• request current entry in some format, async
• provide an entry in 1 or multiple formats, async

---

Blocking reads aren't really a problem.

Async reads could happen via polling a future or via somehow injecting messages into the event loop (probably this would require winit integration).

Writes could potentially happen in a few ways:

• via a future the app is expected to poll frequently until completion (immediately on most platforms, once another app overwrites to the clipboard on X11 and Wayland); this relies on the app to handle this correctly and not e.g. drop the future or sleep too long
• via a background thread, as here
• integrated into winit

---

@hecrj keep's talking about needing to explore the options, but I did that (this PR was mostly a side-effect of that work). IMO we should not rely on the app too heavily, so this leaves us two options:

1. Use a background thread. We can rename Clipboard::new to Clipboard::spawn_runner if you like.
2. Integrate into winit. This IMO would be the neatest approach, but falls foul of the "divide-and-conquer" idiom (and it's clear winit is already struggling for developers).

[kchibisov]

Integrate into winit. This IMO would be the neatest approach, but falls foul of the "divide-and-conquer" idiom (and it's clear winit is already struggling for developers)

If you can write up a proposal to winit to include clipboard, I'm all for it, however we may keep both clipboard and dnd under a feature flag. implementing them in async way isn't really a problem, I'd assume + there's a glfw from where we can port a clipboard stuff.