[Bug 744877] tools: bash completion for gst-inspect and gst-launch

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=744877

--- Comment #24 from Mathieu Duponchelle <mduponchelle1 at gmail.com> ---
Hey(In reply to David Röthlisberger from comment #21)
> I am the author of GStreamer's previous bash-completion implementation,
> which I wrote 2 years ago. Today I decided to finish the job and have it
> installed by "make install" (what can I say, my motivation ebbs and
> flows) only to find that you beat me to it. :-)
> 

Hey, hope you don't take it wrong, I got aware of the previous implementation
only after making my own patch :( On the bright side, it allowed me to start
again with another approach, and I'm quite happy with how it turned out (ie the
amount of shell script being reduced to a strict minimum, as the heavy lifting
is made by the helper tool, and having a base script which can be imported by
other libraries such as GES). I didn't intend my work to be an aggressive fork
:) 

> So here's my belated review of this patch.
> 
> 1. Can someone with commit access please delete the previous
>    implementation:
> 
>     * tools/gstreamer-completion
>     * tests/misc/test-gstreamer-completion.sh
> 

Done and pushed, as I said I didn't want my patch to overlook your work, so I
didn't remove them in the first place.

> 2. The new implementation has the following regressions when completing
>    gst-launch-1.0:
> 

Note that on the other hand, as I had left your work in place I don't consider
these as regressions ;)

>     * No completions offered for property values, for Enum and Boolean
>       properties.
>     * No completions offered for the arguments to "--gst-debug-level"
>       and "--gst-plugin-path".
>     * No trailing space is inserted after an unambiguous completion.
> 
> 3. Regressions when completing gst-inspect-1.0:
> 
>     * No completions are offered for plugin names, only for element
>       names (for example the previous implementation offered "udp",
>       "udpsink" and "udpsrc", but the new implementation only offers
>       the last 2).
> 

All these would be good to have, I didn't test the previous tool enough to see
it offered this.

> 4. I'd recommend namespacing all your bash functions with "_gst" because
>    these get sourced into the user's shell and could in theory conflict
>    with functions defined by other completion scripts or whatever.
> 

Done, commit attached for review.

> 5. $[...] is deprecated and "will be removed in upcoming versions of
> bash".
>    Use $((...)) instead.
> 

Done, thanks I wasn't aware of that (my bash skills are sadly lacking), commit
attached for review.

> Sorry if this sounded too negative. On the positive side, the fact that
> this gets installed at all makes it 100x more useful than the previous
> state of affairs. And maybe the new feature (where you only suggest
> compatible elements) makes all the regressions worthwhile.
> 
> If you have any future patches I'll be happy to help with the review,
> but please CC me as I don't monitor bugzilla.
> 
> Cheers,
> Dave.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.