zathura conpletion for zsh broken

zathura conpletion for zsh broken

[Oliver Freyermuth]

[-- Attachment #1: Type: text/plain, Size: 502 bytes --]

Hi,

the zathura completion which was recently added breaks for me. I have:
-----------------------------
$ ls -1 /usr/lib/zathura/*
/usr/lib/zathura/cb.so
/usr/lib/zathura/djvu.so
/usr/lib/zathura/pdf.so
/usr/lib/zathura/ps.so
-----------------------------
The completion code fails to strip the plugin_path correctly in case the plugin is neither called poppler nor mupdf. 
The attached patch fixes the situation for me. 

I am not subscribed to list, please include me in replies. 

Cheers,
	Oliver

[-- Attachment #2: 0001-Fix-zathura-completion-supported_filetypes.patch --]
[-- Type: text/x-patch, Size: 816 bytes --]

From 5959af248d0574f27612485f43b8064247cb7b6e Mon Sep 17 00:00:00 2001
From: Oliver Freyermuth <o.freyermuth@googlemail.com>
Date: Tue, 25 Sep 2018 17:08:14 +0200
Subject: [PATCH] Fix zathura completion supported_filetypes.

---
 Completion/X/Command/_zathura | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Completion/X/Command/_zathura b/Completion/X/Command/_zathura
index 141cadf63..cc07828b3 100644
--- a/Completion/X/Command/_zathura
+++ b/Completion/X/Command/_zathura
@@ -25,7 +25,7 @@ _zathura_files(){
     elif [[ $pf =~ "poppler" ]]; then
       supported_filetypes+="pdf"
     else
-      supported_filetypes+="${${pf%.so}#${plugins_dir}/lib}"
+      supported_filetypes+="${${pf%.so}#${plugins_dir}/}"
     fi
   done
   _files -g "*.(${(j.|.)supported_filetypes})(-.)"
-- 
2.16.4

Re: zathura conpletion for zsh broken

[Daniel Shahaf]

Oliver Freyermuth wrote on Tue, Sep 25, 2018 at 17:14:04 +0200:
> +++ b/Completion/X/Command/_zathura
> @@ -25,7 +25,7 @@ _zathura_files(){
>      elif [[ $pf =~ "poppler" ]]; then
>        supported_filetypes+="pdf"
>      else
> -      supported_filetypes+="${${pf%.so}#${plugins_dir}/lib}"
> +      supported_filetypes+="${${pf%.so}#${plugins_dir}/}"

Isn't this equivalent to «supported_filetypes+=${pf:t:r}»?

The 'break' on line 12 looks odd.  Does zathura really ignore
/usr/lib/zathura/foo.so if /usr/local/lib/zathura/bar.so exists and
/usr/local/lib/zathura/foo.so does not?

Thanks,

Daniel

Re: zathura conpletion for zsh broken

[Oliver Freyermuth]

Am 25.09.18 um 21:21 schrieb Daniel Shahaf:
> Oliver Freyermuth wrote on Tue, Sep 25, 2018 at 17:14:04 +0200:
>> +++ b/Completion/X/Command/_zathura
>> @@ -25,7 +25,7 @@ _zathura_files(){
>>      elif [[ $pf =~ "poppler" ]]; then
>>        supported_filetypes+="pdf"
>>      else
>> -      supported_filetypes+="${${pf%.so}#${plugins_dir}/lib}"
>> +      supported_filetypes+="${${pf%.so}#${plugins_dir}/}"
> 
> Isn't this equivalent to «supported_filetypes+=${pf:t:r}»?

Indeed, it is, and that would be much easier. 

> 
> The 'break' on line 12 looks odd.  Does zathura really ignore
> /usr/lib/zathura/foo.so if /usr/local/lib/zathura/bar.so exists and
> /usr/local/lib/zathura/foo.so does not?

You are correct in spotting this, if I read the zathura code correctly (not a girara expert...),
it "does the right thing" and searches the full list of paths. 

I'll try to cook up a patch fixing both those issues. Might take a while though, since
I'm just starting with this (basically I started to investigate after "zathura <tab>" stopped
doing anything after a zsh upgrade, makinɡ usage rather cumbersome). 

Thanks for the pointers!
	Oliver

> 
> Thanks,
> 
> Daniel
> 

Re: zathura conpletion for zsh broken

[Daniel Shahaf]

Oliver Freyermuth wrote on Tue, 25 Sep 2018 22:02 +0200:
> Am 25.09.18 um 21:21 schrieb Daniel Shahaf:
> > Oliver Freyermuth wrote on Tue, Sep 25, 2018 at 17:14:04 +0200:
> >> +++ b/Completion/X/Command/_zathura
> >> @@ -25,7 +25,7 @@ _zathura_files(){
> >> -      supported_filetypes+="${${pf%.so}#${plugins_dir}/lib}"
> >> +      supported_filetypes+="${${pf%.so}#${plugins_dir}/}"
> > 
> > Isn't this equivalent to «supported_filetypes+=${pf:t:r}»?
> 
> Indeed, it is, and that would be much easier. 

Pushed.  I changed the log message to avoid mentioning implementation
terms (the variable's name) in ChangeLog, which is user-facing.

> > The 'break' on line 12 looks odd.  Does zathura really ignore
> > /usr/lib/zathura/foo.so if /usr/local/lib/zathura/bar.so exists and
> > /usr/local/lib/zathura/foo.so does not?
> 
> You are correct in spotting this, if I read the zathura code correctly
> (not a girara expert...), it "does the right thing" and searches the
> full list of paths.
> 
> I'll try to cook up a patch fixing both those issues. Might take a
> while though, since I'm just starting with this (basically I started
> to investigate after "zathura <tab>" stopped doing anything after a
> zsh upgrade, makinɡ usage rather cumbersome).

Thanks for the patch and looking into the additional issue.

Cheers,

Daniel

P.S. Our of curiosity, what's that U+0261 LATIN SMALL LETTER SCRIPT G doing there?

> Thanks for the pointers!
> 	Oliver

Re: zathura conpletion for zsh broken

[Oliver Freyermuth]

[-- Attachment #1: Type: text/plain, Size: 2357 bytes --]

Am 25.09.18 um 22:36 schrieb Daniel Shahaf:
> Oliver Freyermuth wrote on Tue, 25 Sep 2018 22:02 +0200:
>> Am 25.09.18 um 21:21 schrieb Daniel Shahaf:
>>> Oliver Freyermuth wrote on Tue, Sep 25, 2018 at 17:14:04 +0200:
>>>> +++ b/Completion/X/Command/_zathura
>>>> @@ -25,7 +25,7 @@ _zathura_files(){
>>>> -      supported_filetypes+="${${pf%.so}#${plugins_dir}/lib}"
>>>> +      supported_filetypes+="${${pf%.so}#${plugins_dir}/}"
>>>
>>> Isn't this equivalent to «supported_filetypes+=${pf:t:r}»?
>>
>> Indeed, it is, and that would be much easier. 
> 
> Pushed.  I changed the log message to avoid mentioning implementation
> terms (the variable's name) in ChangeLog, which is user-facing.

Thanks! Didn't know this is copied as-is, I'll take better care
in the future. Hopefully the commit message of the attached patch is better. 

>>> The 'break' on line 12 looks odd.  Does zathura really ignore
>>> /usr/lib/zathura/foo.so if /usr/local/lib/zathura/bar.so exists and
>>> /usr/local/lib/zathura/foo.so does not?
>>
>> You are correct in spotting this, if I read the zathura code correctly
>> (not a girara expert...), it "does the right thing" and searches the
>> full list of paths.
>>
>> I'll try to cook up a patch fixing both those issues. Might take a
>> while though, since I'm just starting with this (basically I started
>> to investigate after "zathura <tab>" stopped doing anything after a
>> zsh upgrade, makinɡ usage rather cumbersome).
> 
> Thanks for the patch and looking into the additional issue.

The attached patch fixes the second issue, I tested with a dummy .so
in /usr/local/lib/zathura. Plugin file formats are now also made unique. 
Let me know if this can be simplified, or if it is in any case unneeded since the "uniqueness"
is implicit in the matching code later on - I'm still learning more and more about the wonders of zsh expansion,
and every bit I learn leaves me wondering how I could ever survive without that knowledge up to now. 

Cheers,
Oliver

> P.S. Our of curiosity, what's that U+0261 LATIN SMALL LETTER SCRIPT G doing there?
Spotted very well, my font and mail client hid that from me. 
I did accidentally hit the keybinding for SCIM (Smart Common Input Method) while typing,
and it started to interfere. That created some funny characters, and apparently I missed to remove the
"g" once I noticed it. 

[-- Attachment #2: 0001-Fix-zathura-completion-ignoring-multiple-plugin-dire.patch --]
[-- Type: text/x-patch, Size: 1082 bytes --]

From 3d95608365dfc165c7a77bd3caad69b66b09f97b Mon Sep 17 00:00:00 2001
From: Oliver Freyermuth <o.freyermuth@googlemail.com>
Date: Tue, 25 Sep 2018 23:08:33 +0200
Subject: [PATCH] Fix zathura completion ignoring multiple plugin directories.

---
 Completion/X/Command/_zathura | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Completion/X/Command/_zathura b/Completion/X/Command/_zathura
index 64888d013..5335c1143 100644
--- a/Completion/X/Command/_zathura
+++ b/Completion/X/Command/_zathura
@@ -8,8 +8,7 @@ _zathura_files(){
     ${(Qv)opt_args[(i)-p|--plugins-dir]}(#qN) \
     {/usr/local,/usr,}/lib/zathura(#qN)
   do
-    plugins_files=( $plugins_dir/*.so )
-    (( $#plugins_files )) && break
+    plugins_files+=( $plugins_dir/*.so )
   done
   if [[ -z "${plugins_files}" ]]; then
     _files -g "*.pdf(-.)"
@@ -28,7 +27,7 @@ _zathura_files(){
       supported_filetypes+="${pf:t:r}"
     fi
   done
-  _files -g "*.(${(j.|.)supported_filetypes})(-.)"
+  _files -g "*.(${(j.|.)${(@u)supported_filetypes}})(-.)"
 }
 
 _arguments -s -S \
-- 
2.16.4

Re: zathura conpletion for zsh broken

[Daniel Shahaf]

Oliver Freyermuth wrote on Tue, 25 Sep 2018 23:22 +0200:
> Am 25.09.18 um 22:36 schrieb Daniel Shahaf:
> > Oliver Freyermuth wrote on Tue, 25 Sep 2018 22:02 +0200:
> >> Am 25.09.18 um 21:21 schrieb Daniel Shahaf:
> >>> Oliver Freyermuth wrote on Tue, Sep 25, 2018 at 17:14:04 +0200:
> >>>> +++ b/Completion/X/Command/_zathura
> >>>> @@ -25,7 +25,7 @@ _zathura_files(){
> >>>> -      supported_filetypes+="${${pf%.so}#${plugins_dir}/lib}"
> >>>> +      supported_filetypes+="${${pf%.so}#${plugins_dir}/}"
> >>>
> >>> Isn't this equivalent to «supported_filetypes+=${pf:t:r}»?
> >>
> >> Indeed, it is, and that would be much easier. 
> > 
> > Pushed.  I changed the log message to avoid mentioning implementation
> > terms (the variable's name) in ChangeLog, which is user-facing.
> 
> Thanks! Didn't know this is copied as-is, I'll take better care in the
> future. Hopefully the commit message of the attached patch is better.

Yes, it is, thank you.

Not all developers copy the message as-is.  I happen to use git-am(1)
to apply patches so it's easier for me to consume patches produced
by git-send-email(1) or git-format-patch(1), like yours.  Other developers
use other tools and don't care so much about the format so long as it
can be piped to patch(1) or git-apply(1).

[ Actually, I use a wrapper around git-am(1) that also automatically
adds the mailing list message number to the log message and creates
a ChangeLog entry.  You might find it an interesting read:
https://github.com/danielshahaf/zsh-dev/ ]

> >>> The 'break' on line 12 looks odd.  Does zathura really ignore
> >>> /usr/lib/zathura/foo.so if /usr/local/lib/zathura/bar.so exists and
> >>> /usr/local/lib/zathura/foo.so does not?
> >>
> >> You are correct in spotting this, if I read the zathura code correctly
> >> (not a girara expert...), it "does the right thing" and searches the
> >> full list of paths.
> >>
> >> I'll try to cook up a patch fixing both those issues. Might take a
> >> while though, since I'm just starting with this (basically I started
> >> to investigate after "zathura <tab>" stopped doing anything after a
> >> zsh upgrade, makinɡ usage rather cumbersome).
> > 
> > Thanks for the patch and looking into the additional issue.
> 
> The attached patch fixes the second issue, I tested with a dummy .so
> in /usr/local/lib/zathura. Plugin file formats are now also made
> unique. Let me know if this can be simplified, or if it is in any case
> unneeded since the "uniqueness" is implicit in the matching code later
> on - I'm still learning more and more about the wonders of zsh
> expansion, and every bit I learn leaves me wondering how I could ever
> survive without that knowledge up to now.
>

I've changed:
.
    +    plugins_files+=( $plugins_dir/*.so )
.
to:
.
    +    plugins_files+=( $plugins_dir/*.so(N) )
.
to avoid an error if one of the directories exists and contains no *.so
files.  This was a preëxisting bug in the function (it means the '((
$#plugin_files ))' check was redundant).

Regarding uniqueness, another way to achieve uniqueness is to pass -U to
the 'typedef -a' command that declares the variable.

Also, it's not idiomatic to use [[ -z $array ]], though it will function correctly.

> Cheers,
> Oliver
> 
> > P.S. Our of curiosity, what's that U+0261 LATIN SMALL LETTER SCRIPT G doing there?
> Spotted very well, my font and mail client hid that from me. I did
> accidentally hit the keybinding for SCIM (Smart Common Input
> Method) while typing, and it started to interfere. That created
> some funny characters, and apparently I missed to remove the "g"
> once I noticed it.

OK :)

Cheers,

Daniel

Re: zathura conpletion for zsh broken

[Oliver Freyermuth]

Am 26.09.18 um 00:56 schrieb Daniel Shahaf:
> Oliver Freyermuth wrote on Tue, 25 Sep 2018 23:22 +0200:
>> Thanks! Didn't know this is copied as-is, I'll take better care in the
>> future. Hopefully the commit message of the attached patch is better.
> 
> Yes, it is, thank you.
> 
> Not all developers copy the message as-is.  I happen to use git-am(1)
> to apply patches so it's easier for me to consume patches produced
> by git-send-email(1) or git-format-patch(1), like yours.  Other developers
> use other tools and don't care so much about the format so long as it
> can be piped to patch(1) or git-apply(1).
> 
> [ Actually, I use a wrapper around git-am(1) that also automatically
> adds the mailing list message number to the log message and creates
> a ChangeLog entry.  You might find it an interesting read:
> https://github.com/danielshahaf/zsh-dev/ ]

[ That's indeed a good read, many thanks for the link! ]

>> The attached patch fixes the second issue, I tested with a dummy .so
>> in /usr/local/lib/zathura. Plugin file formats are now also made
>> unique. Let me know if this can be simplified, or if it is in any case
>> unneeded since the "uniqueness" is implicit in the matching code later
>> on - I'm still learning more and more about the wonders of zsh
>> expansion, and every bit I learn leaves me wondering how I could ever
>> survive without that knowledge up to now.
>>
> 
> I've changed:
> .
>     +    plugins_files+=( $plugins_dir/*.so )
> .
> to:
> .
>     +    plugins_files+=( $plugins_dir/*.so(N) )
> .
> to avoid an error if one of the directories exists and contains no *.so
> files.  This was a preëxisting bug in the function (it means the '((
> $#plugin_files ))' check was redundant).

Good point! 

> Regarding uniqueness, another way to achieve uniqueness is to pass -U to
> the 'typedef -a' command that declares the variable.

That's indeed more beautiful in most cases, and noted down for the future. 
I hope I can make good use of that with a future contribution which will surely come
latest when I start to use a tool for which the completion is lacking or even missing. 

Many thanks for the helpful advice! 

Cheers,
	Oliver

> 
> Also, it's not idiomatic to use [[ -z $array ]], though it will function correctly.
> 
>> Cheers,
>> Oliver
>>
>>> P.S. Our of curiosity, what's that U+0261 LATIN SMALL LETTER SCRIPT G doing there?
>> Spotted very well, my font and mail client hid that from me. I did
>> accidentally hit the keybinding for SCIM (Smart Common Input
>> Method) while typing, and it started to interfere. That created
>> some funny characters, and apparently I missed to remove the "g"
>> once I noticed it.
> 
> OK :)
> 
> Cheers,
> 
> Daniel
> 

Re: zathura conpletion for zsh broken

[Leah Neukirchen]

Oliver Freyermuth <o.freyermuth@googlemail.com> writes:

> Am 25.09.18 um 21:21 schrieb Daniel Shahaf:
>> Oliver Freyermuth wrote on Tue, Sep 25, 2018 at 17:14:04 +0200:
>>> +++ b/Completion/X/Command/_zathura
>>> @@ -25,7 +25,7 @@ _zathura_files(){
>>>      elif [[ $pf =~ "poppler" ]]; then
>>>        supported_filetypes+="pdf"
>>>      else
>>> -      supported_filetypes+="${${pf%.so}#${plugins_dir}/lib}"
>>> +      supported_filetypes+="${${pf%.so}#${plugins_dir}/}"
>> 
>> Isn't this equivalent to «supported_filetypes+=${pf:t:r}»?
>
> Indeed, it is, and that would be much easier. 

Hm, on my system (Void Linux with zathura-pdf-mupdf 0.3.3 and meson
build) the files are called

/usr/lib/zathura/libdjvu.so
/usr/lib/zathura/libpdf-mupdf.so
/usr/lib/zathura/libps.so

So the lib-stripping seems to make sense?

-- 
Leah Neukirchen  <leah@vuxu.org>  http://leah.zone

Re: zathura conpletion for zsh broken

[Daniel Shahaf]

[-- Attachment #1: Type: text/plain, Size: 1054 bytes --]

Leah Neukirchen wrote on Wed, 26 Sep 2018 23:08 +0200:
> Oliver Freyermuth <o.freyermuth@googlemail.com> writes:
> 
> > Am 25.09.18 um 21:21 schrieb Daniel Shahaf:
> >> Oliver Freyermuth wrote on Tue, Sep 25, 2018 at 17:14:04 +0200:
> >>> +++ b/Completion/X/Command/_zathura
> >>> @@ -25,7 +25,7 @@ _zathura_files(){
> >>>      elif [[ $pf =~ "poppler" ]]; then
> >>>        supported_filetypes+="pdf"
> >>>      else
> >>> -      supported_filetypes+="${${pf%.so}#${plugins_dir}/lib}"
> >>> +      supported_filetypes+="${${pf%.so}#${plugins_dir}/}"
> >> 
> >> Isn't this equivalent to «supported_filetypes+=${pf:t:r}»?
> >
> > Indeed, it is, and that would be much easier. 
> 
> Hm, on my system (Void Linux with zathura-pdf-mupdf 0.3.3 and meson
> build) the files are called
> 
> /usr/lib/zathura/libdjvu.so
> /usr/lib/zathura/libpdf-mupdf.so
> /usr/lib/zathura/libps.so
> 

I'm on Debian and I have /usr/lib/zathura/{djvu,pdf,ps}.so.

> So the lib-stripping seems to make sense?

Patch attached.

Thanks,

Daniel

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Teach-_zathura-to-support-both-naming-conventions-of.patch --]
[-- Type: text/x-patch; name="0001-Teach-_zathura-to-support-both-naming-conventions-of.patch", Size: 889 bytes --]

From 094fef6e8fa9895c59ddd58e2a1f4492cefdf463 Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Thu, 27 Sep 2018 18:20:19 +0000
Subject: [PATCH] Teach _zathura to support both naming conventions of
 /usr/lib/zathura/*.so modules.

---
 Completion/X/Command/_zathura | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Completion/X/Command/_zathura b/Completion/X/Command/_zathura
index 615488068..9c26910d8 100644
--- a/Completion/X/Command/_zathura
+++ b/Completion/X/Command/_zathura
@@ -24,7 +24,9 @@ _zathura_files(){
     elif [[ $pf =~ "poppler" ]]; then
       supported_filetypes+="pdf"
     else
+      # Some systems have /usr/lib/zathura/djvu.so, others have /usr/lib/zathura/libdjvu.so.
       supported_filetypes+="${pf:t:r}"
+      supported_filetypes+="${${pf:t:r}#lib}"
     fi
   done
   _files -g "*.(${(j.|.)${(@u)supported_filetypes}})(-.)"

Re: zathura conpletion for zsh broken

[Bart Schaefer]

On Thu, Sep 27, 2018 at 11:21 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> Patch attached.

I've asked this before but I'm going to ask it again (and this is not
aimed specially at Daniel or anything):

Please do not attach patches (or anything else) with content-type
"text/x-something" (in this case "text/x-patch").  A remarkable number
of mail clients (or perhaps it's just the operating systems they sit
on) are STILL, even after 25 years, too stupid to understand that the
"text/" prefix means it's OK to display the content without invoking a
special application.  This makes it impossible to view the patch
content without first detaching/downloading it and then explicitly
feeding it to an editor or text viewer.

Also, using text/x-whatever tends to cause sending mailers to
base64-encode the content, so it can't even be read by viewing the
original message source.

If this means not naming your patch files with a ".patch" extension,
I'm sorry.  Rename them ".txt" before you attach them, if your mailer
won't allow you to specify the content-type.  Thanks.

Re: zathura conpletion for zsh broken

[Daniel Shahaf]

Bart Schaefer wrote on Thu, 27 Sep 2018 11:31 -0700:
> On Thu, Sep 27, 2018 at 11:21 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> >
> > Patch attached.
> 
> I've asked this before but I'm going to ask it again (and this is not
> aimed specially at Daniel or anything):
> 
> Please do not attach patches (or anything else) with content-type
> "text/x-something" (in this case "text/x-patch").

Sorry, forgot that was an issue.

> A remarkable number of mail clients (or perhaps it's just the
> operating systems they sit on) are STILL, even after 25 years, too
> stupid to understand that the "text/" prefix means it's OK to display
> the content without invoking a special application.

Hopefully people who see such behaviours will file bugs against those
applications for not supporting rfc2046.  (This is not aimed specially at
Bart, either. :-))

> Also, using text/x-whatever tends to cause sending mailers to
> base64-encode the content, so it can't even be read by viewing the
> original message source.

Alpine uses base64, IIRC, but most others use quoted-printable, which is
fine for eyeballing the patch but breaks applying it.

> If this means not naming your patch files with a ".patch" extension,
> I'm sorry.  Rename them ".txt" before you attach them, if your mailer
> won't allow you to specify the content-type.  Thanks.

Done:

% git config --global format.suffix '.patch.txt'

Cheers,

Daniel
(I also use format.signature='' because the '---' line breaks recountdiff(1))