Re: [PATCH 1/1] Add completion for zathura.

[Doron Behar <doron.behar@xxxxxxxxx>]

On Fri, Jun 08, 2018 at 06:28:44PM +0200, Oliver Kiddle wrote:
> doron.behar@xxxxxxxxx wrote:
> 
> Thanks for this, I have some comments:
> 
> > +(( $+functions[_zathura_files] )) ||
> > +_zathura_files(){
> > +  for plugins_dir in "${opt_args[-p]}" "${opt_args[--plugins-dir]}" "/usr/lib/zathura"; do
> 
> For my installation of zathura, this finds no plugins. Perhaps they are
> compiled in. It handles at least PDF and PostScript.
> 

I tried to get some clues as for this subject by digging into zathura's
source code (https://git.pwmt.org/pwmt/zathura). Could you please tell
me what are the files the `zathura` package (or what ever it's called)
provided under your distribution? If there are no `.so` files for at
least a PDF plugin, I'll put a fall-back here.

> > +  local files_regex="*.{${supported_filetypes[1]},"
> > +  for (( i = 2 ; i < ${#supported_filetypes[*]}; i ++)); do
> > +    files_regex="${files_regex}""${supported_filetypes[$i]}"","
> > +  done
> > +  files_regex="${files_regex}""${supported_filetypes[-1]}""}"
> > +  _files -g "${files_regex}"
> 
> I think this can be simplified to avoid the loop when constructing the
> file glob:
>   "*.(${(j.|.)supported_filetypes})(-.)"
> 

That's smart but it doesn't work. Yet, after reading a little bit the
documentation I've used this instead:

	_files -g "*.{${(j.,.)supported_filetypes}}(-.)"

> > +
> > +_arguments \
> > +  {-e,--reparent=}'[Reparents to window specified by xid]:XID: ' \
> 
> As with your previous function, please stick to conventions on case for
> descriptions: don't use uppercase for the first word. And don't put the
> headings (like XID, PATH, NUMBER, PASSWORD) in block capitals.

Got it, just like last time. I've prepared this contribution quite a
long time ago and I forgot to fix these little things just like with
`_luarocks` and more on the way...

> 
> X IDs are completed by _x_window.

Great.

> 
> 
> > +  {-c,--config-dir=}'[Path to the config directory]:PATH:{_files -/}' \
> > +  {-d,--data-dir=}'[Path to the data directory]:PATH:{_files -/}' \
> > +  {-p,--plugins-dir=}'[Path to the directory containing plugins]:PATH:{_files -/}' \
> > +  {-w,--password=}"[The document's password]:PASSWORD: " \
> > +  {-P,--page=}'[Opens the document at the given page number]:NUMBER: ' \
> > +  {-l,--log-level=}'[Set log level]:LEVEL:(debug, info, warning, error)' \
> 
> Remove the commas in the list. It is a space separated list when you
> specify multiple options like that.
> 

Got it.

> > +  {-x,--synctex-editor-command=}'[Set the synctex editor command]:COMMAND:_command' \
> 
> _command is the completer for the command reserved word so is not
> applicable here. You probably want either _command_names -e or
> _cmdstring.

I think I'll use `_cmdstring` since it doesn't complete aliases of
functions which probably wouldn't be interpreted properly by zathura.

> 
> Oliver