-
Notifications
You must be signed in to change notification settings - Fork 23
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
feature: Copy snippet to clipboard in case of single result #184
base: main
Are you sure you want to change the base?
feature: Copy snippet to clipboard in case of single result #184
Conversation
So it looks like the hardware that runs the Github Actions for this project has a headless JDK installed that doesn't allow the use of |
Thanks for picking this up, @hannotify! I'll make sure to go through the code in more detail later, but I've got one functional issue upfront where I'm a bit in doubt. Indeed, the README.md suggests copying to the clipboard immediately, but now I'm thinking about that again - does it make sense? It would overwrite something else the user has on their clipboard. Would a Curious how you think about this. |
Good point, didn't think about that while implementing it. I agree that overwriting the existing contents on the clipboard seems a tad invasive. But if we hide it behide a command line flag it isn't easily discoverable as a new feature. What if we would suggest the
|
That sounds like a great idea to me! In the meantime, I've opened #187 to explore ways to solve this in a generic way. |
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.
Thanks again for picking up this idea, and apologies for reacting with so much delay!
I am very pleased with your approach. Could you please introduce the -c/--copy
flag as we've discussed in the conversation?
src/main/java/it/mulders/mcs/search/printer/clipboard/SystemClipboard.java
Outdated
Show resolved
Hide resolved
src/test/java/it/mulders/mcs/search/printer/CoordinatePrinterTest.java
Outdated
Show resolved
Hide resolved
I'm about halfway implementing the |
Finished the implementation (so please review again! 🙏) and opened #192 to address dependency injection in general. |
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.
Thanks a lot, @hannotify. Most remaining remarks are about the 'strictness' of tests. One question for the production code, related to #192.
src/test/java/it/mulders/mcs/search/printer/CoordinatePrinterTest.java
Outdated
Show resolved
Hide resolved
src/test/java/it/mulders/mcs/search/printer/CoordinatePrinterTest.java
Outdated
Show resolved
Hide resolved
src/main/java/it/mulders/mcs/search/printer/clipboard/CopyToClipboardConfiguration.java
Outdated
Show resolved
Hide resolved
Applied your suggestions! |
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.
Thanks a lot for your contribution and your patience, @hannotify!
|
Alas, the macOS-arm64-latest build fails due to
I'll have to figure out how to start that runner in a desktop session (and if I actually want to do that for every build) |
This is turning out worse than I thought: apparently, GraalVM doesn't support AWT - at least, not on macOS. Maybe the Liberica Native Image Kit (NIK) could be an alternative solution but I haven't yet looked into that. For now, I am going to revert the changes I've pushed to your branch, @hannotify. This topic clearly needs more attention. Let me be clear: the code looks good, and I'm sure it works in JVM mode, but packaging it to a native executable is going to be another challenge on this road. |
0d47907
to
778f50e
Compare
I read through
README.md
the other day and felt like giving the clipboard feature idea a shot.Feel free to comment on the implementation. 😉