Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide system clipboard handling API on Window #2156

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kchibisov
Copy link
Member

Clipboard handling is essential for system window handling and also is a
complicated task to be implemented and handled properly. So abstracting
this code into a windowing library like winit is the most sensible
thing to do.

While it could be offloaded to crates like copypasta[1] the result is
always not good enough[2][3] and it also results in
overengineering[4] (look at its codebase size and compare to what we've
got in winit for Wayland and how much better the handling is done in
winit).

The new API is on the Window and is the following:

pub fn set_clipboard_content<C: AsRef<[u8]> + 'static>(
    &self,
    content:
    C, mimes: HashSet<String>);

pub fn request_clipboard_content(
    &self,
    mimes: HashSet<String>,
    metadata: Option<std::sync::Arc<ClipboardMetadata>>);

And also on WindowExtUnix where added similar methods to get primary
selection content (The one on middle mouse button).

Working with clipboard is async and loading it is in resulted window
event in event::WindowEvent::ClipboardContent.

The use of metadata is due to provide ability to identify requested
load from system's clipboard.

[1] - https://github.com/alacritty/copypasta.
[2] - alacritty/alacritty#3108
[3] - alacritty/alacritty#3601
[4] - https://github.com/Smithay/smithay-clipboard

Clipboard handling is essential for system window handling and also is a
complicated task to be implemented and handled properly. So abstracting
this code into a windowing library like `winit` is the most sensible
thing to do.

While it could be offloaded to crates like copypasta[1] the result is
always not good enough[2][3] and it also results in
overengineering[4] (look at its codebase size and compare to what we've
got in winit for Wayland and how much better the handling is done in
winit).

The new API is on the `Window` and is the following:

```rust
pub fn set_clipboard_content<C: AsRef<[u8]> + 'static>(
    &self,
    content:
    C, mimes: HashSet<String>);

pub fn request_clipboard_content(
    &self,
    mimes: HashSet<String>,
    metadata: Option<std::sync::Arc<ClipboardMetadata>>);
```

And also on `WindowExtUnix` where added similar methods to get primary
selection content (The one on middle mouse button).

Working with clipboard is async and loading it is in resulted window
event in `event::WindowEvent::ClipboardContent`.

The use of metadata is due to provide ability to identify requested
load from system's clipboard.

[1] - https://github.com/alacritty/copypasta.
[2] - alacritty/alacritty#3108
[3] - alacritty/alacritty#3601
[4] - https://github.com/Smithay/smithay-clipboard
@kchibisov kchibisov marked this pull request as draft January 20, 2022 09:32
@kchibisov
Copy link
Member Author

For now the implementation is only done for Wayland however @chrisduerr wanted to help with X11. As for macOS and Windows I'm not sure for now, but I think it should be pretty easy given that it's already written in https://github.com/alacritty/copypasta deps.

@kchibisov
Copy link
Member Author

The example client which is using new API is alacritty. alacritty/alacritty#5793.

Note, build failures are due to fact that there are stabs only for X11/Wayland.

Comment on lines +363 to +373
#[derive(Debug, Clone)]
pub struct ClipboardContent {
/// Clipboard data.
pub data: Vec<u8>,

/// Mime type which was picked to be loaded.
pub mime: String,

/// Metadata passed to `request_clipboard_content`.
pub metadata: Option<std::sync::Arc<ClipboardMetadata>>,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like there's no way to tell which clipboard request we're responding to. Is this supposed to be handled by the ClipboardMetadata in the consumer? I would have expected a serial.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. However we in alacritty need to apply function on certain loads. We could use u64 and decide what to do based on u64 value. But I've decided to go for Any and downcasting.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just feel like it would be useful to fix out-of-order clipboard replies, since they're asynchronous in nature. Either by filtering directly in winit or by making it obvious to the consumer that the replies are not in order?

Copy link
Member

@maroider maroider Jan 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference here would be for request_primary_clipboard_content to return a serial. Using Any + downcasting here doesn't feel like it's "pulling its weight" API-wise.

@ArturKovacs
Copy link
Contributor

ArturKovacs commented Jan 20, 2022

I think this is a great idea! I also like that we want to allow the application to set the contents in any format.

Full disclosure: I've been working on a clipboard crate called arboard but I don't think that winit should depend on an existing clipboard crate. I think winit should implement clipboard handling itself.

Multiple formats

Winit should allow setting a single clipboard object in multiple formats at once, so that other applications can choose the format they can handle. (For example if I copy a segment of formatted text from a webpage, then that text should be put onto the clipboard in both HTML and plain text.) This is how the clipboard works on X11, macOS, and Windows.

Metadata

As far as I understand the ClipboardMetadata is never used by winit, it's just for the application. In that case it seems simpler to just return a token (like a u32) instead for the clipboard request and then provide the same token for the response in the window event. The Send + Sync requirement on the ClipboardMetadata seems a bit too restrictive.

Mime types

The mime type may map well to Wayland and X11, but for example on Windows there are system specified formats for most basic things. For example text (CF_UNICODETEXT), raster image (CF_DIBV5), and HTML (HTML Format). This means that if a winit application puts text onto the clipboard and marks it as text/plain instead of CF_UNICODETEXT, then most applications won't be able to paste it.

Therefore winit must do some sort of conversion on the clipboard data, to provide identical behavior across platforms.

In order to minimize errors and to provide a typesafe way of specifying clipboard data, I think we should use an enum like shown in the following snipet. A more detailed version of this can be found at the arboard source code, here: https://github.com/ArturKovacs/arboard/blob/efe4c0e9dd40c0d53144e5659190568a8e44d158/src/common.rs#L150

#[derive(Debug, Clone)]
pub struct ImageData<'a> {
    pub width: usize,
    pub height: usize,
    pub bytes: Cow<'a, [u8]>,
}

