Re: PATCH: search XDG_DATA_DIRS in _setxkbmap completion

[Maximilian Bosch <maximilian@mbosch.me>]

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

Thanks a lot for your feedback!

I use ZSH as interactive shell quite excessively, but as you might've
noticed, I'm not that experienced with ZSH scripting, so thanks for
your patience :D

I applied your suggestions and re-checked the patch.

> How should empty array elements in XDG_DATA_DIRS be handled?  E.g.,
> XDG_DATA_DIRS=/foo:/bar::/baz ?

Isn't this handled by `[ -d $fullname ]`?

With best regards,

Maximilian

diff --git a/Completion/X/Command/_setxkbmap b/Completion/X/Command/_setxkbmap
index f7310ecdd..a98a07a7b 100644
--- a/Completion/X/Command/_setxkbmap
+++ b/Completion/X/Command/_setxkbmap
@@ -9,10 +9,12 @@ _setxkbmap() {
     setopt extendedglob
 
     # xkb files may be in different places depending on system
-    local dir sourcedir
-    for dir in /usr/lib/X11/xkb /usr/share/X11/xkb; do
-        if [ -d $dir ] ; then
-           sourcedir=$dir
+    local dir sourcedir fullname
+    local searchdirs="${XDG_DATA_DIRS:-/usr/lib:/usr/share:/usr/local/lib:/usr/local/share}:${XDG_DATA_HOME:-~/.local/share}"
+    for dir in ${(s.:.)searchdirs}; do
+        fullname="$dir/X11/xkb"
+        if [ -d $fullname ] ; then
+           sourcedir=$fullname
            break
         fi
     done

On Tue, Sep 18, 2018 at 04:10:30PM +0000, Daniel Shahaf wrote:
> Maximilian Bosch wrote on Tue, 18 Sep 2018 14:49 +0200:
> > this patch searches in $XDG_DATA_DIRS for `<XDG_DATA_DIR>/share/X11/xkb`
> > rather than using `/usr/lib` and `/usr/share`.
> > 
> > This is helpful on distros such as NixOS which don't adopt the FHS[1][2].
> > 
> > [1] https://github.com/NixOS/nixpkgs/pull/46152#issuecomment-421755892
> > [2] https://github.com/NixOS/nixpkgs/issues/46025
> > 
> > diff --git a/Completion/X/Command/_setxkbmap b/Completion/X/Command/_setxkbmap
> > index f7310ecdd..b3f8b1a46 100644
> > --- a/Completion/X/Command/_setxkbmap
> > +++ b/Completion/X/Command/_setxkbmap
> > @@ -10,12 +10,18 @@ _setxkbmap() {
> >  
> >      # xkb files may be in different places depending on system
> >      local dir sourcedir
> > -    for dir in /usr/lib/X11/xkb /usr/share/X11/xkb; do
> > -        if [ -d $dir ] ; then
> > -           sourcedir=$dir
> > +    setopt sh_word_split
> > +    local IFS=:
> > +    for dir in $XDG_DATA_DIRS; do
> > +        fullName="$dir/X11/xkb"
> > +        if [ -d $fullName ] ; then
> > +           sourcedir=$fullName
> >             break
> >          fi
> >      done
> > +    unset IFS
> > +    unsetopt sh_word_split
> > +
> 
> Thanks for the patch.  It's short, but there's a lot to say about it.
> 
> 1. Shouldn't there be a fallback when $XDG_DATA_DIRS is unset?  Such as
>    the two hardcoded paths the patch is removing?  Maybe also /usr/local
>    equivalents of them.
> 
> 2. Instead of setting + unsetting wordsplit and IFS it will be more
>    idiomatic and more robust to do 'for dir in ${(s.:.)XDG_DATA_DIRS}'.
> 
> 3. fullName should be made local (and not named in CamelCase).
> 
> 4. ${XDG_DATA_HOME:-~/.local/share} should be examined too.
> 
> How should empty array elements in XDG_DATA_DIRS be handled?  E.g.,
> XDG_DATA_DIRS=/foo:/bar::/baz ?
> 
> It might be good to add a helper function that does the XDG_DATA_{DIRS,HOME}
> dance.  It would be reusable and allow for extensibility.
> 
> Cheers,
> 
> Daniel
> 
> >      [ -d $sourcedir ] || return 1
> >  
> >      local -a arguments
> > Email had 1 attachment:
> > + signature.asc
> >   1k (application/pgp-signature)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]