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

CLI: simplify and make friendlier and more Unix-like #100

Open
murlakatamenka opened this issue Mar 12, 2024 · 8 comments
Open

CLI: simplify and make friendlier and more Unix-like #100

murlakatamenka opened this issue Mar 12, 2024 · 8 comments

Comments

@murlakatamenka
Copy link

Here are some of the issues:

  1. Long flags should be made kebab case for readability:
  • --listoutputs -> --list-outputs
  • --choseoutputs -> --choose-outputs

cp, mv, curl, wget, rg have it like that.

  1. --file <PATH> and --stdout can be replaced with optional [FILE] argument.
match `FILE` {
    Some('-') => // stdout
    Some(path) => // write to path
    None => {
       // write <unixtime>-wayshot.png to current dir
       // print "Saved to <path>" to stdin/stdout, to make the default invocation obvious
    }
}

- for stdout is a common Unix convention.

  1. Help (--help)
  • move --debug to the end of the help, because that's not what a user typically looks for when learning how to use a new tool
  • specify supported extensions. Now they are only mentioned in README and manpage
  • colors! Clap v4 can have clap v3 colored help or custom style, for example, I like rustic's (permalink)
  1. --list-outputs

Currently, output of --list-outputs isn't suitable to pipe to another selector (fzf/skim or GUI dmenu-like fuzzel, wofi), because it's done via tracing:

if args.get_flag("listoutputs") {
let valid_outputs = wayshot_conn.get_all_outputs();
for output in valid_outputs {
tracing::info!("{:#?}", output.name);
}
exit(1);
}

It's output to stderr and exits with an error code 1 for no reason.

Should be: just 1 output per line to stdout, and 0 exit code. Good for:

wayshot --output "$(wayshot --list-outputs | fuzzel --dmenu)"

to be bound to a specific hotkey, like PrintScreen.

  1. Add TAB-completion

clap is used for CLI, clap_complete is one step away. Completion scripts can be generated during build time, there is already bulid.rs in the project


I'll make a draft PR to address those issues, to me all of them are straight improvements of wayshot's UX.

@CheerfulPianissimo
Copy link

There's already a CLI UX rewrite in the freeze-feat branch slated for merge. Some of these concerns have been addressed there so it may be better to build off that.

@murlakatamenka
Copy link
Author

@CheerfulPianissimo true that, I'll take a look and give feedback (and contributions, if needed) there. Thanks.


I see it's been rewritten with clap+derive, and "-" is used for stdout, pretty good 👍

murlakatamenka referenced this issue Mar 12, 2024
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.
@AndreasBackx
Copy link
Member

These seem to be the only issues left in the freeze branch:

  1. --list-outputs: I think I missed actually fixing the implementation for it, but it shouldn't be hard to add.
  2. Tab completion: like you said clap_complete should do. I'm unsure if cargo would be able to install it but package managers are able to. We'll need to include them in the releases which shouldn't be hard. Let me know if anyone wants to implement this, I can give some help on the things you need to look out for. The most important part is that it's generated based on an environment variable, not based on the CLI arguments. I currently don't have the time for this myself right now however.

@Gigas002
Copy link

Gigas002 commented Apr 2, 2024

I've just noticed, that #104 partially breaks what's being asked in this issue, the - instead of stdout part, to be precise. While implementing config file, I though that it would be great to give users more freedom with screenshot output options, so users can have a file output and stdout output at the same time, if they want to (when using - in place of file user can't do that). So I brought back the stdout parameter instead of - and slightly changed the overall flow

@AndreasBackx
Copy link
Member

@Gigas002

While implementing config file, I though that it would be great to give users more freedom with screenshot output options, so users can have a file output and stdout output at the same time, if they want to (when using - in place of file user can't do that). So I brought back the stdout parameter instead of - and slightly changed the overall flow

I disagree that one should send output to the stdout AND to an additional file. This is confusing and not implemented anywhere else. Could you give a good usecase where this behaviour is desired?

@CheerfulPianissimo
Copy link

Also the tee tool exists precisely for this functionality. Piping stdout to it seems to me the more UNIX-ey solution.

@Shinyzenith
Copy link
Member

I find myself asking the question: "why not?"
It leads to no extra overhead and is literally changing one else if to a separate if block

@Shinyzenith
Copy link
Member

But there's also the question of "is anyone going to realistically use this?"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants