[PATCH 0/1] *** Add completion for zathura ***

[PATCH 0/1] *** Add completion for zathura ***

[doron.behar]

From: Doron Behar <doron.behar@gmail.com>

Doron Behar (1):
  Add completion for zathura.

 Completion/X/Command/_zathura | 49 +++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Completion/X/Command/_zathura

-- 
2.17.0

[PATCH 1/1] Add completion for zathura.

[doron.behar]

From: Doron Behar <doron.behar@gmail.com>

Make it aware of the plugins installed on the machine.
Use 2 spaces instead of tabs.
Use +functions[] for all helpers.
Make main file completion consider plugins dir.
Improve portability and use safer, more resources efficient when
evaluating files regex.
Small improvements to options' descriptions.
---
 Completion/X/Command/_zathura | 49 +++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Completion/X/Command/_zathura

diff --git a/Completion/X/Command/_zathura b/Completion/X/Command/_zathura
new file mode 100644
index 000000000..29cec11f2
--- /dev/null
+++ b/Completion/X/Command/_zathura
@@ -0,0 +1,49 @@
+#compdef zathura
+
+(( $+functions[_zathura_files] )) ||
+_zathura_files(){
+  for plugins_dir in "${opt_args[-p]}" "${opt_args[--plugins-dir]}" "/usr/lib/zathura"; do
+    if ! [[ -z "${plugins_dir}" ]]; then
+      break
+    fi
+  done
+  local -a plugin_files=(${plugins_dir}/*.so)
+  if [[ -z "${plugin_files}" ]]; then
+    _message -r "no plugins found on plugins dir"
+    return
+  fi
+  local -a supported_filetypes
+  for pf in "${plugin_files[@]}"; do
+    if [[ $pf =~ "mupdf" ]]; then
+      supported_filetypes+="pdf"
+      supported_filetypes+="epub"
+      supported_filetypes+="xps"
+    elif [[ $pf =~ "poppler" ]]; then
+      supported_filetypes+="pdf"
+    else
+      supported_filetypes+="${${pf%.so}#${plugins_dir}}"
+    fi
+  done
+  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}"
+}
+
+_arguments \
+  {-e,--reparent=}'[Reparents to window specified by xid]:XID: ' \
+  {-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)' \
+  {-x,--synctex-editor-command=}'[Set the synctex editor command]:COMMAND:_command' \
+  '--synctex-forward=[Jump to the given position]:INPUT: ' \
+  '--synctex-pid=[Instead of looking for an instance having the correct file opened, try only the instance with the given PID]:PID:_pids' \
+  '--fork[Fork into background]: : ' \
+  '(- :)--version[Display version string and exit]' \
+  '(- :)--help[Display help and exit]' \
+  '*:FILE:_zathura_files'
-- 
2.17.0

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

[Oliver Kiddle]

doron.behar@gmail.com 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.

> +  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})(-.)"

> +
> +_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.

X IDs are completed by _x_window.


> +  {-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.

> +  {-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.

Oliver

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

[Daniel Shahaf]

doron.behar@gmail.com wrote on Fri, 08 Jun 2018 18:53 +0300:
> Use 2 spaces instead of tabs.
> Small improvements to options' descriptions.

Huh?  These lines don't make sense since there is no _zathura in master.
Could you fill us in on the background here?

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

[Bart Schaefer]

On Fri, Jun 8, 2018 at 9:36 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> doron.behar@gmail.com wrote on Fri, 08 Jun 2018 18:53 +0300:
>> Use 2 spaces instead of tabs.
>> Small improvements to options' descriptions.
>
> Huh?  These lines don't make sense since there is no _zathura in master.
> Could you fill us in on the background here?

It's a squashed commit, so it's got all his logs from individual
revisions prior to the squash.

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

[Doron Behar]

On Fri, Jun 08, 2018 at 10:01:32AM -0700, Bart Schaefer wrote:
> On Fri, Jun 8, 2018 at 9:36 AM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > doron.behar@gmail.com wrote on Fri, 08 Jun 2018 18:53 +0300:
> >> Use 2 spaces instead of tabs.
> >> Small improvements to options' descriptions.
> >
> > Huh?  These lines don't make sense since there is no _zathura in master.
> > Could you fill us in on the background here?
> 
> It's a squashed commit, so it's got all his logs from individual
> revisions prior to the squash.

Correct.

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

[Doron Behar]

On Fri, Jun 08, 2018 at 06:28:44PM +0200, Oliver Kiddle wrote:
> doron.behar@gmail.com 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

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

[Oliver Kiddle]

On 9 Jun, Doron Behar wrote:
> 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.

The file list is here:
https://koji.fedoraproject.org/koji/rpminfo?rpmID=4869616
That's a zathura 0.2.7 build for RHEL 7 which is perhaps somewhat old.

I would strongly suggest putting a fallback in for at least *.pdf.
You never know when some overly clever logic is going to fail.

> 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}}(-.)"

I'd suggest sticking to pattern characters like (...|...) over brace
expansion in the parameter to _files -g. If braces are working, it is by
accident rather than design.

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

_cmdstring handled a quoted command-line where you might have arguments
to the command. Judging from the example in the documentation, that is
appropriate in this case.

Oliver