-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Added native Windows clipboard support #373
Conversation
wip better conditional wip wip wip wip make conditional
@@ -77,7 +77,11 @@ pub fn get_clipboard_provider() -> Box<dyn ClipboardProvider> { | |||
copy => "tmux", "load-buffer", "-"; | |||
} | |||
} else { | |||
Box::new(provider::NopProvider) | |||
#[cfg(target_os = "windows")] | |||
return Box::new(provider::WindowsProvider); |
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.
Need the return
s here because it gives you a warning otherwise. (Actually, maybe not now since it's conditional. However, I don't feel like it's that big of a deal to warrant a change. It might make it clearer, even.)
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 think can just remove the return or you could use if cfg!(target_os = "windows") { ... }
.
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.
if cfg!(target_os = "windows") { ... }
sounds good
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.
https://doc.rust-lang.org/std/macro.cfg.html
cfg!
, unlike#[cfg]
, does not remove any code and only evaluates to true or false. For example, all blocks in an if/else expression need to be valid whencfg!
is used for the condition, regardless of whatcfg!
is evaluating.
Unfortunately, cfg!()
would still compile the code and perform the check at runtime, which means it wouldn't compile for other platforms because clipboard-win
depends upon winapi
. This was what I originally did before I realized it didn't work, though the commits are now squashed.
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.
Also, the #[cfg]
macro also seems to want the following line to end in a semi-colon, so I have to make it an explicit return
.
We tried #119 It pulls in a lot of dependencies and doesn't quite work. So I'm not sure we want to merge these changes |
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.
Looks good to me. I don't think any of us have windows to test it out. Maybe someone else can just merge it.
Ah so we are going with |
I think copypaste also work but the issue is for wayland it pulls in a bunch of dependencies. At the end if we do it like this, it's similar to just use copypasta. |
Yeah, we could, but this crate is already small so I'm not sure it's worth it. |
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 like the two extra crates here, but it's on Windows 🤷🏻
I'd like to see this replaced by a direct winapi implementation in the future.
No description provided.