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

Add --list-options and shell completions #377

Merged
merged 3 commits into from
Jun 4, 2024

Conversation

N-R-K
Copy link
Collaborator

@N-R-K N-R-K commented May 27, 2024

This adds a --list-options flag which outputs a specialized TSV of all the options intended to be used in shell completion scripts. A couple TODOs:

  • Make --list-options accept an optional argument. By default it should output human readable format, but when using --list-options=tsv it would output the TSV.
  • make install should install the zsh completion script
  • Completion script for bash
  • Add --list-options to manpage etc
  • Should --help output also include (human readable) list of options??

@N-R-K N-R-K mentioned this pull request May 27, 2024
3 tasks
@N-R-K N-R-K force-pushed the list-options branch 3 times, most recently from 212d429 to 774ac94 Compare May 27, 2024 08:02
@N-R-K
Copy link
Collaborator Author

N-R-K commented May 27, 2024

make install should install the zsh completion script

Not sure how to tackle this one. Shell completion script dirs are not as well "standardized" as /usr/local/. For example, older zsh version doesn't recognize /usr/local/share/zsh/site-functions by default. On the other hand, installing into /usr/share.. violates the assumption that make install files goes to /usr/local....

Maybe we shouldn't try to install the completion files and just let distro packagers install it to appropriate directory for the respective distro. Or make it so that completion files will be installed only when explicitly requested via something like ./configure --with-zsh-completion-path="..." or make install ZSH_COMP_PATH="...".

@N-R-K
Copy link
Collaborator Author

N-R-K commented May 28, 2024

Should --help output also include (human readable) list of options??

I'm thinking we could probably just remove the current usage string entirely and utilize the new --list-options for help string:

$ ./scrot -h
Usage:  scrot [OPTIONS] [FILE]

OPTIONS
=======
-a, --autoselect <x,y,w,h>      autoselect provided region
-b, --border                    capture the window borders as well
-C, --class <NAME>              capture specified window class
-c, --count                     display a countdown for delay
-D, --display <DISPLAY>         capture specified display
-d, --delay <[b]SEC>            add delay before screenshot
-e, --exec <CMD>                execute command on saved image
-F, --file <FILE>               specify output file
-f, --freeze                    freeze the screen when -s is used
-h, --help                      display help and exit
-i, --ignorekeyboard            ignore keyboard
-k, --stack[=v|h]               capture overlapped window and join them
-l, --line <STYLE>              specify the style of the selection line
-M, --monitor <NUM>             capture monitor
-m, --multidisp                 capture all monitors
-o, --overwrite                 overwrite the output file if needed
-p, --pointer                   capture the mouse pointer as well
-q, --quality <NUM>             image quality
-s, --select[=OPTS]             interactively select a region to capture
-t, --thumb <% | WxH>           also generate a thumbnail
-u, --focused                   capture the currently focused window
-u, --focussed                  capture the currently focused window
-v, --version                   output version and exit
-w, --window <WID>              X window ID to capture
-Z, --compression <LVL>         image compression level
-z, --silent                    prevent beeping
--format <FMT>                  specify output file format
--list-options[=human|tsv]      list all options

This seems significantly more helpful than the current -h output. It also includes long opt only options like --format which the older help string didn't.

EDIT: Updated the --help output to the above. Feedback welcome.

@N-R-K N-R-K force-pushed the list-options branch 2 times, most recently from 2d8c228 to 171f38b Compare May 28, 2024 22:54
@N-R-K N-R-K marked this pull request as ready for review May 28, 2024 22:55
@N-R-K N-R-K force-pushed the list-options branch 2 times, most recently from 5b14c2e to 4cf3ce8 Compare May 28, 2024 23:04
@N-R-K N-R-K added this to the v1.11 milestone May 29, 2024
@N-R-K N-R-K changed the title Add --list-options and zsh completion Add --list-options and shell completions May 29, 2024
@N-R-K
Copy link
Collaborator Author

N-R-K commented Jun 1, 2024

Any feedback on this? I'd like to merge this and cut a release sometime this week.

@daltomi
Copy link
Collaborator

daltomi commented Jun 2, 2024

This seems significantly more helpful than the current -h output

Yes I agree. Previously the format was like this but then it was decided to change it, see: b4ca02e ("use the manual's synopsis as help text (#104)", 2021-07-28)
I don't currently have a preference.

