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

Bump clipboard-win from v3.0.2 to v4.5.0 #66

Closed
wants to merge 1 commit into from
Closed

Bump clipboard-win from v3.0.2 to v4.5.0 #66

wants to merge 1 commit into from

Conversation

rhysd
Copy link

@rhysd rhysd commented Nov 20, 2023

I confirmed examples worked on my Windows 10 machine.

Small trick in this patch is that clipboard_win::SystemError does not implement std::error::Error so we need to wrap the error instance by ourselves. This should be okay because the wrapped error instance is hidden by Box<dyn Error> trait object.

Fixes #65


use crate::common::{ClipboardProvider, Result};

// Wrap `SystemError` since it does not implement `Error` trait
struct WrappedError(SystemError);
Copy link
Member

Choose a reason for hiding this comment

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

WrappedError is a pretty bad name, would probably be better to make it WindowsError, WindowsClipboardError, WindowsSystemError, ClipboardSystemError, or something similar.

Copy link
Author

Choose a reason for hiding this comment

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

Sure. I will update it.

Copy link
Author

Choose a reason for hiding this comment

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

@chrisduerr I chose WindowsSystemError and force-pushed the change to this branch: 795726f

@kchibisov
Copy link
Member

The only reason I haven't bumped dep is because there was no point in doing so given that I don't remember that it provided any value. However I was waiting for their 5.0 version, which will use windows-rs so we should be able to drop winapi.

@rhysd
Copy link
Author

rhysd commented Nov 20, 2023

However I was waiting for their 5.0 version, which will use windows-rs so we should be able to drop winapi.

I noticed it, but I'm not sure when it is actually released. And a new major version tends to be buggy. Once 5.0 is released, I'll try to make another PR. Meanwhile, can we use v4.5.0?

@kchibisov
Copy link
Member

And a new major version tends to be buggy

Tends or is? It's not like you can't try to update and test yourself.

but I'm not sure when it is actually released.

You can ask and explain why you want a bump?

@chrisduerr
Copy link
Member

Considering version 4 is the most widely used version out there, it's probably just useful for deduplication purposes. Though not sure why you'd have two versions of this in your tree.

That said, the maintenance isn't amazing so not updating is probably the safe choice.

@rhysd
Copy link
Author

rhysd commented Nov 20, 2023

Sorry I may not get the point of discussion. My intention was upgrading the dependency to the most stable latest version v4.5.0 and then upgrading the version to v5 once it is released. What is a problem with it?

@kchibisov
Copy link
Member

I mean, you could try to work on upgrade straight to v5 and work with upstream on getting stable version. They already have pre-release, I don't mind bump, but it seems like we'll bump dep anyway to v5, which is preferred.

@chrisduerr
Copy link
Member

My intention was upgrading the dependency to the most stable latest version v4.5.0 and then upgrading the version to v5 once it is released. What is a problem with it?

Well technically if 4.Y.Z is receiving constant updates, but doesn't provide anything over 3.Y.Z which is working without drawbacks without updates, then 3.Y.Z is the more stable version.

@rhysd
Copy link
Author

rhysd commented Nov 20, 2023

Hmm, OK, I'll give up this upgrade and try other crate.

@rhysd rhysd closed this Nov 20, 2023
rhysd added a commit to rhysd/tui-textarea that referenced this pull request Nov 20, 2023
@Tastaturtaste
Copy link

That said, the maintenance isn't amazing so not updating is probably the safe choice.

How is not getting any patches safer than getting some patches?

Not updating this dependency means this bug still causes crashes today even though it was fixed in 2022.

Obviously you are free to do whatever you want with your crate in your own time, but, like @rhysd, I will consider arboard over this crate for my needs because of this.

@kchibisov
Copy link
Member

You can update to v5.0.0 as was said. abroad serves different purpose compared to this crate, but again, if you don't do windowing you don't need this crate.

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

Successfully merging this pull request may close these issues.

clipboard-win dependency is outdated
4 participants