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 `castnow` completions #3744

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@moverest
Contributor

moverest commented Jan 18, 2017

Add castnow completions (https://github.com/xat/castnow).

@faho

This comment has been minimized.

Show comment
Hide comment
@faho

faho Jan 19, 2017

Member

Please add a comment at the top describing what this tool is, then this'll be good to merge.

Member

faho commented Jan 19, 2017

Please add a comment at the top describing what this tool is, then this'll be good to merge.

@faho faho added this to the next-minor milestone Jan 19, 2017

@@ -0,0 +1,30 @@
set -l __fish_castnow_keys "space\tToggle\ between\ play\ and\ pause m\tToggle\ mute t\tToggle\ subtitles up\tVolume\ up down\tVolume\ down left\tSeek\ backward right\tSeek\ forward n\tNext\ in\ playlist s\tStop\ playback quit\tQuit"

This comment has been minimized.

@krader1961

krader1961 Jan 19, 2017

Contributor

Did you test this? Because it shouldn't work given how you're using the var below. Backslash sequences like \t aren't interpreted inside quoted strings. Notice that other scripts, such as share/completions/mount.fish, which employ a similar pattern do not quote the string.

@krader1961

krader1961 Jan 19, 2017

Contributor

Did you test this? Because it shouldn't work given how you're using the var below. Backslash sequences like \t aren't interpreted inside quoted strings. Notice that other scripts, such as share/completions/mount.fish, which employ a similar pattern do not quote the string.

This comment has been minimized.

@moverest

moverest Jan 19, 2017

Contributor

castnow_command_completion

I sure did test it. I can remove the quotes if you prefer it without them.

@moverest

moverest Jan 19, 2017

Contributor

castnow_command_completion

I sure did test it. I can remove the quotes if you prefer it without them.

This comment has been minimized.

@moverest

moverest Jan 19, 2017

Contributor

I can see why this worked...

mount.fish uses single quotes in complete -a argument.

complete -c mount -x -s o --description 'Mount option' -a '(__fish_append , $__fish_mount_opts)'

So $__fish_mount_opts doesn't get replaced before being passed to complete.

Whereas I've used double quotes:

complete -c castnow -l command -d "Execute key command(s)" -x -a "(__fish_append , $__fish_castnow_keys)"

So here, $__fish_castnow_keys gets replaced with its value before being passed to complete.

From my understanding fish executes what's inside the -a argument when completing. So this way, it runs:

__fish_append , space\tToggle\ between\ play\ and\ pause m\tToggle\ mute...

Which works okay.

The advantage here is that I can define __fish_castnow_keys as a local variable (and not a global one as in mount.fish).

So what would you prefer?

@moverest

moverest Jan 19, 2017

Contributor

I can see why this worked...

mount.fish uses single quotes in complete -a argument.

complete -c mount -x -s o --description 'Mount option' -a '(__fish_append , $__fish_mount_opts)'

So $__fish_mount_opts doesn't get replaced before being passed to complete.

Whereas I've used double quotes:

complete -c castnow -l command -d "Execute key command(s)" -x -a "(__fish_append , $__fish_castnow_keys)"

So here, $__fish_castnow_keys gets replaced with its value before being passed to complete.

From my understanding fish executes what's inside the -a argument when completing. So this way, it runs:

__fish_append , space\tToggle\ between\ play\ and\ pause m\tToggle\ mute...

Which works okay.

The advantage here is that I can define __fish_castnow_keys as a local variable (and not a global one as in mount.fish).

So what would you prefer?

This comment has been minimized.

@krader1961

krader1961 Jan 20, 2017

Contributor

Bingo! Your analysis is correct. I missed the single versus double-quote difference. Thank you for thinking about this and responding to feedback in a thoughtful manner. I prefer the idiom in your change. I think the other, similar, patterns should match this one for consistency. But that's for another issue/pull-request.

@krader1961

krader1961 Jan 20, 2017

Contributor

Bingo! Your analysis is correct. I missed the single versus double-quote difference. Thank you for thinking about this and responding to feedback in a thoughtful manner. I prefer the idiom in your change. I think the other, similar, patterns should match this one for consistency. But that's for another issue/pull-request.

Show outdated Hide outdated share/completions/castnow.fish
@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jan 20, 2017

Contributor

Also, you were correct about my comment regarding the use of .* in the string match. I have a tendency to read that as a boolean grep where the trailing .* doesn't make any sense but does make sense in the context of string match in fish.

Contributor

krader1961 commented Jan 20, 2017

Also, you were correct about my comment regarding the use of .* in the string match. I have a tendency to read that as a boolean grep where the trailing .* doesn't make any sense but does make sense in the context of string match in fish.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Jan 20, 2017

Contributor

Squashed merged as commit 85212c5.

Contributor

krader1961 commented Jan 20, 2017

Squashed merged as commit 85212c5.

@krader1961 krader1961 closed this Jan 20, 2017

@moverest moverest deleted the moverest:castnow_completions branch Jan 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment