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

Add support for vi mode #3

Merged
merged 1 commit into from
May 17, 2016
Merged

Add support for vi mode #3

merged 1 commit into from
May 17, 2016

Conversation

terlar
Copy link
Contributor

@terlar terlar commented May 16, 2016

VI mode will be useless with the current bindings since they are bound
to normal mode, this adds the possibility to set which mode should be
used to enable the pisces bindings in vi mode. (-M insert).

@terlar
Copy link
Contributor Author

terlar commented May 16, 2016

The alternative would be to not rely on the user configuring this and instead look at $fish_bind_mode.

@laughedelic
Copy link
Owner

👍 As far as I understand $fish_bind_mode stores the current mode, so it's not what we want here.
I was going to do it using the $fish_key_bindings var. I think there's no need to expose a new var for that:

# in key_bindings and pass it to _pisces_bind_pair as an argument
set -l _pisces_bind_mode default
if [ "$fish_key_bindings" = "fish_vi_key_bindings" ]
    set _pisces_bind_mode insert
end

Could you do it this way?

@terlar
Copy link
Contributor Author

terlar commented May 17, 2016

@laughedelic I updated using this instead. Only thing I am not sure about is the position of the mode argument, I would prefer having it first, but that would break existing installations if they have copied the key_bindings. This new way fallbacks to default inside the _pisces_bind_mode in order to safeguard calling the function without mode also.

What do you think?

@laughedelic
Copy link
Owner

Looks good! I would also make mode the first argument, but I think it's not so important now, because I'm going to introduce some other breaking changes in #5 and leave this _pisces_bind_pair for backward compatibility: old settings (with pisces_pairs) will work using that function as before for those who have their own settings and if they want to upgrade, they can just reset that var to the new default.

So I would merge it as it is now, but it requires updating this branch to the current master first (with the changes from #4).

VI mode will be useless with the current bindings since they are bound
to normal mode, this adds the possibility to set which mode should be
used to enable the pisces bindings in vi mode. (`-M insert`).
@terlar
Copy link
Contributor Author

terlar commented May 17, 2016

Rebased towards new master, also switched around mode to be in the beginning.

@laughedelic
Copy link
Owner

Well done 👍

@laughedelic laughedelic merged commit 863a34c into laughedelic:master May 17, 2016
@laughedelic
Copy link
Owner

@terlar one thing I forgot about is the uninstall script. It erases bindings and it should also take mode parameter. I'm fixing it.

@laughedelic
Copy link
Owner

Published v0.4.0. Thanks for your contribution @terlar 👍

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

Successfully merging this pull request may close these issues.

2 participants