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 completions for mkvextract #3492

Merged
merged 4 commits into from Nov 18, 2016
Merged

Add completions for mkvextract #3492

merged 4 commits into from Nov 18, 2016

Conversation

@occivink
Copy link
Contributor

occivink commented Oct 25, 2016

Mostly useful for the dynamic tracks and attachments completions.

Much like mkvextract, mkvmerge is part of mkvtoolnix so it should be okay to rely on it for completions.

I only did mkvextract because the others are much more complicated to provide useful completions for, but I might try to do them in the future.

@occivink
Copy link
Contributor Author

occivink commented Oct 26, 2016

Is that just a spurious error? I can't make sense of the message

line 2165: autosuggest_suggest_special() failed for command cd ~haha
Error: Test failed on line 2045: !comps.empty()
@faho
Copy link
Member

faho commented Oct 26, 2016

@occivink: Yes, we've been having some trouble with travis lately. That test has nothing to do with your changes. Just ignore it for now.

complete -f -c mkvextract -n "test (count (commandline -opc)) -lt 2" -s V -l "version" -d "Show version information"
complete -f -c mkvextract -n "test (count (commandline -opc)) -lt 2" -s h -l "help" -d "Show help"
# commands
complete -f -c mkvextract -n "test (count (commandline -opc)) -lt 2" -a "tracks" -d "Extract tracks to external files"

This comment has been minimized.

Copy link
@faho

faho Oct 26, 2016

Member

Most of the time, commands support options also before subcommands, i.e. something like mkvextract --parse-fully tags would also work. If that is the case here, just counting the number of tokens would break.

Can you confirm if it is?

This comment has been minimized.

Copy link
@occivink

occivink Oct 26, 2016

Author Contributor

I can confirm it doesn't accept options before subcommands.

@@ -0,0 +1,47 @@
# Sample output of 'mkvmerge -i file.mkv'

This comment has been minimized.

Copy link
@faho

faho Oct 26, 2016

Member

Thank you for this.

This comment has been minimized.

Copy link
@occivink

occivink Oct 26, 2016

Author Contributor

Well, thank you for working on fish.

# Chapters: 7 entires

function __fish_print_mkvextract_attachments
mkvmerge -i $argv | string match 'Attachment ID*' | string replace -r '.*?(\d+).*? type \'(.*?)\'.*?file name \'(.*?)\'' '$1:\t$3 ($2)'

This comment has been minimized.

Copy link
@faho

faho Oct 26, 2016

Member

If I'm reading this correctly, the completion itself has a trailing ":". Is that correct? Is "1:" really the expected argument?

This comment has been minimized.

Copy link
@occivink

occivink Oct 26, 2016

Author Contributor

Yes, how it works is you actually specify
mkvextract tracks file.mkv 2:audio.aac 3:subtitles.ass
And the files audio.aac and subtitles.ass will be created. The name of the output file is up to the user, the completion only proposes available streams.

It's actually rather convenient that it doesn't automatically add a space if the completion ends in : but that's not configurable, right? I would need that with completions ending in = to make useful dynamic ones for mkvpropedit

complete -f -c mkvextract -n "test (count (commandline -opc)) -ge 3" -r -l "output-charset" -d "Outputs messages in specified charset"
complete -f -c mkvextract -n "test (count (commandline -opc)) -ge 3" -r -s r -l "redirect-output" -d "Redirect all messages into a file"
# command-specific options
complete -f -c mkvextract -n "test (count (commandline -opc)) -ge 3; and contains -- (commandline -opc)[2] tracks" -r -s c -d "Convert text subtitles to a charset"

This comment has been minimized.

Copy link
@faho

faho Oct 26, 2016

Member

We'd usually write this test as __fish_seen_subcommand_from tracks - that will also skip all options.

This comment has been minimized.

Copy link
@occivink

occivink Oct 26, 2016

Author Contributor

I'll fix that

@occivink
Copy link
Contributor Author

occivink commented Oct 26, 2016

Somewhat offtopic, but do you know if ffmpeg completions have ever been considered? It's a popular application so I would expect so, but then it has a rather unconventional interface so it might not be possible. Because auto-completing streams from the input file would be pretty nice.

@occivink
Copy link
Contributor Author

occivink commented Oct 30, 2016

Okay I realized that generic options can be specified before the mkv file (contrary to what the help says), which breaks the assumption made by:
(__fish_print_mkvextract_tracks (commandline -opc)[3])
What would be a fishy way to iterate through arguments and check if they're actually a file?

Tangentially, do you know of some non-trivial completions that are well implemented and that I could draw inspiration from?

@faho
Copy link
Member

faho commented Oct 31, 2016

What would be a fishy way to iterate through arguments and check if they're actually a file?

You don't want "actually a file". You want "a non-option after the command".

Tangentially, do you know of some non-trivial completions that are well implemented and that I could draw inspiration from?

Our biggest and possibly best completion script is the one for git, and it does this:

