Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add gsettings completions #4001

Merged
merged 1 commit into from May 8, 2017
Merged

Add gsettings completions #4001

merged 1 commit into from May 8, 2017

Conversation

@sbstnk
Copy link
Contributor

@sbstnk sbstnk commented May 1, 2017

Description

This adds completions for the 'gsettings' utility shipped with glib. The suggestions for schemas/keys follow the custom schemas directory specified using --schemadir. This is useful for configuring gnome-shell extensions that ship their own schemas and done in a similar way as in the bash completions.

set -l cmd (commandline -poc)
set -e cmd[1]

if [ (count $cmd) -ge 2 ]; and string match -q -- '--schemadir' $cmd[1]
Copy link
Contributor

@krader1961 krader1961 May 1, 2017

FWIW, This isn't a deal breaker but we prefer that the semi-magic [...] syntax be avoided. Instead use this style:

if test (count $cmd) -ge 2
and string match ...

Even better since this avoids the test and count:

if set -q cmd[3]
and string match ....

set offset 2
end

if [ (count $cmd) -lt (math 1 + $offset) ]
Copy link
Contributor

@krader1961 krader1961 May 1, 2017

You're invoking math with the same arguments more than once. This screams out to be refactored to do that calculation only once, assigning the result to a local variable, then using that var.

Copy link
Contributor Author

@sbstnk sbstnk May 1, 2017

I solved this differently now by removing the first the first two entries from $cmd if '--schemadir' is used. No more 'math' or offsets required.

return 1
end

set -l valid_commands get monitor writable range describe set reset reset-recursively list-schemas list-relocatable-schemas list-keys list-children list-recursively help --version
Copy link
Contributor

@krader1961 krader1961 May 1, 2017

Including --version as a valid subcommand seems wrong and unnecessary.

Copy link
Contributor Author

@sbstnk sbstnk May 1, 2017

It is mostly to prevent further suggestions after 'gsettings --version', because they wouldn't work anyway. From 'gsettings help':

Usage:
gsettings --version
gsettings [--schemadir SCHEMADIR] COMMAND [ARGS?]

set -l key $cmd[(math 3 + $offset)]

if [ (count $cmd) -lt (math 4 + $offset) ]
set -l key_type (gsettings $schemadir range $schema $key ^/dev/null | head -n 1)
Copy link
Contributor

@krader1961 krader1961 May 1, 2017

How much output is this gsettings invocation likely to output? if it is less than a thousand lines it would be better to eliminate the | head -n 1 and instead do (gsettings ...)[1] to select just the first line.

Copy link
Contributor Author

@sbstnk sbstnk May 1, 2017

Thanks, that's a good suggestion. Usually there is just one line, except when there is an enum, in that case there is a (short) list of all possible values. I guess I could replace 'tail' in the enum case as well.

@sbstnk
Copy link
Contributor Author

@sbstnk sbstnk commented May 1, 2017

I've updated everything according to the comments and also fixed another issue I noticed. The 'echo' was not enough to expand '~' if it was used in the '--schemadir' option, I needed to add an 'eval' as well.

set -l cmd (commandline -poc)
set -e cmd[1]

if set -q cmd[2]; and string match -q -- '--schemadir' $cmd[1]
Copy link
Contributor

@krader1961 krader1961 May 2, 2017

Doesn't this long flag support this form: --schemadir=/some/dir? If it does then this block won't work correctly. If it does not then it's better to do test $cmd[1] = --schemadir.

Copy link
Contributor Author

@sbstnk sbstnk May 2, 2017

No, it only supports the space separated form.

$ gsettings --schemadir=/ list-schemas
Unknown command --schemadir=/

Copy link
Member

@faho faho May 3, 2017

Are you sure that "--schemadir" can only appear before the command? Also, is it the only option that still allows commands?

echo false
case 'enum'
for line in $range[2..-1]
echo $line | sed -e 's/\'//g'
Copy link
Contributor

