* Re: PATCH: search XDG_DATA_DIRS in _setxkbmap completion
@ 2018-09-19 4:53 TS
2018-09-19 14:29 ` Daniel Shahaf
0 siblings, 1 reply; 9+ messages in thread
From: TS @ 2018-09-19 4:53 UTC (permalink / raw)
To: Zsh hackers list
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PATCH: search XDG_DATA_DIRS in _setxkbmap completion
2018-09-19 4:53 PATCH: search XDG_DATA_DIRS in _setxkbmap completion TS
@ 2018-09-19 14:29 ` Daniel Shahaf
2018-09-19 19:53 ` TS
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Shahaf @ 2018-09-19 14:29 UTC (permalink / raw)
To: TS, Zsh hackers list
> > 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PATCH: search XDG_DATA_DIRS in _setxkbmap completion
2018-09-19 14:29 ` Daniel Shahaf
@ 2018-09-19 19:53 ` TS
0 siblings, 0 replies; 9+ messages in thread
From: TS @ 2018-09-19 19:53 UTC (permalink / raw)
To: Daniel Shahaf, Zsh hackers list
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PATCH: search XDG_DATA_DIRS in _setxkbmap completion
2018-09-19 20:36 ` Maximilian Bosch
@ 2018-09-19 21:30 ` Daniel Shahaf
0 siblings, 0 replies; 9+ messages in thread
From: Daniel Shahaf @ 2018-09-19 21:30 UTC (permalink / raw)
To: Maximilian Bosch, zsh-workers
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PATCH: search XDG_DATA_DIRS in _setxkbmap completion
2018-09-18 19:32 ` Daniel Shahaf
@ 2018-09-19 20:36 ` Maximilian Bosch
2018-09-19 21:30 ` Daniel Shahaf
0 siblings, 1 reply; 9+ messages in thread
From: Maximilian Bosch @ 2018-09-19 20:36 UTC (permalink / raw)
To: zsh-workers
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PATCH: search XDG_DATA_DIRS in _setxkbmap completion
2018-09-18 19:05 ` Maximilian Bosch
@ 2018-09-18 19:32 ` Daniel Shahaf
2018-09-19 20:36 ` Maximilian Bosch
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Shahaf @ 2018-09-18 19:32 UTC (permalink / raw)
To: Maximilian Bosch, zsh-workers
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PATCH: search XDG_DATA_DIRS in _setxkbmap completion
2018-09-18 16:10 ` Daniel Shahaf
@ 2018-09-18 19:05 ` Maximilian Bosch
2018-09-18 19:32 ` Daniel Shahaf
0 siblings, 1 reply; 9+ messages in thread
From: Maximilian Bosch @ 2018-09-18 19:05 UTC (permalink / raw)
To: zsh-workers
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: PATCH: search XDG_DATA_DIRS in _setxkbmap completion
2018-09-18 12:49 Maximilian Bosch
@ 2018-09-18 16:10 ` Daniel Shahaf
2018-09-18 19:05 ` Maximilian Bosch
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Shahaf @ 2018-09-18 16:10 UTC (permalink / raw)
To: Maximilian Bosch, zsh-workers
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)
^ permalink raw reply [flat|nested] 9+ messages in thread
* PATCH: search XDG_DATA_DIRS in _setxkbmap completion
@ 2018-09-18 12:49 Maximilian Bosch
2018-09-18 16:10 ` Daniel Shahaf
0 siblings, 1 reply; 9+ messages in thread
From: Maximilian Bosch @ 2018-09-18 12:49 UTC (permalink / raw)
To: zsh-workers
[-- 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 --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-09-19 21:31 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-19 4:53 PATCH: search XDG_DATA_DIRS in _setxkbmap completion TS
2018-09-19 14:29 ` Daniel Shahaf
2018-09-19 19:53 ` TS
-- strict thread matches above, loose matches on Subject: below --
2018-09-18 12:49 Maximilian Bosch
2018-09-18 16:10 ` Daniel Shahaf
2018-09-18 19:05 ` Maximilian Bosch
2018-09-18 19:32 ` Daniel Shahaf
2018-09-19 20:36 ` Maximilian Bosch
2018-09-19 21:30 ` Daniel Shahaf
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/zsh/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).