Re: PATCH: search XDG_DATA_DIRS in _setxkbmap completion

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

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)