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

feature: Copy snippet to clipboard in case of single result #184

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

hannotify
Copy link
Contributor

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. 😉

@hannotify
Copy link
Contributor Author

hannotify commented Feb 13, 2023

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 java.awt classes. So I'll try to update the build.yml with support for a dummy display.

@mthmulders
Copy link
Owner

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 -c/--copy switch or similar make sense to address that? The disadvantage of that would of course be that it's easy to "forget" that.

Curious how you think about this.

@hannotify
Copy link
Contributor Author

hannotify commented Feb 16, 2023

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 --copy flag each time a single result is found?

❯ mcs search io.github.er1c:scala-stringutils_2.12
Searching for io.github.er1c:scala-stringutils_2.12...

    <dependency>
        <groupId>io.github.er1c</groupId>
        <artifactId>scala-stringutils_2.12</artifactId>
        <version>0.1.0</version>
    </dependency>

(To directly copy this snippet to the clipboard, run mcs with the -c or --copy flag.)

@mthmulders
Copy link
Owner

mthmulders commented Feb 22, 2023

What if we would suggest the --copy flag each time a single result is found?

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.

Copy link
Owner

@mthmulders mthmulders left a 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?

@hannotify
Copy link
Contributor Author

I'm about halfway implementing the -c/--copy flag, but I'm battling a nasty rippling effect there (lots of methods that need to pass the value of this copy parameter to each other). I feel like the project could eventually benefit from a lightweight DI framework, to enable injecting values (and names) of command-line parameters anywhere in the code. I'll probably create an issue on the topic somewhere next week.

@mthmulders mthmulders added this to the 0.4 milestone Mar 3, 2023
@hannotify
Copy link
Contributor Author

Finished the implementation (so please review again! 🙏) and opened #192 to address dependency injection in general.

Copy link
Owner

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

@hannotify hannotify requested a review from mthmulders April 10, 2023 17:39
@hannotify
Copy link
Contributor Author

Applied your suggestions!

Copy link
Owner

@mthmulders mthmulders left a 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!

@mthmulders
Copy link
Owner

  • The failing mutation tests are most likely going to be fixed in chore(build): Upgrade Pitest and friends #226.
  • The failure in the macOS-arm64-latest build is caused by a faulty runner (i.e., my Macbook), I'll take care of that.

@mthmulders
Copy link
Owner

mthmulders commented May 22, 2023

Alas, the macOS-arm64-latest build fails due to

The application is not running in a desktop session,
but this program performed an operation which requires it.

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)

@mthmulders
Copy link
Owner

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.

@mthmulders mthmulders force-pushed the feature/copy-snippet-to-clipboard branch from 0d47907 to 778f50e Compare May 22, 2023 19:30
@mthmulders mthmulders modified the milestones: 0.4, 0.5 Jul 11, 2023
@mthmulders mthmulders removed this from the 0.5 milestone Sep 5, 2023
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.

2 participants