M4444 / TMatrix

Terminal based replica of the digital rain from The Matrix.
GNU General Public License v2.0
380 stars 14 forks source link

Bash completion #16

Closed Br1ght0ne closed 5 years ago

Br1ght0ne commented 5 years ago

@eliasrg Thank you for your help!

M4444 commented 5 years ago

I didn't yet had time to test the completions but I just wanted to clarify what is expected of them. Git is a good benchmark because it has many flags and pretty good completions. I've messed around with bash completions and it's not that hard to implement short options like -C but long options that end in = like --color= become a bit tricky and probably require custom functions. Here's a test example - when you type:

git show --pr

and hit Tab you get:

git show --pretty=

Notice the cursor is just after =, no empty space was added.

A couple more tabs and the options appear:

email     format:   full      fuller    medium    oneline   raw       short

\ The completions for TMatrix should work in a similar way, for example for the --color= flag.

eliasrg commented 5 years ago

I didn't yet had time to test the completions but I just wanted to clarify what is expected of them. Git is a good benchmark because it has many flags and pretty good completions. I've messed around with bash completions and it's not that hard to implement short options like -C but long options that end in = like --color= become a bit tricky and probably require custom functions. Here's a test example - when you type:

git show --pr

and hit Tab you get:

git show --pretty=

Notice the cursor is just after =, no empty space was added.

A couple more tabs and the options appear:

email     format:   full      fuller    medium    oneline   raw       short

The completions for TMatrix should work in a similar way, for example for the --color= flag.

They do work this way, at least the zsh completions. I'm not sure how to use the bash ones.

Br1ght0ne commented 5 years ago

@M4444 Currently, these bash completions do about 50% of what you described. zsh makes it easy to add values for such arguments, but I'm currently not sure on how to easily do that in bash. Completions for git are huge...

Here's what I have now (my shell is zsh): https://asciinema.org/a/9qkc4RC65PTIvE1KIhZc4cAYc

Br1ght0ne commented 5 years ago

@M4444 I did an alpha-grade bash completion in bcd239a, could you please take a look? Now it completes modes and color names, and doesn't put unnecessary spaces.

Br1ght0ne commented 5 years ago

@M4444 If you are fine with RANGE, this can probably be merged.

Also, we can do a separate PR for changes to the man page like these.

M4444 commented 5 years ago

Also, we can do a separate PR for changes to the man page like these.

I don't want to disjoin the man page from the rest, so if this gets merged there will be 2 commits. The first one will contain the changes to the source and the equivalent changes in the man page, while the second will have the completions. If the bash completions aren't yet ready they can be separated into a third commit.

Br1ght0ne commented 5 years ago

Also, we can do a separate PR for changes to the man page like these.

I don't want to disjoin the man page from the rest, so if this gets merged there will be 2 commits. The first one will contain the changes to the source and the equivalent changes in the man page, while the second will have the completions. If the bash completions aren't yet ready they can be separated into a third commit.

Okay, I'll do a rebase now.

M4444 commented 5 years ago

@M4444 I did an alpha-grade bash completion in bcd239a, could you please take a look? Now it completes modes and color names, and doesn't put unnecessary spaces.

The bash completion is looking good. Completion of short arguments still need to be added so let's separate this into a third commit so the other two can be merged quicker.

Br1ght0ne commented 5 years ago

@M4444 Thank you for all the feedback; I'll be on it soon.

Br1ght0ne commented 5 years ago

@M4444 I did some work on the changes you requested; feel free to review when you have time.

EDIT: sorry for an awful lot of force pushes; I mixed up my master and yours when rebasing.

M4444 commented 5 years ago

EDIT: sorry for an awful lot of force pushes; I mixed up my master and yours when rebasing.

It's not a problem. It's always better when everything is correctly rebased to master. You can squash the commits into the 3 I described before.

Br1ght0ne commented 5 years ago

You can squash the commits into the 3 I described before.

Let me make this clear: we should have 3 commits -

Where bash completions can be worked on in a separate PR.

Is this right?

M4444 commented 5 years ago

Yes, that's right. I'll merge the first 2 commits now and I'll leave it up to you if you want to separate bash completions into a new PR so this one can be merged and closed now or you want bash completions to stay here in which case the the PR would remain open unit they are done.

Br1ght0ne commented 5 years ago

or you want bash completions to stay here in which case the the PR would remain open

Sure, I'd like to finish with short arguments before everything is merged.

Br1ght0ne commented 5 years ago

I've looked into bash completions some more, mostly into the examples from scop/bash-completion.

I think that bash doesn't provide for parameter naming like zsh. And _parse_help doesn't parse short arguments for options that have the long versions.

For now, I hardcoded the list of short options into the completion script, tagged it as FIXME. Let me know if you have any ideas.

M4444 commented 5 years ago

Merged the first 2 commits with some changes:

M4444 commented 5 years ago

For now, I hardcoded the list of short options into the completion script, tagged it as FIXME. Let me know if you have any ideas.

I don't any better ideas so I think it's fine to commit it like this. You can remove the FIXME. If anyone familiar with this PR comes up with a better way to do it in the future they will know what to fix.

Also, use tabs for indentation to conform with the rest of the project.

Br1ght0ne commented 5 years ago

@M4444 Done, now we have only bash completion here.

M4444 commented 5 years ago

Just leave the commit with the title, the commit message was probably left over from previous commits.

Br1ght0ne commented 5 years ago

@M4444 Thank you for the continued help. Hope it's good now.

M4444 commented 5 years ago

@filalex77 thanks for you countribution and @eliasrg thank you for the suggestions and review. I think the completion scripts turned out really great and I really appreciate your unwavering effort.

eliasrg commented 5 years ago

@M4444 My pleasure. It was interesting to learn how completions work! I can also confirm that the Arch package builds without changes.

Br1ght0ne commented 5 years ago

@eliasrg @M4444 Yes, this is really cool. Glad to have completions on my Gentoo :)

M4444 commented 5 years ago

Great, it's nice to know that everything works fine for your distributions.