@daltomi
Copy link
Collaborator

daltomi commented Jun 2, 2024

In this one, as in previous commits ;-) the copyright date needs to be updated.

@daltomi
Copy link
Collaborator

daltomi commented Jun 2, 2024

make install ZSH_COMP_PATH="...".

I like this one better, if possible, I don't know. But making a target in Makefile ala make install-shell or something like that would be best.

@daltomi
Copy link
Collaborator

daltomi commented Jun 3, 2024

It would be necessary to add the etc/ directory, so that when doing make dist it would include it.

diff --git a/Makefile.am b/Makefile.am
index 63e275e..497b212 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -40,6 +40,8 @@ dist_doc_DATA = AUTHORS ChangeLog CONTRIBUTING.md doc/scrot.png FAQ.md README.md
 
 EXTRA_DIST = $(man_MANS) autogen.sh deps.pc
 
+EXTRA_DIST += etc/
+
 SUBDIRS = src
 
 distclean-local:

@N-R-K
Copy link
Collaborator Author

N-R-K commented Jun 3, 2024

It would be necessary to add the etc/ directory, so that when doing make dist it would include it.

Good catch. Fixed.

the copyright date needs to be updated.

I will go through all the commits since the previous version and update the copyright date in another commit.

"use the manual's synopsis as help text (#104)"

That was okay in the past, but currently we have long options which have no short options (e.g --format). Showing only short opt excludes these long only opt.

N-R-K added 3 commits June 3, 2024 07:50
shows a more detailed list of options now via the
`--list-options` flag.
@daltomi
Copy link
Collaborator

daltomi commented Jun 3, 2024

Or make it so that completion files will be installed only when explicitly requested via something like ./configure --with-zsh-completion-path="..."

Hi,
I was doing some tests and as you indicate it can be done (It is similar but with environment variable), but not with make install-shell as I would like :-/
I attach the patch.
scrot.txt
This works as follows. First, clarify that the names of the variables/options are vague, it's just an idea.

The new options are:

 --with-zsh-confpath     install autocomplete zsh files
 --with-bash-confpath    install autocomplete bash files

Example:

$ ./configure --with-zsh-confpath
configure: error: the variable ZSH_CONFPATH no set

It expects to find a ZSH_CONFPATH environment variable defined. We define it like this( or whatever, export, set, etc)

$  ./configure --prefix=/usr/local --with-zsh-confpath ZSH_CONFPATH=/tmp/zsh-autocompletion

Then we install to test:

make DESTDIR=/tmp/scrot install

It is installed on: /tmp/scrot/usr/local and /tmp/zsh-autocompletion/
Note that the --prefix option had no impact on the installation of zsh

That is an approximation, it would be necessary to see if there is a better or more robust one because this patch does not uninstall the shell files for example.

@N-R-K
Copy link
Collaborator Author

N-R-K commented Jun 4, 2024

Note that the --prefix option had no impact on the installation of zsh

I think that's probably fine, but I'm not sure.

configure: error: the variable ZSH_CONFPATH no set

Can we make it so that the path is given as an argument? E.g:

$ ./configure --with-zsh-confpath=/tmp/zsh-complete

I think this is more easy to use.

@N-R-K N-R-K merged commit 9c6da4b into resurrecting-open-source-projects:master Jun 4, 2024
18 checks passed
@N-R-K
Copy link
Collaborator Author

N-R-K commented Jun 4, 2024

I've merged this PR since the autocompletion itself works.

I attach the patch.

For installing the files, you can now create your own PR. I'm not good with autoconf, so I wouldn't be able to do it myself anyways.

@N-R-K N-R-K deleted the list-options branch June 4, 2024 07:06
@daltomi
Copy link
Collaborator

daltomi commented Jun 4, 2024 via email

@N-R-K
Copy link
Collaborator Author

N-R-K commented Jun 9, 2024

@daltomi I was thinking it might be time for a release. Do you think we should wait to add autoconf/makefile completion installation before making a release?

I think most distros/package-managers will not have any trouble installing those files. It will be useful only for people who are not using package manager and using make install directly.

@daltomi
Copy link
Collaborator

daltomi commented Jun 9, 2024 via email

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

Successfully merging this pull request may close these issues.

2 participants