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(clipboard): implement clipboard integration #91

Conversation

CheerfulPianissimo
Copy link

This PR adds a --clipboard flag and implements functionality to make the screenshot available on the clipboard using wl-clipboard-rs. All the caveats described in #89 (comment) are still applicable:

  • Reading up on the protocol, it appears that a process can only offer data to be copied only for as long as it stays alive. This is mostly fine for GUI apps but problems arise when you want to copy something to the clipboard and exit as a terminal app. Basically with this PR wayshot will offer up the screenshot for a small moment and immediately exit.
  • What this means in practice is that the clipboard functionality in this PR is unusable without a clipboard manager setup. If you have a clipboard manager listening, it will copy the screenshot into it's own memory the moment wayshot offers up the screenshot and the user can get their screenshot from there even after wayshot quits.
  • The way to circumvent this is to keep the process alive. wl-clipboard-rs's version of wl-copy for instance does some unsafe shenanigans to fork itself, disconnect stdin/stdout and maintains the image in-memory till some other process overwrites the clipboard: https://github.com/YaLTeR/wl-clipboard-rs/blob/10b35fb2699a0ff65888b1220804bb0c44b65e0f/wl-clipboard-rs-tools/src/bin/wl-copy.rs#L160 I believe this is the same thing wl-copy does. We'll also have to do something similar to achieve parity with the wayshot --stdout | wl-copy method.

Add the --clipboard flag and implement functionality to make image available on the clipboard using wl-clipboard-rs.
@Shinyzenith
Copy link
Member

Apart from some stylistics nits which can be addressed later on this pr looks fine to me! Thanks for the contribution.

CC: @Decodetalkers Can I get your input on it too?

@Shinyzenith Shinyzenith requested review from Decodetalkers and Shinyzenith and removed request for Decodetalkers February 29, 2024 14:16
Copy link
Collaborator

@Decodetalkers Decodetalkers left a comment

Choose a reason for hiding this comment

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

seems good to me!

@CheerfulPianissimo
Copy link
Author

CheerfulPianissimo commented Mar 2, 2024

The way to circumvent this is to keep the process alive. wl-clipboard-rs's version of wl-copy for instance does some unsafe shenanigans to fork itself, disconnect stdin/stdout and maintains the image in-memory till some other process overwrites the clipboard: https://github.com/YaLTeR/wl-clipboard-rs/blob/10b35fb2699a0ff65888b1220804bb0c44b65e0f/wl-clipboard-rs-tools/src/bin/wl-copy.rs#L160 I believe this is the same thing wl-copy does. We'll also have to do something similar to achieve parity with the wayshot --stdout | wl-copy method.

I tried this out and found that it could be pretty easily done with the help of the fork crate: https://docs.rs/fork/latest/fork/
I'm not sure if the feature is worth adding yet another dependency for though. fork being unix specific doesn't appear in the stdlib so I can't see a way to do it without a dep. How should I proceed, should I commit the change here or should it be in a another branch/PR.

@Shinyzenith
Copy link
Member

I think it's worth the dependency graph increase in this case. You can edit this pr itself.

Add functionality offering image on clipboard persistently in the background
@CheerfulPianissimo
Copy link
Author

Have added info about the wayshot persisting in background to the cli flag's description. Is there anything else that needs to be done here?

wayshot/src/wayshot.rs Outdated Show resolved Hide resolved
@CheerfulPianissimo
Copy link
Author

Oops, corrected the typo.

@Shinyzenith Shinyzenith merged commit b9219b1 into waycrate:freeze-feat-andreas Mar 23, 2024
3 checks passed
@Shinyzenith
Copy link
Member

Thank you for your work!

@Shinyzenith
Copy link
Member

Something I forgot to suggest but which can be done later - documentation ( the flag should've been explained for the users.)

@CheerfulPianissimo
Copy link
Author

As in the man pages? It is already documented in the CLI help. The man pages are entirely out of sync with the CLI changes in this branch. They will all have to be modified.

@Shinyzenith
Copy link
Member

As in the man pages?

Yes

The man pages are entirely out of sync with the CLI changes in this branch. They will all have to be modified.

I am aware but incrementally fixing it while introducing the changes is ideal, I will rewrite the outdated docs anyways.

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.

3 participants