GNOME Bugzilla – Bug 744877
tools: bash completion for gst-inspect and gst-launch
Last modified: 2015-03-24 18:14:12 UTC
I think we'll quickly get used to this :) Should work with zsh, following : http://stackoverflow.com/questions/3249432/i-have-a-bash-tab-completion-script-is-there-a-simple-way-to-use-it-from-zsh
Created attachment 297465 [details] [review] configure: check for bash completion.
Created attachment 297466 [details] [review] tools: create a tool to help completion. Could arguably be installed in gst-inspect, not sure that makes sense. Parsing the output of gst-inspect in its current state is not fast enough and too complicated for fast and accurate context-aware completion.
Created attachment 297467 [details] [review] gst-inspect: enable bash completion + Start defining a common gst completion file.
Created attachment 297468 [details] [review] gst-launch: enable bash completion + Complete -- options + Complete element names + Complete compatible element names after a '!' + Complete property names + Is very nice
Created attachment 297474 [details] [review] gst-inspect: enable bash completion + Start defining a common gst completion file.
Created attachment 297475 [details] [review] gst-launch: enable bash completion + Complete -- options + Complete element names + Complete compatible element names after a '!' + Complete property names + Is very nice
How does this relate to the existing bash completion stuff, and bug #740108 ?
Damn I wasn't aware of that :( My solution is better though, simpler and has more features (completion with compatible elements only), also the existing thing outputs -- options after "!" which can't be correct.
And the configure / Makefile part is as olivier suggests in bug #740108 with my patches
Created attachment 297682 [details] [review] bash-completion: Implement in a different way. + Gets installed + Uses a helper tool, gst-completion-helper, installed in bash-completions/helpers. + Adds a common script that other tools can source.
Review of attachment 297682 [details] [review]: Shouldn't the gst-completion-helper.c perhaps go into libs/gst/helpers/ instead? I don't think it belongs in data/ ::: configure.ac @@ -137,0 +137,6 @@ +dnl check for bash completion +AC_ARG_WITH([bash-completion-dir], + AS_HELP_STRING([--with-bash-completion-dir[=PATH]], ... 3 more ... --with-bash-completion-dir=PATH (and "install in this directory2) and then the values are yes/no? @@ -137,0 +137,12 @@ +dnl check for bash completion +AC_ARG_WITH([bash-completion-dir], + AS_HELP_STRING([--with-bash-completion-dir[=PATH]], ... 9 more ... I'm not sure this will always work if you use BASH_COMPLETION for a second time, it might just skip it because it's got cached that it ran already.
(In reply to Tim-Philipp Müller from comment #11) > Review of attachment 297682 [details] [review] [review]: > > Shouldn't the gst-completion-helper.c perhaps go into libs/gst/helpers/ > instead? I don't think it belongs in data/ > Sure, should be doable > ::: configure.ac > @@ -137,0 +137,6 @@ > +dnl check for bash completion > +AC_ARG_WITH([bash-completion-dir], > + AS_HELP_STRING([--with-bash-completion-dir[=PATH]], > ... 3 more ... > > --with-bash-completion-dir=PATH (and "install in this directory2) and then > the values are yes/no? > copy paste sorry will fix > @@ -137,0 +137,12 @@ > +dnl check for bash completion > +AC_ARG_WITH([bash-completion-dir], > + AS_HELP_STRING([--with-bash-completion-dir[=PATH]], > ... 9 more ... > > I'm not sure this will always work if you use BASH_COMPLETION for a second > time, it might just skip it because it's got cached that it ran already. Not sure what you mean here ?
> --with-bash-completion-dir=PATH (and "install in this directory2) and then > the values are yes/no? Actually it is correct, it allows to disable bash completion (no), let it use the default directories (yes) or specify a custom directory (/custom/path). There was an issue though in that BASH_COMPLETION_DIR should be "$with_bash_completion_dir/completions" and same for HELPERS_DIR, fixed in the commit I'll push now.
Yeah, just go ahead and push.
commit 46e7baff745d35a2079d714ffb4284e85b74d1f7 Author: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com> Date: Fri Feb 20 22:04:22 2015 +0100 bash-completion: Implement in a different way. + Gets installed + Uses a helper tool, gst-completion-helper, installed in bash-completions/helpers. + Adds a common script that other tools can source. https://bugzilla.gnome.org/show_bug.cgi?id=744877 Done, thanks
Review of attachment 297465 [details] [review]: rejected
Review of attachment 297466 [details] [review]: rejected
Review of attachment 297474 [details] [review]: rejected
Review of attachment 297475 [details] [review]: rejected
Review of attachment 297682 [details] [review]: rejected ::: configure.ac @@ +147,3 @@ + [BASH_COMPLETION_DIR="$datadir/bash-completion/completions"]) + PKG_CHECK_MODULES([BASH_COMPLETION], [bash-completion >= 2.0], + [BASH_HELPERS_DIR="`pkg-config --variable=helpersdir bash-completion`"], Not sure I understand
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. :-) 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 2. The new implementation has the following regressions when completing gst-launch-1.0: * 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). 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. 5. $[...] is deprecated and "will be removed in upcoming versions of bash". Use $((...)) instead. 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.
Created attachment 299709 [details] [review] completions: prefix shell functions with _gst + To make it more difficult for them to conflict in the global namespace. https://bugzilla.gnome.org/show_bug.cgi?id=744877#c21
Created attachment 299710 [details] [review] completions: remove deprecated shell syntax. https://bugzilla.gnome.org/show_bug.cgi?id=744877#c21
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.
No worries. :-) Your first patch missed "_mandatory__argument" in helpers/gst. I just realised that there's a similar problem with the environment variables (HELPERDIR and HELPER) created in both the gst-launch-1.0 and gst-inspect-1.0 completion scripts. I suppose you'd need to put this inside a function that uses a local variable for the intermediate calculations. Also, I'm not convinced that the bash-completion scripts are actually being installed properly. "make distcheck" fails for me as of 17349158 (if I revert 56dd2d89 which added "--with-bash-completion-dir=no" during distcheck). Note that I *do* have 8786655b which attempts to fix the DESTDIR issue but it still fails for me. Note that DESTDIR will have to work in order for the debian packages to get the bash-completion scripts. Cheers, Dave.
Created attachment 299749 [details] [review] completions: remove last unnamespaced symbols.
Created attachment 299750 [details] [review] configure: fix installing completions when a prefix is defined.
Created attachment 299752 [details] [review] configure: fix installing completions when a prefix is defined.
make distcheck fails here for now with master, but I think the last patch I attached should make distcheck pass without bash-completion-dir=no (it passes in GES thanks to the define-variable "trick").
I rewrote that to avoid duplicating everything, and ensure support for prefix with spaces. Probably the GES should be updated too. commit 038c0a136053dc89f162b38f003e9ededdb88281 Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk> Date: Tue Mar 24 13:13:29 2015 -0400 bash-completion: Respect the prefix Don't try and install the bash helpers outside the defined prefix. https://bugzilla.gnome.org/show_bug.cgi?id=744877
Review of attachment 299709 [details] [review]: commit c72abb27377feec378f4712241e93a445a93ed2a Author: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com> Date: Wed Mar 18 14:37:11 2015 +0100 completions: prefix shell functions with _gst + To make it more difficult for them to conflict in the global namespace. https://bugzilla.gnome.org/show_bug.cgi?id=744877#c21
Review of attachment 299710 [details] [review]: commit 92d204be44263151ae08b83ad9312aec8741ceb5 Author: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com> Date: Wed Mar 18 14:44:21 2015 +0100 completions: remove deprecated shell syntax. https://bugzilla.gnome.org/show_bug.cgi?id=744877#c21
Review of attachment 299749 [details] [review]: commit c6e3c859a430f23720634622e229e62c8ce590bc Author: Mathieu Duponchelle <mathieu.duponchelle@opencreed.com> Date: Wed Mar 18 19:38:15 2015 +0100 completions: remove last unnamespaced symbols. https://bugzilla.gnome.org/show_bug.cgi?id=744877