gabm / Satty

Satty - Modern Screenshot Annotation. A tool inspired by Swappy and Flameshot.
Mozilla Public License 2.0
388 stars 18 forks source link

Clipboard save doesn't seem to be picked by `wl-paste` #19

Closed marcosnils closed 4 months ago

marcosnils commented 7 months ago

:wave: thx for building satty, it's super useful.

I'm using cliphist (https://github.com/sentriz/cliphist) as a history manager and the way that its instructions recommend setting it up is via wl-paste --watch cliphist store

I've noticed that when using satty, wl-paste --watch doesn't catch things that are being copied into the clipboard by it. Not sure what's missing here but a possible solution could be to allow sending the output to stdout so I can pipe it to wl-copy. I just did a quick test myself by commenting this validation https://github.com/gabm/Satty/blob/main/src/sketch_board.rs#L157-L164 and then doing satty -- output-filename /dev/stdout | wl-copy and that worked.

gabm commented 7 months ago

yeah.. this is bugging me a bit.. there are multiple issues with clipboard in wayland

The strategy you describe sounds good, maybe we can use --output-filename - for stdout similar to --filename - for stdin?

There are some things to take care of though:

ystyle commented 7 months ago

I solved this way

# watch image and store
exec-once = wl-paste --type image --watch cliphist store
# watch text copy and store
exec-once = wl-paste --type text --watch cliphist store

# shift + super + v , show  clipboard history, and select item will copy
bind = SUPER_SHIFT, V, exec, cliphist list | rofi -dmenu | cliphist decode | wl-copy
# screenshot
bind = ,Print, exec, grim -g "$(slurp)" - | satty --filename - --fullscreen --output-filename ${HOME}/Pictures/截图/截图_$(date +%Y-%m-%d_%H%M%S).png
marcosnils commented 7 months ago

I solved this way

this works, but seems like it doesn't with --early-exit.

Good thing is that the /dev/stdout approach works with early-exit

gabm commented 7 months ago

i suppose it doesn't work because the of the race condition (see above). we might add a timeout to make early exit work with clipboard managers

marcosnils commented 7 months ago

i suppose it doesn't work because the of the race condition (see above). we might add a timeout to make early exit work with clipboard managers

yes, I was assuming that. Good thing is that the /dev/stdout approach works all the time consistently without requiring timeouts. I'd prefer to keep using this variant if possible :pray:

marcosnils commented 7 months ago

There are some things to take care of though:

  • what about logs, do they need to go to stderr then?

I'd say so.

  • if stdout is used, its the same as "no filename specified", meaning the "save to file" action is not available somehow.. or is it? .. mode confusion ;)

Maybe if no flag specified still render the "save" option and make it output via stdout by default. If you want to redirect it to a file, add the --output-filename flag. This way you don't break backwards compatibility and/or make the --output-filename flag confusing.

gabm commented 7 months ago

or even remove the output option and always write to stdout - and always read from stdin...

gabm commented 7 months ago

do you feel like giving it a shot for the dash option to write to stdout?

marcosnils commented 7 months ago

do you feel like giving it a shot for the dash option to write to stdout?

sure, been wanting to find an excuse to write some rust code. You'll have to bare with me since I haven't done any rust before.

gabm commented 7 months ago

like me then :) go ahead..

i can give you guidance. Contact me on @gabm:matrix.org

marcosnils commented 7 months ago

do you feel like giving it a shot for the dash option to write to stdout?

is the dash used for stdout though? Isn't it better to make stdout by default? So satty --filename - would take the file through stdin and then automatically save via stdout.

gabm commented 7 months ago

I am not 100% convinced of this unfortunately. What happens if the user presses "save" twice? then wl-copy will not work I suppose?

What about fixing the core-problem by using: https://github.com/YaLTeR/wl-clipboard-rs/? That provides a way to fork a daemon that stays alive and serves the data until someone else copies something in the system.

marcosnils commented 7 months ago

I am not 100% convinced of this unfortunately. What happens if the user presses "save" twice? then wl-copy will not work I suppose?

every time the user presses save, it'll write to stdout each image. It's actually quite convenient since if you do: satty | wl-copy --type image, each time you press save, the "last" modified image will be in the clipboard.

What about fixing the core-problem by using: https://github.com/YaLTeR/wl-clipboard-rs/? That provides a way to fork a daemon that stays alive and serves the data until someone else copies something in the system.

how would this work with the --early-exit option? Will it keep the daemon alive until someone copies the data from it?

gabm commented 7 months ago

how would this work with the --early-exit option? Will it keep the daemon alive until someone copies the data from it?

yes it would fork the process and keep the forked process alive until something else takes over the clipboard..


I gave it more thought and while I am still not 100% convinced, I see no harm in writing the result to stdout. Still, I would ask you to trigger that option by providing - as argument. I would like to keep the current behavior of "nothing specified -> no saved icon visible"...

gabm commented 7 months ago

I am still open to merge your PR, but I did solve it differently using a new option --copy-command wl-copy. Released in https://github.com/gabm/Satty/releases/tag/v0.8.0.

marcosnils commented 6 months ago

Still, I would ask you to trigger that option by providing - as argument

thing is that using - to indicate that something is sent to stdout is really confusing. - at the end of a command is generally used to indicate that the arguments that the commend will expect will be sent through stdin.. I'm not sure about this.

gabm commented 6 months ago

Okay.. understood. I did read about best practices for this, for example this: https://clig.dev/#output. The bottom line is

So to summarize, if you still want to go ahead with your PR, I kindly ask you to

marcosnils commented 6 months ago
  • send all log to stderr - or at least create an issue for it :)

I can do this. That should be a quick change. Thx for coming back to this one.

  • I did not find any resource talking about the meaning of -

found an article that explains has a reasonable explanation: https://www.baeldung.com/linux/dash-in-command-line-parameters.

gabm commented 6 months ago

thanks for the link.. unfortunately (for you) it even supports my point

As we can see, we used the “-” for both stdin and stdout. It’s up to the command itself to interpret it accordingly.

but again: no strong feelings

gabm commented 4 months ago

for now this became stale.. pls re-open if you feel like it