function __fish_git_needs_command
    set cmd (commandline -opc)
    if [ (count $cmd) -eq 1 ]
        return 0
    else
        set -l skip_next 1
        # Skip first word because it's "git" or a wrapper
        for c in $cmd[2..-1]
            test $skip_next -eq 0
            and set skip_next 1
            and continue
            # git can only take a few options before a command, these are the ones mentioned in the "git" man page
            # e.g. `git --follow log` is wrong, `git --help log` is okay (and `git --help log $branch` is superfluous but works)
            # In case any other option is used before a command, we'll fail, but that's okay since it's invalid anyway
            switch $c
                # General options that can still take a command
                case "--help" "-p" "--paginate" "--no-pager" "--bare" "--no-replace-objects" --{literal,glob,noglob,icase}-pathspecs --{exec-path,git-dir,work-tree,namespace}"=*"
                    continue
                    # General options with an argument we need to skip. The option=value versions have already been handled above
                case --{exec-path,git-dir,work-tree,namespace}
                    set skip_next 0
                    continue
                    # General options that cause git to do something and exit - these behave like commands and everything after them is ignored
                case "--version" --{html,man,info}-path
                    return 1
                    # We assume that any other token that's not an argument to a general option is a command
                case "*"
                    echo $c
                    return 1
            end
        end
        return 0
    end
    return 1
end

to determine if a command has been given. As you can see, this also handles options that take arguments (--work-tree /some/path).

@occivink
Copy link
Contributor Author

occivink commented Nov 6, 2016

Okay that should fix all edge cases I was aware of.
Other question, is there a way to get both the output and the return code of a command into two variables?

For example, I do

set -l matroska (__fish_mkvextract_find_matroska_in_args)
if test -n '$matroska'

but it'd be nicer to just use the return code of the command. I couldn't find how to do that though.

@faho
Copy link
Member

faho commented Nov 6, 2016

Other question, is there a way to get both the output and the return code of a command into two variables?

set won't alter $status, so you can just use that.

set -l matroska (__fish_mkvextract_find_matroska_in_args)
if test $status -eq 0

or even

if set -l matroska (__fish_mkvextract_find_matroska_in_args)

(of course, if you'd like to use it later, you'll need to save it to another variable)

function __fish_print_mkvextract_attachments
mkvmerge -i $argv | string match 'Attachment ID*' | string replace -r '.*?(\d+).*? type \'(.*?)\'.*?file name \'(.*?)\'' '$1:\t$3 ($2)'
function __fish_mkvextract_find_matroska_in_args
for c in (commandline -opc)[2..-1]

This comment has been minimized.

Copy link
@faho

faho Nov 6, 2016

Member

Of note is that this will fail if there is no second token (because of our annoying habit of failing invalid array indices).

This doesn't occur here because this is only ever called if there is already a subcommand, so there's no pressing need to change it.

@occivink
Copy link
Contributor Author

occivink commented Nov 6, 2016

Yet another question, it seems like paths starting with ~ (or containing a variable) are not working properly because they're not expanded by the shell. How can I handle that?

@faho
Copy link
Member

faho commented Nov 6, 2016

@occivink: Don't. We need a helper function to do that, because a few completions currently try via eval (btw, please don't do that).

@occivink
Copy link
Contributor Author

occivink commented Nov 6, 2016

Okay thanks, I thought that there might have been a function to do that.

@@ -8,7 +8,11 @@
# Chapters: 7 entires

function __fish_mkvextract_find_matroska_in_args
for c in (commandline -opc)[2..-1]
set -l cmd (commandline -opc)
if test (count $cmd) -lt 3

This comment has been minimized.

Copy link
@faho

faho Nov 10, 2016

Member

if not set -q cmd[3]

mkvmerge -i $matroska | string match 'Attachment ID*' | string replace -r '.*?(\d+).*? type \'(.*?)\'.*?file name \'(.*?)\'' '$1:\t$3 ($2)'
if set -l matroska (__fish_mkvextract_find_matroska_in_args)
if set -l info (mkvmerge -i $matroska)
string match 'Attachment ID*' $info | string replace -r '.*?(\d+).*? type \'(.*?)\'.*?file name \'(.*?)\'' '$1:\t$3 ($2)'

This comment has been minimized.

Copy link
@faho

faho Nov 10, 2016

Member

Ideally, you'd guard the "$info" from being interpreted as options: string match 'Attachment ID*' -- $info. Otherwise if a line is e.g. -r, string will go into regex mode.

mkvmerge -i $matroska | string match 'Track ID*' | string replace -r '.*?(\d+): (.*)' '$1:\t$2'
if set -l matroska (__fish_mkvextract_find_matroska_in_args)
if set -l info (mkvmerge -i $matroska)
string match 'Track ID*' $info | string replace -r '.*?(\d+): (.*)' '$1:\t$2'

This comment has been minimized.

Copy link
@faho

faho Nov 10, 2016

Member

Same.

@occivink
Copy link
Contributor Author

occivink commented Nov 10, 2016

Happy to comply, thanks for the tips.

@faho
faho approved these changes Nov 17, 2016
@faho faho added this to the fish 2.5.0 milestone Nov 17, 2016
@faho
Copy link
Member

faho commented Nov 18, 2016

@occivink: Are you done here?

@occivink
Copy link
Contributor Author

occivink commented Nov 18, 2016

Yes I am.

@faho faho merged commit 8423345 into fish-shell:master Nov 18, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@faho
Copy link
Member

faho commented Nov 18, 2016

Merged, thanks!

@occivink
Copy link
Contributor Author

occivink commented Nov 18, 2016

Thanks for all the feedback.

@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

2 participants
You can’t perform that action at this time.