Re: [PATCH] Add completion for zathura.

[Doron Behar <doron.behar@xxxxxxxxx>]

Sorry for the long wait on my reply. I've responded to your comments
below. I'll resend a patch when this is done.

On Sun, Jul 01, 2018 at 02:06:57PM -0500, dana wrote:
> On 1 Jul 2018, at 12:11, Doron Behar <doron.behar@xxxxxxxxx> wrote:
> >Hey guys, just reminding you there's a patch here ready for your
> >comments / merge.
> 
> Some things i noticed (disregard anything that's too nit-picky i guess):
> 
> On 13 Jun 2018, at 11:31, doron.behar@xxxxxxxxx wrote:
> >+_zathura_files(){
> 
> * I don't think you want to quote the variables in your for loop; whenever
>   they're empty, you'll be looking for files matching /*.so
> 
> * However, you maybe *do* want to apply (Q) to the opt_args values, since
>   they're taken 'raw' from the command line, and quoting file paths is a very
>   common thing that people do

How quoting `opt_args` and use `(Q)` will be better as for this issue?

> 
> * Is the first check really supposed to be [[ -z $plugin_files ]] rather than
>   -n? It seems like it's going to immediately break out of the loop unless
>   _arguments found -p (or there are random shared libs in /)

You were right, the opposite is correct - the break is needed when have
reached a plugins directory which has no shared objects in it. But I
don't see a difference between `-z` and `-n` so I'll show here what I've
come up with so far:

	for plugins_dir in "${opt_args[-p]}" "${opt_args[--plugins-dir]}" "/usr/local/lib/zathura" "/usr/lib/zathura" "/lib/zathura"; do
	  if [[ ! -z "${plugins_dir}" ]]; then
	    plugins_files=("${plugins_dir}"/*.so)
	    if [[ ! -z "${plugins_files}" ]]; then
	      break
	    fi
	  fi
	done

> 
> * Should /usr/lib have precedence over /usr/local/lib? It's not typical
> 

Agree.

> * plugins_dir and pf need made local

Done.

> 
> On 13 Jun 2018, at 11:31, doron.behar@xxxxxxxxx wrote:
> >+_arguments \
> 
> * I think this could use -s and -S
> 
> * Since Zathura uses GLib's dumb option parser, i think (?) it's right that
>   these are given in -o,--opt= form rather than the usual -o+,--opt=. Might want
>   to make a note tho; i always double-take when i see that personally

Done.

> 
> * The descriptions for the options are inconsistently worded (some are verb
>   phrases, some use the indicative mood, some are just describing the argument)
>   and they don't conform to the usual capitalisation conventions
> 
> * Some of the optarg descriptions aren't that helpful ('number')
> 

These are the options' descriptions I've ended up with:

	{-e,--reparent=}'[specify xid of window to reparent to]:xid:_x_window' \
	{-c,--config-dir=}'[specify path to the config directory]:config directory:{_files -/}' \
	{-d,--data-dir=}'[specify path to the data directory]:data directory:{_files -/}' \
	{-p,--plugins-dir=}'[specify path to the directory containing plugins]:plugins directory:{_files -/}' \
	{-w,--password=}"[specify a password for the document]:password: " \
	{-P,--page=}'[open the document at the given page number]:page number: ' \
	{-l,--log-level=}'[set log level]:log level:(debug info warning error)' \
	{-x,--synctex-editor-command=}'[specify synctex editor command]:synctex editor command:_cmdstring' \
	'--synctex-forward=[jump to the given position]:synctex position: ' \
	'--synctex-pid=[specify pid of an instance having the correct file opened]:synctex pid:_pids' \
	'--fork[fork into background]' \
	'(- :)--version[display version string and exit]' \
	'(- :)--help[display help and exit]' \

> dana
> 

Doron