Re: PATCH: search XDG_DATA_DIRS in _setxkbmap completion

[Maximilian Bosch <maximilian@mbosch.me>]

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

> Depends on how empty elements should be handled.  In some contexts an
> empty element means "look in the current working directory", and I
> wasn't sure how the XDG spec defines this case.  It's not a blocker but
> it would be nice to get this right while we're here.

I had a look at the XDG specification[1], unfortunately it doesn't
mention the case of empty search path entries. Do you know what's the
preferred default behavior?

Again, thanks a lot for your feedback, I've implemented your
suggestions.

Best regards,

Maximilian

[1] https://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html

diff --git a/Completion/X/Command/_setxkbmap b/Completion/X/Command/_setxkbmap
index f7310ecdd..486268ed7 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 -a searchdirs=(${XDG_DATA_HOME:-~/.local/share} ${(s.:.)XDG_DATA_DIRS:-/usr/lib:/usr/share:/usr/local/lib:/usr/local/share})
+    for dir in $searchdirs; do
+        fullname="$dir/X11/xkb"
+        if [ -d $fullname ] ; then
+           sourcedir=$fullname
            break
         fi
     done

On Tue, Sep 18, 2018 at 07:32:52PM +0000, Daniel Shahaf wrote:
> Maximilian Bosch wrote on Tue, 18 Sep 2018 21:05 +0200:
> > 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
> > 
> 
> You're welcome.
> 
> > 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 ]`?
> 
> Depends on how empty elements should be handled.  In some contexts an
> empty element means "look in the current working directory", and I
> wasn't sure how the XDG spec defines this case.  It's not a blocker but
> it would be nice to get this right while we're here.
>  
> > +++ 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}"
> 
> Two tweaks here:
> 
> 1. XDG_DATA_HOME should be listed before XDG_DATA_DIRS, according to
>    the XDG spec.
> 
> 2. I'd use an array, both because it's cleaner and in case
>    $XDG_DATA_HOME contains colons (not likely, but there's no
>    maintenance cost to supporting that).  Thus:
> 
>     local -a searchdirs=( ${XDG_DATA_HOME:-...} ${(s.:.)XDG_DATA_DIRS:-...} )
>     for dir in $searchdirs; do
> 
> > +    for dir in ${(s.:.)searchdirs}; do
> > +        fullname="$dir/X11/xkb"
> > +        if [ -d $fullname ] ; then
> > +           sourcedir=$fullname
> >             break
> >          fi
> >      done
> 
> Cheers,
> 
> Daniel

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