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

Added native Windows clipboard support #373

Merged
merged 3 commits into from
Jun 30, 2021
Merged

Conversation

kirawi
Copy link
Member

@kirawi kirawi commented Jun 24, 2021

No description provided.

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);
Copy link
Member Author

@kirawi kirawi Jun 24, 2021

Choose a reason for hiding this comment

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

Need the returns 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.)

Copy link
Contributor

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") { ... }.

Copy link
Member

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

Copy link
Member Author

@kirawi kirawi Jun 27, 2021

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 when cfg! is used for the condition, regardless of what cfg! 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.

Copy link
Member Author

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.

@cessen
Copy link
Contributor

cessen commented Jun 24, 2021

A possibly dumb question, not directly related to this PR (which is just following the pattern already set out in the code), but:
why aren't we just using something like the clipboard or copypasta crates to handle the details for us in a (presumably) cross-platform way?

@archseer
Copy link
Member

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

@kirawi kirawi closed this Jun 25, 2021
@kirawi kirawi reopened this Jun 25, 2021
Copy link
Contributor

@pickfire pickfire left a 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.

@archseer
Copy link
Member

Ah so we are going with clipboard-win then? I thought the idea was to implement this directly using winapi

@pickfire
Copy link
Contributor

pickfire commented Jun 26, 2021

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.

@kirawi
Copy link
Member Author

kirawi commented Jun 26, 2021

Ah so we are going with clipboard-win then? I thought the idea was to implement this directly using winapi

Yeah, we could, but this crate is already small so I'm not sure it's worth it.

Copy link
Member

@archseer archseer left a 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.

@archseer archseer merged commit acaf22d into helix-editor:master Jun 30, 2021
@kirawi kirawi deleted the patch-02 branch August 22, 2021 01:05
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.

5 participants