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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

muja
Copy link

@muja muja commented Nov 28, 2018

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

let cls: Id<Class> = unsafe { Id::from_ptr(class("NSURL")) };
unsafe { transmute(cls) }
};
let classes = vec![url_class, image_class, string_class];
Copy link
Author

Choose a reason for hiding this comment

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

Note that the more "specific" class needs to come first, as readObjectsForClasses will try to match in order

@aweinstock314
Copy link
Owner

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
Copy link
Author

muja commented Dec 1, 2018

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants