-
Notifications
You must be signed in to change notification settings - Fork 77
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
For consistency use png on macOS #110
base: master
Are you sure you want to change the base?
Conversation
@@ -11,8 +11,8 @@ edition = "2018" | |||
|
|||
[features] | |||
default = ["image-data"] | |||
image-data = ["core-graphics", "image", "winapi/minwindef", "winapi/wingdi", "winapi/winnt"] | |||
wayland-data-control = ["wl-clipboard-rs"] | |||
image-data = ["dep:core-graphics", "dep:image", "winapi/minwindef", "winapi/wingdi", "winapi/winnt"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes the feature flags in the docs a bit more readable as right now it shows image
, core-graphics
and wl-clipboard-rs
as "features" but they are just optional dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, this requires a bump to Rust 1.60 to support:
The dep: syntax is only available starting with Rust 1.60. Previous versions can only use the implicit feature name.
arboard
doesn't currently have an MSRV, and 1.60 is over a year old at this point, so I don't think its a big deal. Its also easy enough to rollback if it causes someone problems. I'm mentioning it just in case though.
Thanks for the PR. I think this change makes sense as long as it doesn't break copying compatibility in any noticeable way. Could I ask you to try some testing by copying images from various apps/websites to see if there's a difference in what |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding my comment about compatibility testing, I got a little bit ahead of myself since I didn't look at the code first. Sorry. Feel free to play around on my other PR though where you could fit PNG reading in though.
@@ -11,8 +11,8 @@ edition = "2018" | |||
|
|||
[features] | |||
default = ["image-data"] | |||
image-data = ["core-graphics", "image", "winapi/minwindef", "winapi/wingdi", "winapi/winnt"] | |||
wayland-data-control = ["wl-clipboard-rs"] | |||
image-data = ["dep:core-graphics", "dep:image", "winapi/minwindef", "winapi/wingdi", "winapi/winnt"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, this requires a bump to Rust 1.60 to support:
The dep: syntax is only available starting with Rust 1.60. Previous versions can only use the implicit feature name.
arboard
doesn't currently have an MSRV, and 1.60 is over a year old at this point, so I don't think its a big deal. Its also easy enough to rollback if it causes someone problems. I'm mentioning it just in case though.
@@ -227,14 +227,14 @@ impl<'clipboard> Get<'clipboard> { | |||
Some(_) | None => return Err(Error::ContentNotAvailable), | |||
}; | |||
|
|||
let tiff: &NSArray<NSObject> = unsafe { msg_send![obj, TIFFRepresentation] }; | |||
let png: &NSArray<NSObject> = unsafe { msg_send![obj, PNGRepresentation] }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe NSImage supports this representation (and is causing a test failure), unlike UIImage which does.
One alternative may be to wait for #106 to merge which switches to a different method of getting images from the clipboard. That way does support PNG like you'd want.
Instead of storing as png on linux and tiff on macos, it feels nicer to just store it as png everywhere.
(haven't tested it yet, but macos docs say this is possible)