add write-to-disk argument to screenshot actions

[sornas]

Closes #916

I added a write_to_disk: bool parameter to most of the screenshot actions and a condition in save_screenshot. It was fairly straightforward but I would probably want to add a test. Where do you think that would go?

An alternative that I thought about at the end is to store the screenshot in the tmp directory instead if !write_to_disk, because my understanding is that in order to show an image preview in the notification we still need a file on disk somewhere. That would be very easy to implement since all of the parameter stuff would be the same.

Also, maybe we should rename save_screenshot since the name may be a bit misleading now.

Let me know what you think!

[YaLTeR]

Thanks, looks reasonable. Some small changes.

It was fairly straightforward but I would probably want to add a test. Where do you think that would go?

Hm, not sure what you can usefully and reasonably easily test here. What did you have in mind?

An alternative that I thought about at the end is to store the screenshot in the tmp directory instead if !write_to_disk, because my understanding is that in order to show an image preview in the notification we still need a file on disk somewhere.

There are actually some other ways to show an image: https://specifications.freedesktop.org/notification-spec/latest/icons-and-images.html But you'd need to check the actual notification daemons like mako to see what they support.

Anyhow I don't think we should save to /tmp, at least not in this PR. Maybe something for later after considering our options.

Also, maybe we should rename save_screenshot since the name may be a bit misleading now.

Hmm, I guess, but I don't immediately have any good name suggestions so let's leave it as is.

[YaLTeR]

How does this look on the CLI? Do you have to do --write-to-disk=false? Or is there a --no-write-to-disk? Could you show what the help looks like?

[sornas]

$ cargo r -- msg action screenshot-screen --help
Screenshot the focused screen

Usage: niri msg action screenshot-screen [OPTIONS]

Options:
  -w, --write-to-disk <WRITE_TO_DISK>  Save the screenshot in addition to putting it in your clipboard [default: true] [possible values: true, false]
  -h, --help                           Print help

so you would do niri msg action screenshot-window --write-to-disk false (or -w false). --no-write-to-disk would probably also work, I'd have to look at clap but I would be surprised if they don't support it. I think screenshot-window -w false is a bit more intuitive in short form but I don't have strong opinions here.

[sornas]

Apparently clap does not support automatic --no-XYZ flags (clap-rs/clap#815) so we would have to do it manually. I can implement that if you want. As I said, I have no strong opinion.

[YaLTeR]

Eh, I suppose it's fine as is then

[YaLTeR]

Bikeshed: should the short flag be -d for disk perhaps rather than -w for write? Or would that be more confusing

[sornas]

-d also works. I think it's fine to change it later if another flag is added that fits better, especially for the short flag.

[sornas]

It was fairly straightforward but I would probably want to add a test. Where do you think that would go?

Hm, not sure what you can usefully and reasonably easily test here. What did you have in mind?

No, you're right, I'm also having trouble figuring out what would be tested.

[sornas]

That should be all review comments. if write_to_disk.then(|| match ...).flatten() is a bit more crowded than I would've liked but I think it's ok. At least it feels better than let path = if write_to_disk { match ... } else { None };.

[YaLTeR]

Missed the: "The screenshot is saved according to the screenshot-path config setting."

[sornas]

Sorry, I can't find where that would be. (I tried grepping for it.)

[YaLTeR]

My suggested change from earlier: #975 (comment)

[sornas]

Oh! I misunderstood the diff. Sorry about that.

[sornas]

$ cargo r -- msg action screenshot-window --help
     Running `target/debug/niri msg action screenshot-window --help`
Screenshot the focused window

Usage: niri msg action screenshot-window [OPTIONS]

Options:
      --id <ID>
          Id of the window to screenshot.
          
          If `None`, uses the focused window.

  -d, --write-to-disk <WRITE_TO_DISK>
          Write the screenshot to disk in addition to putting it in your clipboard.
          
          The screenshot is saved according to the `screenshot-path` config setting.
          
          [default: true]
          [possible values: true, false]

  -h, --help
          Print help (see a summary with '-h')

[YaLTeR]

Thanks!