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.

Sign up for GitHub

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

Jump to bottom

Feat/improve copy to clipboard #118

Merged
merged 3 commits into from Feb 2, 2022
Merged

Feat/improve copy to clipboard #118

merged 3 commits into from Feb 2, 2022

Conversation

Jarsop
Copy link
Contributor

@Jarsop Jarsop commented Jan 9, 2022 •
edited

Adding a new field (copy_cmd) in the default configuration and in the configuration file.
It allows to easily add a new copy command.

Ninjani
Ninjani requested changes Jan 9, 2022
View reviewed changes
Copy link
Member

@Ninjani Ninjani left a comment

Choose a reason for hiding this comment

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

Thanks! I've added some comments

src/copy.rs Outdated Show resolved Hide resolved
src/copy.rs Outdated Show resolved Hide resolved
src/copy.rs Outdated Show resolved Hide resolved
tests/cli.rs Outdated Show resolved Hide resolved
@Jarsop
Copy link
Contributor Author

Jarsop commented Jan 9, 2022

Hello @Ninjani, does it means that you want to support only one command by OS and just override the default command by the configuration field ?

@Ninjani
Copy link
Member

Ninjani commented Jan 9, 2022

Yes, that's what I had in mind, that way it's easy to swap out with xsel or wl-copy or anything the user wants. What do you think?

@Jarsop
Copy link
Contributor Author

Jarsop commented Jan 9, 2022

Sounds good too, maybe a bit harder to handling the errors properly but more generic. I think an example in the README for that case will be welcome :) (I will add it)

@Jarsop
Copy link
Contributor Author

Jarsop commented Jan 9, 2022

I think that I will also add an error kind for the case of we are on a non supported OS by default copy command

@Jarsop Jarsop force-pushed the feat/improve_copy_to_clipboard branch from cf69edd to ccd658e Compare January 9, 2022 21:15
@Jarsop Jarsop requested a review from Ninjani January 9, 2022 21:15
@Jarsop
Copy link
Contributor Author

Jarsop commented Jan 9, 2022

I will update the README soon, after your review

@Jarsop
Copy link
Contributor Author

Jarsop commented Jan 23, 2022

@Ninjani Any update about this topic?

Ninjani
Ninjani requested changes Jan 25, 2022
View reviewed changes
Copy link
Member

@Ninjani Ninjani left a comment

Choose a reason for hiding this comment

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

Sorry got caught up with work. Looks pretty good, just a minor change and we're good to go I think.

src/errors.rs Outdated Show resolved Hide resolved
@Jarsop Jarsop force-pushed the feat/improve_copy_to_clipboard branch from ccd658e to 2c606ee Compare January 25, 2022 23:10
@Jarsop Jarsop requested a review from Ninjani January 25, 2022 23:11
@Jarsop
Copy link
Contributor Author

Jarsop commented Jan 25, 2022

Sorry got caught up with work. Looks pretty good, just a minor change and we're good to go I think.

No problem we are all busy :)

@Ninjani
Copy link
Member

Ninjani commented Jan 27, 2022

Looks good! Would you like to update the README and CHANGELOG?

@Jarsop
Copy link
Contributor Author

Jarsop commented Jan 27, 2022 •
edited

Looks good! Would you like to update the README and CHANGELOG?

Yes I will do it !

@Jarsop
Copy link
Contributor Author

Jarsop commented Jan 28, 2022 •
edited

Done. I also added an example with xsel and I incremented the minor version (I don't know if you consider it a patch or a minor increment). Moreover I added a warning message when the copy_cmd field value is empty.

@Jarsop
refact(copy): refactor copy clipboard
675f835 
Adding a new field (`copy_cmd`) in the default configuration
and in the configuration file.
It allows to easily add a new copy command.

Signed-off-by: Jean-Pierre Geslin <jarsop@outlook.com>
@Jarsop
refact(snippets): apply Clippy suggestions
5a82124 
Signed-off-by: Jean-Pierre Geslin <jarsop@outlook.com>
@Jarsop Jarsop force-pushed the feat/improve_copy_to_clipboard branch from c887cde to 4be6eb6 Compare January 28, 2022 02:18
@Jarsop
docs(cp): add copy_cmd field to readme and changelog
35d45cd 
Signed-off-by: Jean-Pierre Geslin <jarsop@outlook.com>
@Jarsop Jarsop force-pushed the feat/improve_copy_to_clipboard branch from 4be6eb6 to 35d45cd Compare January 28, 2022 02:23
This was linked to issues Feb 2, 2022
Replace copy_to_clipboard code with arboard #76
Closed
Add xsel support #110
Closed
@Ninjani Ninjani merged commit c882c12 into out-of-cheese-error:master Feb 2, 2022
@Ninjani
Copy link
Member

Ninjani commented Feb 2, 2022

Thanks @Jarsop I'll make a new release this weekend

@Jarsop Jarsop deleted the feat/improve_copy_to_clipboard branch February 9, 2022 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ninjani Ninjani Ninjani approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add xsel support Replace copy_to_clipboard code with arboard
2 participants
@Jarsop @Ninjani