@krader1961 krader1961 May 2, 2017

We no longer need to rely on sed for such string manipulations. Replace it with string replace --all \' '' here and below to strip single quotes.

return 0
end

set -l key $cmd[3]
Copy link
Contributor

@krader1961 krader1961 May 2, 2017

This statement should be move after the next if test block since it isn't used if that block returns from the function.

return 0
end

set -l key $cmd[3]
Copy link
Contributor

@krader1961 krader1961 May 2, 2017

In fact it should be put inside the if not set -q cmd[4] block since that is the only place the var is used.

@sbstnk sbstnk force-pushed the master branch 2 times, most recently from a07f7fd to dc47a46 May 2, 2017

if set -q cmd[2]; and string match -q -- '--schemadir' $cmd[1]
# use eval to ensure the expansion of ~
set schemadir --schemadir (eval echo $cmd[2])
Copy link
Member

@faho faho May 3, 2017

Please don't use eval. The most egregious issue there is that command substitutions will be executed, so e.g. gsettings --schemadir (rm -rf ~/*) will remove your home directory. While that example is silly, that behavior is highly unexpected and potentially dangerous.

Instead, use something like

    # Expand "~".
    if string match -q '~*' -- $comp
        set -l user (string replace -r '/.*$' '' -- $comp)
        # Tilde-expansion happens after variable expansion.
        set comp (string replace -r '~[^/]*/' ~user/$comp -- $comp)
    end

(which I will add to __fish_complete_directories in a moment since that fails in this case currently)

Copy link
Member

@faho faho May 3, 2017

Note that it's okay to fail in case of command substitutions, since the only way to get to what it expands to is to execute it.

We might want to add some way to expand more complicated variable expansions and such, though.

Copy link
Contributor Author

@sbstnk sbstnk May 3, 2017

Are you sure that "--schemadir" can only appear before the command? Also, is it the only option that still allows commands?

Yes. You can see here that it can only occur at this position and is the only option:
https://git.gnome.org/browse/glib/tree/gio/gsettings-tool.c#n717

The upstream bash completion handles it similarly:
https://git.gnome.org/browse/glib/tree/gio/completion/gsettings#n10

Please don't use eval. [...]

Good point!

Instead, use something like...

Could something like that be added as a separate "expand_path" function? Seems wrong to have copies of that spread across different completions. This could later also be extended to expand variables as well if you decide to do so.

Copy link
Member

@faho faho May 3, 2017

After further consideration, just leave the expansion bit out for now. This is lacking across the board, and should not be fixed in a single completion script. It's also more than just tilde, it's also variables and braces.

echo true
echo false
case 'enum'
for line in $range[2..-1]
Copy link
Member

@faho faho May 3, 2017

This for-loop is unnecessary since string can also take arguments.

string replace --all \' '' -- $range[2..-1]

(also, since you're not using $range[1] anywhere, it'd be better to set -e range[1] before and drop this slice - we currently error for invalid slices)

set -l cmd (commandline -poc)
set -e cmd[1]

if set -q cmd[2]; and string match -q -- '--schemadir' $cmd[1]
Copy link
Member

@faho faho May 3, 2017

Are you sure that "--schemadir" can only appear before the command? Also, is it the only option that still allows commands?

@sbstnk
Copy link
Contributor Author

@sbstnk sbstnk commented May 7, 2017

I've changed everything to use 'string' now instead of loops+echo. I've also noticed that the single quotes are required in the case of complex settings (e.g. org.gnome.settings-daemon.plugins.xsettings overrides), so I'm not removing them anymore in the enum or * case. The boolean case on the other hand does not work when using single quotes, so I did not add them there. And finally I've removed the path expansion code and added a TODO comment about supporting this properly.

@faho faho merged commit 6620b9e into fish-shell:master May 8, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@faho faho added this to the 2.6.0 milestone May 8, 2017
@faho
Copy link
Member

@faho faho commented May 8, 2017

Merged, thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants