PATCH: search XDG_DATA_DIRS in _setxkbmap completion

PATCH: search XDG_DATA_DIRS in _setxkbmap completion

[Maximilian Bosch]

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

Hi,

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

Maximilian

[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
+
     [ -d $sourcedir ] || return 1
 
     local -a arguments

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

Re: PATCH: search XDG_DATA_DIRS in _setxkbmap completion

[Daniel Shahaf]

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)

Re: PATCH: search XDG_DATA_DIRS in _setxkbmap completion

[Maximilian Bosch]

[-- 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 --]

Re: PATCH: search XDG_DATA_DIRS in _setxkbmap completion

[Daniel Shahaf]

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

Re: PATCH: search XDG_DATA_DIRS in _setxkbmap completion

[Maximilian Bosch]

[-- 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 --]

Re: PATCH: search XDG_DATA_DIRS in _setxkbmap completion

[Daniel Shahaf]

Maximilian Bosch wrote on Wed, 19 Sep 2018 22:36 +0200:
> > 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?
> 

No.  I just committed the patch as-is: it's an improvement, and if we
find that empty elements need special handling we can add it in a
subsequent patch.

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

You're welcome, and thanks for the iterations.

Cheers,

Daniel

Re: PATCH: search XDG_DATA_DIRS in _setxkbmap completion

[TS]

Daniel Shahaf schrieb/wrote:
>>> How should empty array elements in XDG_DATA_DIRS be handled?  E.g.,
>>> XDG_DATA_DIRS=/foo:/bar::/baz ?
>>
>> how about s.th. around the lines of:
>>
>> tosh ~ % searchdirs=(${^~searchdirs}(N/))
> 
> Thanks for the suggestion.  To be clear, I was mainly asking how the XDG
> basedirs spec wants empty elements to be treated: as invalid values, or
> as aliases to the current working directory, or to the root directory, etc.

https://standards.freedesktop.org/basedir-spec/basedir-spec-latest.html
nothing really clear about that.



kind regards,

     Thilo

Re: PATCH: search XDG_DATA_DIRS in _setxkbmap completion

[Daniel Shahaf]

> > How should empty array elements in XDG_DATA_DIRS be handled?  E.g.,
> > XDG_DATA_DIRS=/foo:/bar::/baz ?
> 
> how about s.th. around the lines of:
> 
> tosh ~ % searchdirs=(${^~searchdirs}(N/))

Thanks for the suggestion.  To be clear, I was mainly asking how the XDG
basedirs spec wants empty elements to be treated: as invalid values, or
as aliases to the current working directory, or to the root directory, etc.

As I said in my other reply, if we can't easily find the answer I'd be
happy to commit the patch with its current behaviour.

Cheers,

Daniel

Re: PATCH: search XDG_DATA_DIRS in _setxkbmap completion

[TS]

Hello

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

how about s.th. around the lines of:

tosh ~ %
XDG_DATA_DIRS=/usr/lib/X11/xkb:/usr/share/X11/xkb:/usr/local/lib/X11/xkb:/usr/local/share/X11/xkb
tosh ~ % typeset -aU searchdirs
tosh ~ % searchdirs=(${(s.:.)XDG_DATA_DIRS}
{/usr/lib,/usr/share,/usr/local/lib,/usr/local/share}/X11/xkb ${XDG_DATA_HOME}
~/.local/share/X11/xkb)
tosh ~ % ev searchdirs
array-unique  searchdirs:
1='/usr/lib/X11/xkb'
2='/usr/share/X11/xkb'
3='/usr/local/lib/X11/xkb'
4='/usr/local/share/X11/xkb'
5='/home/heinb/.local/share/X11/xkb'
tosh ~ % searchdirs+=('')      << just for example
tosh ~ % ev searchdirs
array-unique  searchdirs:
1='/usr/lib/X11/xkb'
2='/usr/share/X11/xkb'
3='/usr/local/lib/X11/xkb'
4='/usr/local/share/X11/xkb'
5='/home/heinb/.local/share/X11/xkb'
6=''                          <<------ empty
tosh ~ % searchdirs=(${^~searchdirs}(N/))
tosh ~ % ev searchdirs
array-unique  searchdirs:
1='/usr/share/X11/xkb'




kind regards,

     Thilo