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

Zsh completion #841

Closed
wants to merge 2 commits into from
Closed

Zsh completion #841

wants to merge 2 commits into from

Conversation

ghedo
Copy link
Member

@ghedo ghedo commented Jun 8, 2014

Generate and install zsh completion script (as per #775). The Perl script can probably be adapted for bash and other shells, if someone wants to look into it.

@ghost
Copy link

ghost commented Jun 8, 2014

I guess that's ok... Exactly how much --list-options output does it parse?

@ghedo
Copy link
Member Author

ghedo commented Jun 8, 2014

All of it (in theory anyway, there may be bugs). It also works around the * that appears after --af and --vf (I'm not sure if that's on purpose). Also, I've just noticed that --list-options mangles --write-filename-in-watch-later-config because it's apparently too long.

I can add patches that fix them if desired.

@ghost
Copy link

ghost commented Jun 8, 2014

It's because there are the magic --vf-add etc. options.

I want to change how some option info is output (e.g. the possible choices), is that parsed?

@ghedo
Copy link
Member Author

ghedo commented Jun 8, 2014

Not really. It takes the "description" as is and shows it to the user. So as long as the format doesn't change (space option space* description), the script should work fine (the same applies to --vo, --ao, --vf and --af).

@ghost
Copy link

ghost commented Jun 8, 2014

Should be fine then.

@ghost
Copy link

ghost commented Jun 8, 2014

Merged.

@ghost ghost closed this Jun 8, 2014
@ghost
Copy link

ghost commented Jun 9, 2014

Unfortunately, the generation of it was pretty broken. It'll break cross-compilation for example. But even with normal compilation, it seems to break for some people: https://gist.github.com/henry0312/cc40992f89f07a8c0dea#file-gistfile1-txt-L14

Disabled by default for now.

@ghedo
Copy link
Member Author

ghedo commented Jun 9, 2014

Ugh... I think something like this should work (not sure about the cross-compilation though).

commit 38b10b83741d86251020507476decb3375e1a11c
Author: Alessandro Ghedini <alessandro@ghedini.me>
Date:   Mon Jun 9 15:43:47 2014 +0200

    build: fix generation of zsh completion

    The Perl script must be run *after* the mpv executable is generated. Also use
    an absolute path to it.

diff --git a/waftools/generators/sources.py b/waftools/generators/sources.py
index 4516d09..25b08e7 100644
--- a/waftools/generators/sources.py
+++ b/waftools/generators/sources.py
@@ -38,8 +38,8 @@ def __matroska_definitions__(ctx, **kwargs):

 def __zshcomp__(ctx, **kwargs):
     ctx(
-        rule   = __zshcomp_cmd__(ctx, './mpv'),
-        before = ("c",),
+        rule   = __zshcomp_cmd__(ctx, ctx.bldnode.abspath() + '/mpv'),
+        after  = ("c", "cprogram",),
         name   = os.path.basename(kwargs['target']),
         **kwargs
     )

@henry0312
Copy link
Member

it seems to work well :)

@ghost
Copy link

ghost commented Jun 9, 2014

Still won't work with cross-compilation. You can't run any programs you build.

@ghedo
Copy link
Member Author

ghedo commented Jun 9, 2014

Well, depends on what you are cross-compiling to, but yeah... I wonder if it can be detected by waf somehow for use with deps_neg. In any case I'm ok with the completion being disabled by default. Also, should I open a new PR with that other commit?

@ghost
Copy link

ghost commented Jun 9, 2014

I'd argue that the program should never be run in the first place. Maybe run mpv as the shell completion script is invoked instead?

The only other case where we compile and run a C program is the Lua check; I'm not sure how that handles cross-compilation, but it seems to work.

@ghedo
Copy link
Member Author

ghedo commented Jun 9, 2014

Maybe run mpv as the shell completion script is invoked instead?

Running mpv and parsing the output every time a shell is started doesn't sound too good... not to mention that it's gonna be a hell of a lot of messy shell code to write.

If you don't want to run mpv at build time we can just generate the completion script manually, check it into git and only update it when it's needed. But I'd have to remove the completion for --ao/--af/--vo/--vf first.

@ghost
Copy link

ghost commented Jun 9, 2014

Running mpv and parsing the output every time a shell is started doesn't sound too good...

You could install the Perl script, and run it when the completion is invoked, and then cache it somehow.

If you don't want to run mpv at build time we can just generate the completion script manually, check it into git and only update it when it's needed.

That's probably too big, so no.

@ghost
Copy link

ghost commented Jun 9, 2014

Meanwhile, I could apply the patch that fixes compilation itself.

@ghedo
Copy link
Member Author

ghedo commented Jun 12, 2014

You could install the Perl script, and run it when the completion is invoked, and then cache it somehow.

That doesn't sound like a good solution. Where would you save the cached script? How would you know when to regenerate it? etc... FWIW, running the Perl script every time is not that slow, but still noticeably slower than a "native" completion script (and it would get re-run every time one presses TAB after mpv). It also adds a runtime dependency on perl (though I guess this is the minor of the problems).

Maybe both the "generate script at build time" and "generate script at runtime" can be provided (it's not that much code), not sure if all that is really worth it though.

I still don't quite understand why the "disabled by default" route is not ok though (it's kinda like libmpv that currently can't be built on all platforms).

That's probably too big, so no.

Without the ao/vo/... stuff it's around 400 lines, 385 of which are mpv's options (it also wouldn't have to be edited manually).

If all else fails I can try to ask the zsh developers if they want the completion script themselves (since they also have the mplayer one), though given mpv's development rate it would be hard to keep it up to date (not to mention that zsh has pretty long release cycles).

@ghost
Copy link

ghost commented Jun 12, 2014

Where would you save the cached script?

Yes, that's a problem. Possibly somewhere under ~/.config/?

I still don't quite understand why the "disabled by default" route is not ok though

That is ok, but what's bad IMO is that the compiled program is run as part of the build process. As said before, that's not safe at least for cross-compiling.

But we could leave it disabled by default, then it will not just break things.

@ghedo ghedo mentioned this pull request Jun 13, 2014
@ghedo
Copy link
Member Author

ghedo commented Jun 13, 2014

I opened #850 with the build fix patch for now.

I was also thinking that instead of having a separate script to parse the output of --list-options, mpv could output the completion itself, something like mpv --shell-completion=zsh, which would be similar to --list-options (but with a different formatting). This would be much faster than the Perl script and remove the need for the cache and buildtime genration.

@ghedo
Copy link
Member Author

ghedo commented Jun 13, 2014

I tried to do the --shell-completion thing, and while it works fine, it's really ugly so probably not a very good idea.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants