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
108 stars 18 forks source link

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

Open murlakatamenka opened 4 months ago

murlakatamenka commented 4 months ago

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.

  2. --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:

    https://github.com/waycrate/wayshot/blob/cb6bd68dbbe6ab70a5d8fe3bd04cc154f0631cd8/wayshot/src/wayshot.rs#L75-L81

    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.

  3. 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 commented 4 months ago

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 commented 4 months ago

@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 :+1:

AndreasBackx commented 4 months ago

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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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

Shinyzenith commented 3 months ago

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 commented 3 months ago

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