After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 744877 - tools: bash completion for gst-inspect and gst-launch
tools: bash completion for gst-inspect and gst-launch
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gstreamer (core)
unspecified
Other All
: Normal enhancement
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-02-20 23:38 UTC by Mathieu Duponchelle
Modified: 2015-03-24 18:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
configure: check for bash completion. (1.48 KB, patch)
2015-02-20 23:38 UTC, Mathieu Duponchelle
rejected Details | Review
tools: create a tool to help completion. (7.68 KB, patch)
2015-02-20 23:39 UTC, Mathieu Duponchelle
rejected Details | Review
gst-inspect: enable bash completion (3.26 KB, patch)
2015-02-20 23:39 UTC, Mathieu Duponchelle
none Details | Review
gst-launch: enable bash completion (3.18 KB, patch)
2015-02-20 23:39 UTC, Mathieu Duponchelle
none Details | Review
gst-inspect: enable bash completion (4.97 KB, patch)
2015-02-20 23:53 UTC, Mathieu Duponchelle
rejected Details | Review
gst-launch: enable bash completion (3.99 KB, patch)
2015-02-20 23:53 UTC, Mathieu Duponchelle
rejected Details | Review
bash-completion: Implement in a different way. (19.74 KB, patch)
2015-02-23 16:00 UTC, Mathieu Duponchelle
rejected Details | Review
completions: prefix shell functions with _gst (4.58 KB, patch)
2015-03-18 13:45 UTC, Mathieu Duponchelle
committed Details | Review
completions: remove deprecated shell syntax. (1.36 KB, patch)
2015-03-18 13:45 UTC, Mathieu Duponchelle
committed Details | Review
completions: remove last unnamespaced symbols. (4.24 KB, patch)
2015-03-18 18:39 UTC, Mathieu Duponchelle
committed Details | Review
configure: fix installing completions when a prefix is defined. (2.01 KB, patch)
2015-03-18 18:44 UTC, Mathieu Duponchelle
none Details | Review
configure: fix installing completions when a prefix is defined. (2.01 KB, patch)
2015-03-18 19:08 UTC, Mathieu Duponchelle
none Details | Review

Description Mathieu Duponchelle 2015-02-20 23:38:52 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
Comment 1 Mathieu Duponchelle 2015-02-20 23:38:57 UTC
Created attachment 297465 [details] [review]
configure: check for bash completion.
Comment 2 Mathieu Duponchelle 2015-02-20 23:39:04 UTC
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.
Comment 3 Mathieu Duponchelle 2015-02-20 23:39:11 UTC
Created attachment 297467 [details] [review]
gst-inspect: enable bash completion

+ Start defining a common gst completion file.
Comment 4 Mathieu Duponchelle 2015-02-20 23:39:17 UTC
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
Comment 5 Mathieu Duponchelle 2015-02-20 23:53:20 UTC
Created attachment 297474 [details] [review]
gst-inspect: enable bash completion

+ Start defining a common gst completion file.
Comment 6 Mathieu Duponchelle 2015-02-20 23:53:26 UTC
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
Comment 7 Tim-Philipp Müller 2015-02-21 00:06:39 UTC
How does this relate to the existing bash completion stuff, and bug #740108	 ?
Comment 8 Mathieu Duponchelle 2015-02-21 00:29:23 UTC
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.
Comment 9 Mathieu Duponchelle 2015-02-21 00:31:44 UTC
And the configure / Makefile part is as olivier suggests in bug #740108 with my patches
Comment 10 Mathieu Duponchelle 2015-02-23 16:00:11 UTC
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.
Comment 11 Tim-Philipp Müller 2015-02-23 16:17:35 UTC
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.
Comment 12 Mathieu Duponchelle 2015-02-23 16:20:57 UTC
(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 ?
Comment 13 Mathieu Duponchelle 2015-02-23 16:40:30 UTC
> --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.
Comment 14 Tim-Philipp Müller 2015-02-23 16:45:58 UTC
Yeah, just go ahead and push.
Comment 15 Mathieu Duponchelle 2015-02-23 16:58:39 UTC
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
Comment 16 Mathieu Duponchelle 2015-02-23 16:59:08 UTC
Review of attachment 297465 [details] [review]:

rejected
Comment 17 Mathieu Duponchelle 2015-02-23 16:59:22 UTC
Review of attachment 297466 [details] [review]:

rejected
Comment 18 Mathieu Duponchelle 2015-02-23 16:59:36 UTC
Review of attachment 297474 [details] [review]:

rejected
Comment 19 Mathieu Duponchelle 2015-02-23 16:59:49 UTC
Review of attachment 297475 [details] [review]:

rejected
Comment 20 Mathieu Duponchelle 2015-02-23 17:00:03 UTC
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
Comment 21 David Röthlisberger 2015-03-15 19:56:16 UTC
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.
Comment 22 Mathieu Duponchelle 2015-03-18 13:45:47 UTC
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
Comment 23 Mathieu Duponchelle 2015-03-18 13:45:53 UTC
Created attachment 299710 [details] [review]
completions: remove deprecated shell syntax.

https://bugzilla.gnome.org/show_bug.cgi?id=744877#c21
Comment 24 Mathieu Duponchelle 2015-03-18 13:47:21 UTC
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.
Comment 25 David Röthlisberger 2015-03-18 17:57:12 UTC
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.
Comment 26 Mathieu Duponchelle 2015-03-18 18:39:31 UTC
Created attachment 299749 [details] [review]
completions: remove last unnamespaced symbols.
Comment 27 Mathieu Duponchelle 2015-03-18 18:44:17 UTC
Created attachment 299750 [details] [review]
configure: fix installing completions when a prefix is defined.
Comment 28 Mathieu Duponchelle 2015-03-18 19:08:43 UTC
Created attachment 299752 [details] [review]
configure: fix installing completions when a prefix is defined.
Comment 29 Mathieu Duponchelle 2015-03-18 19:10:02 UTC
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").
Comment 30 Nicolas Dufresne (ndufresne) 2015-03-24 17:17:32 UTC
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
Comment 31 Mathieu Duponchelle 2015-03-24 18:13:40 UTC
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
Comment 32 Mathieu Duponchelle 2015-03-24 18:13:57 UTC
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
Comment 33 Mathieu Duponchelle 2015-03-24 18:14:12 UTC
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