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: xclip fallback for linux with xsel #352 #390

Merged
merged 4 commits into from
Nov 1, 2020

Conversation

denismaxim0v
Copy link
Contributor

No description provided.

@denismaxim0v
Copy link
Contributor Author

denismaxim0v commented Oct 30, 2020

Implements #352

@extrawurst
Copy link
Owner

Thanks @denismaxim0v ❤️Will look into this today

@extrawurst
Copy link
Owner

so this looks ok. could be done a little more readable/scalable by having a list of tools and their arguments and iterate it. but I need a second pair of eyes with a linux machine to test this on either way

@denismaxim0v
Copy link
Contributor Author

so this looks ok. could be done a little more readable/scalable by having a list of tools and their arguments and iterate it. but I need a second pair of eyes with a linux machine to test this on either way

Should I propose another solution in that case? And if so what kind of tools would we include in the list?

Copy link
Contributor

@1wilkens 1wilkens left a comment

Choose a reason for hiding this comment

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

Overall this looks good. The gen_command function could be a bit more generic, but for now this should be fine.

Cargo.toml Show resolved Hide resolved
src/clipboard.rs Show resolved Hide resolved
@1wilkens
Copy link
Contributor

so this looks ok. could be done a little more readable/scalable by having a list of tools and their arguments and iterate it. but I need a second pair of eyes with a linux machine to test this on either way

Sorry didn't see this comment before reviewing, but that's basically my only nitpick as well. I for one would like Wayland support eventually, but I can also do that in another PR.

@denismaxim0v
Copy link
Contributor Author

so this looks ok. could be done a little more readable/scalable by having a list of tools and their arguments and iterate it. but I need a second pair of eyes with a linux machine to test this on either way

Sorry didn't see this comment before reviewing, but that's basically my only nitpick as well. I for one would like Wayland support eventually, but I can also do that in another PR.

OK cool, I could participate in another PR too. Anything else needed from me here?

@denismaxim0v
Copy link
Contributor Author

Let me know if I have any work to do here or should we move to another issue please?

Co-authored-by: Florian Wilkens <gh@1wilkens.org>
@extrawurst extrawurst merged commit cad40d1 into extrawurst:master Nov 1, 2020
@extrawurst
Copy link
Owner

@denismaxim0v and @1wilkens thanks for your help❤️

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.

3 participants