waycrate / wayshot

Mirrored at https://git.sr.ht/~shinyzenith/wayshot | screenshot tool for wlroots based compositors implementing zwlr_screencopy_v1
https://crates.io/crates/wayshot
BSD 2-Clause "Simplified" License
114 stars 20 forks source link

Improve CLI design #76

Closed AndreasBackx closed 7 months ago

AndreasBackx commented 11 months ago

Improve CLI design

This "improves" (and that is subjective) the design of the CLI. I am aiming to get some feedback on what people think of the new design:

Screenshot tool for wlroots based compositors implementing the zwlr_screencopy_v1 protocol.

Usage: wayshot [OPTIONS] [OUTPUT]

Arguments:
  [OUTPUT]
          Where to save the screenshot, "-" for stdout. Defaults to "$UNIX_TIMESTAMP-wayshot.$EXTENSION"

Options:
      --log-level <LOG_LEVEL>
          Log level to be used for printing to stderr

          [default: info]
          [possible values: trace, debug, info, warn, error]

  -s, --slurp <SLURP_ARGS>
          Arguments to call slurp with for selecting a region

  -c, --cursor
          Enable cursor in screenshots

      --encoding <FILE_EXTENSION>
          Set image encoder, if output file contains an extension, that will be used instead

          [default: png]
          [aliases: extension, format, output-format]

          Possible values:
          - jpg: JPG/JPEG encoder
          - png: PNG encoder
          - ppm: PPM encoder
          - qoi: Qut encoder

  -l, --list-outputs
          List all valid outputs

  -o, --output <OUTPUT>
          Choose a particular output/display to screenshot

      --choose-output
          Present a fuzzy selector for output/display selection

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

  -V, --version
          Print version

The main changes are:

  1. --debug is now --log-level because this makes it easy to select more specifically what log level to use. I considered using -v, -vv... to increase verbosity but the clap-verbosity-crate uses log and not tracing. We could use it somewhat, but we'd pull in log (which seems fine) and we'd need to map from log's Level to tracing's Level enums (they use inverse ordering).
  2. --stdout and --file has been made an optional positional argument. This because it's what other CLIs often do and I wasn't sure what to call the option otherwise because --output and -O/-o is often what others use but we use it here to refer to displays/monitors/Wayland outputs. This avoids that confusion hopefully and I've also clarified this in the documentation.
    • Additionally if a path is given, its extension will always be used. So you cannot save jpg to foo.png. Perhaps this behaviour can be changed, though I don't see a reason to support this weird edge case? When is someone saving png to jpg?
  3. --extension is --encoding with aliases like extension.

Again, let me know what you think.


Stack created with Sapling. Best reviewed with ReviewStack.

Shinyzenith commented 11 months ago

I'm in support of all these changes. Can we just emit a warning or similar level claiming that custom encoding was passed but file inferred a different encoding.

Let's default to the custom provided encoding through --encoding

Shinyzenith commented 11 months ago

Let's default to the custom provided encoding through --encoding

I suggest this because the user is explicitly providing the encoding which is the point of the flag regardless of the context inferred from the file name.

If there's any arguments against this, I'd love to hear them and find a better path forward.

AndreasBackx commented 11 months ago

@Shinyzenith --encoding now is always used even if the extension is not the same. Then a warning is posted however:

➜ cargo run --release -- --encoding qoi something.png
   Compiling wayshot v1.4.0-dev (/home/andreas/dev/wayshot-sapling/wayshot)
    Finished release [optimized] target(s) in 1.01s
     Running `target/release/wayshot --encoding qoi something.png`
2023-11-09T19:52:51.916179Z  WARN wayshot: The encoding requested 'qoi' does not match the output file's encoding 'png'. Still using the requested encoding however.
AndreasBackx commented 11 months ago

ppm is giving an error. Though I don't know enough about color types and image formats whether or not this is expected:

➜ cargo run --release -- --encoding ppm
   Compiling wayshot v1.4.0-dev (/home/andreas/dev/wayshot-sapling/wayshot)
    Finished release [optimized] target(s) in 1.00s
     Running `target/release/wayshot --encoding ppm`
Error: The parameter is malformed: Color type can not be represented in the chosen format

Location:
    wayshot/src/wayshot.rs:117:9