Re: Small patch for FFmpeg completion function

[Haakon Riiser <haakon.riiser@xxxxxxxxxx>]

On Sat, 30 Apr 2011 21:59:44 +0100, Peter Stephenson 
<p.w.stephenson@xxxxxxxxxxxx> wrote:
> On Thu, 28 Apr 2011 16:46:19 +0200
> Haakon Riiser <haakon.riiser@xxxxxxxxxx> wrote:
> >  Decided to clean it up further. Obviously it is better to avoid 
> > having
> >  Perl as a dependency, so the attached version does everything using 
> > zsh
> >  scripting. It also uses _call_program and $words[1] instead of 
> > "ffmpeg"
> >  directly.
>
> Thanks, committed this without doing more than a cursory scan of it 
> ---
> no doubt someone will say if they're having problems.

Thank you for committing the patch so soon. Unfortunately, I found a 
couple of more things to improve. :)

First of all, there was a bug in the pixel format completion - it 
included an invalid match. This bug was not introduced by my previous 
patch, it has been there for a long time.

The second thing I changed was not a bug, but probably something that 
was poor form. Previously, I did this:

local BOLD=$'\e[1m'
local NORM=$'\e[m'
compadd -X "${BOLD}description{NORM}" -q -S '' -a array

Here, "array" contains a list of keywords to be completed (e.g., a list 
of codec names), and "description" contains a description headline to be 
displayed at the top, above the list of keywords. The $BOLD and $NORM 
escape codes were used to make it visually identical to description 
headers in other functions (e.g., _files). This method worked, but I 
never liked hard-coding ANSI escape codes directly. Now, this is 
replaced by:

_wanted ffmpeg-XXX expl 'description' compadd -a array

Seems to work just as well, and I don't need the ANSI escapes. 
Hopefully, this is the correct way to insert matches directly. If you 
notice something else that is poor form, let me know and I will fix it.

PS: I have also recently written a completion function for nmcli (the 
command-line interface to NetworkManager, which is used in a lot of 
Linux distros). It was a bit tricky to get right, because a deep nesting 
of subcommands and non-standard command line parsing. I think I got it 
right, but I haven't posted it here yet. Thought I'd get the _ffmpeg 
improvements out of the way first, so that I don't make the same 
mistakes in _nmcli.

--
 Haakon

Index: _ffmpeg
===================================================================
RCS file: /cvsroot/zsh/zsh/Completion/Unix/Command/_ffmpeg,v
retrieving revision 1.3
diff -u -1 -0 -r1.3 _ffmpeg
--- _ffmpeg	30 Apr 2011 20:58:49 -0000	1.3
+++ _ffmpeg	30 Apr 2011 22:46:59 -0000
@@ -1,64 +1,56 @@
 #compdef ffmpeg
 
-local context state line
+local context state line expl
 typeset -A opt_args
 
-local -a _ffmpeg_argspecs
-
-local BOLD=$'\e[1m'
-local NORM=$'\e[m'
-
-(( $+functions[_ffmpeg_compadd] )) || _ffmpeg_compadd() {
-    compadd -X "${BOLD}$1${NORM}" -q -S "$3" -a $2
-}
-
 (( $+functions[_ffmpeg_presets] )) || _ffmpeg_presets() {
     local presets
     presets=(~/.ffmpeg/*.ffpreset(:t:r) "$FFMPEG_DATADIR"/*.ffpreset(:t:r))
-    _ffmpeg_compadd 'select preset' presets ''
+    _wanted ffmpeg-presets expl 'select preset' compadd -a presets
 }
 
 (( $+functions[_ffmpeg_acodecs] )) || _ffmpeg_acodecs() {
     local acodecs
     acodecs=(copy ${${(M)${(f)"$(_call_program audio-codecs $words[1] -codecs 2>/dev/null)"}:#[[:space:]][D[:space:]][E[:space:]]A[S[:space:]][D[:space:]][T[:space:]][[:space:]][^[:space:]]##*}//(#b)????????([^[:space:]]##)*/$match[1]})
-    _ffmpeg_compadd 'force audio codec (''copy'' to copy stream)' acodecs ''
+    _wanted ffmpeg-audio-codecs expl 'force audio codec (''copy'' to copy stream)' compadd -a acodecs
 }
 
 (( $+functions[_ffmpeg_vcodecs] )) || _ffmpeg_vcodecs() {
     local vcodecs
     vcodecs=(copy ${${(M)${(f)"$(_call_program video-codecs $words[1] -codecs 2>/dev/null)"}:#[[:space:]][D[:space:]][E[:space:]]V[S[:space:]][D[:space:]][T[:space:]][[:space:]][^[:space:]]##*}//(#b)????????([^[:space:]]##)*/$match[1]})
-    _ffmpeg_compadd 'force video codec (''copy'' to copy stream)' vcodecs ''
+    _wanted ffmpeg-video-codecs expl 'force video codec (''copy'' to copy stream)' compadd -a vcodecs
 }
 
 (( $+functions[_ffmpeg_formats] )) || _ffmpeg_formats() {
     local formats
     formats=(${(ou)${=${(s:,:)${${(M)${(f)"$(_call_program formats $words[1] -formats 2>/dev/null)"}:#[[:space:]][D[:space:]][E[:space:]][[:space:]][^[:space:]]##*}//(#b)????([^[:space:]]##)*/$match[1]}}}})
-    _ffmpeg_compadd 'force format' formats ''
+    _wanted ffmpeg-formats expl 'force format' compadd -a formats
 }
 
 (( $+functions[_ffmpeg_list_pix_fmts] )) || _ffmpeg_list_pix_fmts() {
-    print -l ${${(M)${(f)"$(_call_program pix-fmts $words[1] -pix_fmts 2>/dev/null)"}:#[I.]*}//(#b)??????([^[:space:]]##)*/$match[1]}
+    echo - ${${${(M)${(f)"$(_call_program formats $words[1] -pix_fmts 2>/dev/null)"}:#[I.][O.][H.][P.][B.] [^=[:space:]]*}#* }%% *}
 }
 
 (( $+functions[_ffmpeg_pix_fmts] )) || _ffmpeg_pix_fmts() {
     local pix_fmts
     pix_fmts=($(_ffmpeg_list_pix_fmts))
-    _ffmpeg_compadd 'set pixel format' pix_fmts ''
+    _wanted ffmpeg-pix-fmts expl 'set pixel format' compadd -a pix_fmts
 }
 
 (( $+functions[_ffmpeg_bsfs] )) || _ffmpeg_bsfs() {
     local bsfs
     bsfs=(${${(f)"$(_call_program bsfs $words[1] -bsfs 2>/dev/null)"}:#*:})
-    _ffmpeg_compadd 'set bitstream filter' bsfs ''
+    _wanted ffmpeg-bsfs expl 'set bitstream filter' compadd -a bsfs
 }
 
+local -a _ffmpeg_argspecs
 {
     local lastopt
     local lastopt_description
     local lastopt_takesargs
     local -a lastopt_values
 
     _call_program options $words[1] -h 2>/dev/null | while IFS=$'\n' read -r; do
         if [[ $REPLY == -* ]]; then
             if [[ -n $lastopt ]]; then
                 if (( lastopt_takesargs )); then