enum ClipboardItem<'a> {
    Text(Cow<'a, str>),
    RawImage(ImageData<'a>),
    TextHtml(Cow<'a, str>),
    ImagePng(Cow<'a, [u8]>),

    // ...
}
impl<'main> ClipboardItem<'main> {
    /// A string identifier for the type of the item.
    ///
    /// For most items this is the MIME type, but there are exceptions:
    /// - The [`Text`](Self::Text) variant, for which this returns
    ///   "winit-text".
    /// - The [`RawImage`](Self::RawImage) variant, for which this returns
    ///   "winit-image".
    pub fn media_type(&self) -> &'static str {
        match self {
            ClipboardItem::Text(_) => "winit-text",
            ClipboardItem::RawImage(_) => "winit-image",
            ClipboardItem::TextHtml(_) => "text/html",
            ClipboardItem::ImagePng(_) => "image/png",
        }
    }
}

EDIT: Renamed ClipboardData to ClipboardItem

@kchibisov
Copy link
Member Author

@ArturKovacs Looking at arboard I think most things could be taken from it when it comes to text and mime handling. I'd play with it.

As for serving multiple mimes, well it does supported right now, however I think set_clipboard_content should accept a callback so you can adjust your data based on what end user asked for.

Similar situation comes when dealing with loading clipboard since other clients could serve image and text at the same time, so users need a way to pick what mime type they want. Maybe vector of priorities? Like first element is the most preferred one, and so on?

@ArturKovacs
Copy link
Contributor

Looking at arboard I think most things could be taken from it when it comes to text and mime handling. I'd play with it.

Feel free to do so.

I think set_clipboard_content should accept a callback so you can adjust your data based on what end user asked for.

Yeah this also seems to be supported on

  • Windows under the name delayed rendering
  • macOS: NSPasteboard / writeObjects: -> NSPasteboardItem / setDataProvider:forTypes:. Winit would have to declare a class that implements NSPasteboardItemDataProvider and that can store the reference to the callback closure, and call it when the data is needed.

@ArturKovacs
Copy link
Contributor

Similar situation comes when dealing with loading clipboard since other clients could serve image and text at the same time

The way this works on Windows, is that the first format in the list of available formats is the one that best represents the data. So as you said, it's a priority list. So the application would first ask for the list of available formats, and then request the contents for the first format that the application supports.

@madsmtm
Copy link
Member

madsmtm commented Jan 23, 2022

Linking to some of the places this was discussed in the past:

Clipboard handling has been explicitly left out of our stated scope, and while I could probably persuaded to change that, I think it warrants a much bigger discussion (incl. involving more parties) before we stray from that.

To be fair, I haven't given clipboards nearly as much thought as you have, but it sounds like the problems mainly stem from not being given enough access to the event loop? So maybe there's a solution to this that only requires exposing some way to hook into that (see #2120)?

@madsmtm
Copy link
Member

madsmtm commented Jan 23, 2022

@maroider suggested creating a winit-extras crate (name up for bikeshedding) at some point, I wouldn't have a problem with putting it there (or having it depend on copypasta while doing some fixes where the platforms require it, if possible).

@chrisduerr
Copy link
Contributor

@maroider suggested creating a winit-extras crate (name up for bikeshedding) at some point, I wouldn't have a problem with putting it there (or having it depend on copypasta while doing some fixes where the platforms require it, if possible).

Copypasta's clipboard handling is unusable by clients. If that's your great solution to clipboard handling in winit, it's broken lol.

@madsmtm
Copy link
Member

madsmtm commented Jan 23, 2022

Copypasta's clipboard handling is unusable by clients

I was thinking, use copypasta (or arboard, or window_clipboard, or whatever library that is most used + maintained) for the platforms/window managers/whatever that they properly support, and for the ones that require winit integration we would do said integration in winit-extras

@madsmtm madsmtm added C - needs discussion Direction must be ironed out S - api Design and usability S - enhancement Wouldn't this be the coolest? labels Jan 23, 2022
@ArturKovacs
Copy link
Contributor

ArturKovacs commented Jan 24, 2022

@madsmtm makes a good point. I think that implementing lazy-clipboard-data-provision will be possible outside of winit after #2120

Edit: little correction: It's already possible today, but it can't be done through the winit window. Clipboard crates currently have to make a separate, invisible window just for clipboard handling if they want to do such things.

@kchibisov
Copy link
Member Author

The problem with clipboard is that it's essential part of windowing and putting it outside of the winit just doesn't make any sense to me. For example all libraries I'm aware of have clipboard API. So if someone migrating from glfw, sdl, etc it should be easy for them to move into winit.

One more point is that drag and drop and clipboard are the same if you think about them. Same concepts apply to both of them. Drag and drop is in the scope of winit and is implemented for certain platforms, except Wayland. On Wayland when it comes to drag and drop it's using the same API as clipboard. After adding clipboard into winit adding drag and drop for Wayland is just a few lines of code. However without clipboard you're forcing the end user to have one more global for data device manager and winit and so called clipboard crate will have the exact same code but just duplicated and less efficient.

As for the external source feed for events. It'll be nice, however for Wayland I'd basically need winit's epoll from calloop and wayland-client types to be reexported, since don't it from C types won't really fly, since I need to attach my objects to winit's event queue. Why wayland-client API isn't stable I'm not sure it's a good idea. Also, I should be able to control winit's Env to ask for globals I'd need instead of having multiple envs around. (The second part isn't really a problem).

The same applies for X11 clipboard, but it's a bit simpler, however one problem with X11 clipboard in its current impl in crates out side of winit is that it starts to get really slow over time since if it doesn't run a separate thread and process events it gets it could take a while for it to start pasting. Having one more thread isn't an option, since it's just silly.

macOS and Windows I guess are simpler here.

Also with putting something outside of winit scope you'll likely end up giving up on its maintenance eventually or slow up the updates, since updating winit won't require updating clipboard, and it'll require updating clipboard after winit's releases, so users of so called clipboard-data-provider won't be able to update winit until you update clipboard provider.

My motivation behind the clipboard API in winit is that it's really annoying to maintain implementation out side of winit and ensure that it works. I've been working/using clipboard crates for a while and they all don't really work. Not even saying that Wayland clipboard is just different and you can't even use it without window around. And you can't even create invisible window, since it's a bug that it works on some compositors. Compositor shouldn't pass focus to invisible windows in a first place.

I think there are also folks who would love to have clipboard API right in winit, it's not just what alacritty wants in the end.

@ArturKovacs
Copy link
Contributor

Drag and drop is in the scope of winit and is implemented for certain platforms, except Wayland. On Wayland when it comes to drag and drop it's using the same API as clipboard.

This alone convinced me that clipboard handling should be inside of winit.

@maroider
Copy link
Member

My main concern is the viability of mapping to mime-type strings on platforms which don't use mime-types for tagging the contents of the clipboard (maybe this is a Windows-only problem, idk). Going by @ArturKovacs's previous comment, this issue appears to be solvable?

@kchibisov
Copy link
Member Author

My main concern is the viability of mapping to mime-type strings on platforms which don't use mime-types for tagging the contents of the clipboard (maybe this is a Windows-only problem, idk). Going by @ArturKovacs's previous comment, this issue appears to be solvable?

Yeah, I've just use strings to have something working on at least one platform. I'd convert it to some common winit type and use what @ArturKovacs suggested. I just pushed this PR to gather opinions and get aware of how other platforms behave, since I think you were in situation writing clipboard handling in your toolkit/app already.

@madsmtm
Copy link
Member

madsmtm commented Jan 27, 2022

You raise some fair points, and in any case, I don't have any major objections to this being in winit, I mostly just want to ensure that we don't blatantly override past consensus before properly discussing the tradeoffs, and ensure we get input from stakeholders.

At the very least, I'd like to hear from @Osspial (I know you don't have much time for winit lately, so if you can't really answer this, that's fine, but you've clearly thought about it in the past: Do you think things have changed enough / is @kchibisov's argument above strong enough that you would agree we should add this?) and maybe @vberger?

@elinorbgr
Copy link
Contributor

and maybe @vberger?

For what it's worth, I've always been in favor of having clipboard support inside winit. As pointed by @kchibisov, this makes the implementation dramatically simpler to integrate it directly into the internal event loop on Wayland. I cannot say anything about other platforms though.

@trimental
Copy link
Contributor

Whilst Wayland introduces this clipboard problem for the rest of the ecosystem. I personally believe Wayland handles the clipboard in a more strict and 'correct' way, and other platforms are simply more relaxed with their coupling of windows and their clipboard events.

Since winit users are already choosing a convenient cross-platform api at the cost of some platform specific functionality and control, I would expect the vast majority of winit users to want a clipboard api that just works.

@Ralith
Copy link
Contributor

Ralith commented Jan 28, 2022

Including these seems reasonable to me.

More narrowly, the mimes: HashSet<String> argument feels like a strange choice of type, particularly as the setter immediately proceeds to convert it into a Vec<String> instead.

  • Should it be borrowed instead? It's unclear why ownership is required.
  • Should it be a simpler type like &[&str]? This would be more convenient (and cheaper) to supply in the typical case of a small hardcoded set of options.

@LaylBongers
Copy link

As a personal note on the inclusion of clipboard events, I've always treated winit in my projects more as a 'platform event loop' library than a windowing library specifically. It just so happens that this is generally considered the same thing in these kinds of libraries, but myself I treat them as separate concepts. To me windowing is more a sub-category of the platform event loop pipeline.

@madsmtm
Copy link
Member

madsmtm commented Feb 4, 2022

Alright, a week has passed an no objections raised, and it seems like all current maintainers are on board with this, so I'll consider that diligent enough, thanks for your input everyone!

I apologize if I've discouraged you @kchibisov, would you still be willing to work on this?

@kchibisov
Copy link
Member Author

@madsmtm I want to work on this, however my time for winit is a bit limited, but finishing clipboard handling is on my todo list and I'll likely go back to it after we sort out IME. Though, if anyone feel like adding implementation for macOS/Windows/X11 feel free.

Though, I think the API should get some shape before adding other platforms.


/// Create a `File` from `RawFd` marking it with `O_NONBLOCK`.
unsafe fn raw_fd_into_non_blocking_file(raw_fd: RawFd) -> Result<File> {
let flags = libc::fcntl(raw_fd, libc::F_GETFD);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised this works!

Suggested change
let flags = libc::fcntl(raw_fd, libc::F_GETFD);
let flags = libc::fcntl(raw_fd, libc::F_GETFL);

@notgull
Copy link
Member

notgull commented Feb 4, 2023

The current API uses an impl AsRef<[u8]> as the clipboard input and a Vec<u8> as the clipboard output. While this works fine, I was thinking that an ideal clipboard API would use impl Write to write to the clipboard and impl Read to read from the clipboard. This way, if you want to upload a file to the clipboard, it can be done without buffering. It looks like the Wayland implementation uses read/write pipes under the hood, so I think that a Read/Write-based implementation would fit better anyhow.

I guess the main issue with that would be "what if the reader blocks the event loop", which I don't really have a good answer for, except that buffering would likely have the same issues.

@WhyNotHugo
Copy link

As for serving multiple mimes, well it does supported right now, however I think set_clipboard_content should accept a callback so you can adjust your data based on what end user asked for.

I strongly agree with this approach. This makes a big difference when data is large and/or requires expensive computation to convert to another mime type.

For example: if you copy a huge image, and are offering it in image/png, image/jpeg and image/x-dcraw. If the data needs to be provided ahead of time, the image needs to be encoded into all these formats, but then just one (or maybe none) of them may be used, not to mention the added memory usage.

@airstrike
Copy link

airstrike commented Jul 11, 2024

Since this was removed from milestone 0.30.0, can it now be added to 0.30.4 or possibly 0.31? This would be an amazing feature to have in winit since rich clipboards are really table stakes for more complex GUI apps

@kchibisov
Copy link
Member Author

It's planned for 0.31+, it can not be added to 0.30.x series since the current winit API is not ergonomic for clipboards/dnds to be heavy used and text only keyboard already exists in the source of https://github.com/alacritty/copypasta and similar tools.

The thing is that more complex mime types should generally handled without copying them a lot or parsing/reparsing and also there could be multiple mimes present at the same time, so users should have a way to ergonomically pick or even access the window state to alter alter/trigger rendering.

Just stay tunned I guess, since I need this myself in alacritty and I don't forget about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - needs discussion Direction must be ironed out S - api Design and usability S - enhancement Wouldn't this be the coolest?
Development

Successfully merging this pull request may close these issues.