search

aweinstock314 / rust-clipboard

System Clipboard interfacing library in Rust
Apache License 2.0
365 stars 75 forks source link

feat(*): get non-string contents from clipboard for OSX #60

Open muja opened 5 years ago

muja commented 5 years ago

Hi, this addresses in part issue #31, and #46.

Some things I feel may be done differently:

  1. I'm currently using a non-exhaustive enum, arguably this may not be the "best" cross-platform way. It may make sense to work with MIME types / Content-Types, however this offloads some work to the caller. Also, if the user copies a file from Finder (OS X), it will be a NSURL instead of a String, which cannot be captured as a mime type.
  2. I'm currently copying the entire &[u8] from the objc memory (I added a comment) to an owned Vec which is very inefficient (small clipboards I experimented with when screen capping windows were >18MB). Not sure what's the best approach here

This is also just for OSX, happy for your feedback. For windows, it should be trivial as far as I can tell, since the clipboard-win crate exposes an API for this already

aweinstock314 commented 5 years ago

This looks pretty good. As is though, it breaks the builds on Windows and Linux. Would you mind adding a commit to this PR that adds implementations of get_binary_contents in {windows,x11}_clipboard.rs? It doesn't need to be anything fancy, and can probably default to Some(ClipboardContent::Utf8(self.get_contents()?)).

muja commented 5 years ago

Yeah, that could be a way to implement them.. I've been thinking about a different way, though, I'm thinking the caller side should look something like this, so that they can give a precedence of possible types:

clipboard.get(&[formats::IMAGE, formats::HTML, formats::UTF8])

One problem here is that windows doesn't seem to support PNG while OS X does. So we could separate the formats::IMAGE into formats::TIFF and formats::PNG, again leaving it up to the caller to prioritize, and risking that the user only chooses PNG because it's "better" while accidentally un-supporting Windows clipboards. Here, I think it might be better to not provide formats::PNG, and instead, provide formats::IMAGE as well as formats::TIFF (which is a "subset" of formats::IMAGE), and the return value can be again the enum that is already in this PR, so the caller in the case of formats::IMAGE needs to check for both PNG(Vec<u8>) and TIFF(Vec<u8>), while for formats::TIFF they only need to check for TIFF(Vec<u8>).

Hope that makes it clear, I will amend this PR next week.

Btw, do you have any idea how it is in Linux? PNG/TIFF support?

aweinstock314 commented 5 years ago

Having formats::{IMAGE, PNG, TIFF} would work as a first draft. I feel like there should be a more elegant way to encode that PNG and TIFF are both subtypes of IMAGE in terms of trait hierarchies, which should eventually be designed.