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

Clipboard integration #89

Conversation

CheerfulPianissimo
Copy link

This PR attempts to implement native clipboard support for wayshot using wl-clipboard-rs as discussed in #48

First, I decided to refactor in a new Enum type SaveLocation for representing the where the screenshot goes now that it's no longer a boolean choice. I've used -p or --clipboard as the command line switch for the feature.

wl-clipboard-rs has been added as a dependency and the copying functionality seems to be working with a few caveats I wanted to discuss before proceeding further:

  • wl-clipboard-rs only works with compositors using the wlr-data-control protocol, it doesn't work on GNOME's mutter for instance: Implement hack/workaround for missing wlr-data-control YaLTeR/wl-clipboard-rs#39
  • 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.

@Shinyzenith
Copy link
Member

wl-clipboard-rs only works with compositors using the wlr-data-control protocol, it doesn't work on GNOME's mutter for instance:

That is fine as anything apart from wlroots is out of our project scope.

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.

This is a very well known limitation of the protocol and partly the reason why it was left out of the feature set for a while.

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.

Maybe we can explore this approach if it can be robust enough. I say this from a preliminary overview and naturally I'd need some more code review before I can confirm this.

@Shinyzenith
Copy link
Member

Before you write any further code. I'd like to kindly request you to rebase your work on freeze-feat-andreas branch as we're currently working on some breaking changes for libwayshot and wayshot.

@CheerfulPianissimo
Copy link
Author

I think It'll probably be easier to close this PR and do another one on that branch with how changed it is, I'll admit I don't know enough about git convention to be sure of this tho so advice will be appreciated.

I'm also considering having the clipboard copy feature not be exclusive of the stdout or file save modes as it is in this PR. That would also make it a feature not trivially synonymous to wayshot - | wl-copy. wayshot can copy to the clipboard and simultaneously save to a file/stdout.

@Shinyzenith
Copy link
Member

@CheerfulPianissimo Apologies for the late reply, if you're not comfortable with rebase then feel free to open a new pr with the target branch set to the devel branch.

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.

2 participants