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

Allow selection with vi keys #46

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

afh
Copy link
Contributor

@afh afh commented Feb 27, 2024

when the VISUAL environment variable is set.

What are your thoughts on this, @humblepenguinn, for a first attempt to please long time vi-users, who mostly deleted the cursor keys from the muscle memory?

when the VISUAL environment variable is set.
@humblepenguinn humblepenguinn merged commit 2847b95 into envio-cli:main Feb 27, 2024
7 checks passed
@humblepenguinn
Copy link
Collaborator

You beat me to it 😅.

Thank you @afh

@afh afh deleted the multiselect-vim-mode branch February 27, 2024 17:08
@afh
Copy link
Contributor Author

afh commented Feb 27, 2024

Sorry about that 😉
I have a PR out on upstream inquire enabling use of h and l keys to clear the current selection and select all. Maybe you'd like to show your support for the PR if you see value in it for envio?

@humblepenguinn
Copy link
Collaborator

@afh That sounds like a great idea! I'll definitely show my support

Also I updated your implementation to accept a boolean value only, since currently the user just has to pass the VISUAL env with any value. So in the new implementation the VISUAL env is parsed and converted to a boolean.

@afh
Copy link
Contributor Author

afh commented Feb 27, 2024

@humblepenguinn the VISUAL environment variable usually holds the command (or path to) that should be used as a visual editor, e.g. vi or vim or /usr/bin/vim. So if envio expects a boolean value, i.e. true or false that would break with "the UNIX tradition".
I did not want to parse the value of VISUAL, because it can be "too complex" for envio and the presence of the environment variable should be indicator enough that the user would appreciate inquire's vim_mode

@humblepenguinn
Copy link
Collaborator

@afh, well in that case VISUAL doesn't seem appropriate in my opinion. Since we don't really care about the visual editor being used, but rather just want a way to figure out if the user want's to use vim_mode.

And neither does inquire, require a visual editor argument but rather a boolean value (vim_mode).

What do you say?

@afh
Copy link
Contributor Author

afh commented Feb 27, 2024

Quickly reading up on VISUAL it can also be emacs. I thought VISUAL was specific to vi and its variants. Seems like it is not.

Not sure how to detect if the shell has its vi mode enabled, e.g. zsh: bindkey -v, bash: set -o vi.

🤔

@humblepenguinn
Copy link
Collaborator

humblepenguinn commented Feb 28, 2024

Perhaps we can parse the output of the following command: set -o | grep vi, but it only seems to work when I run set -o vi.

However, I don't see why we need to keep the VISUAL environment variable. We don't need either the vi or the emacs editor path. We just need a way for the user to tell us if they want vim mode enabled in their selection prompts. So, the path to the editor doesn't really matter here.

The Emacs key bindings in the inquire crate are already enabled by default, so we can't provide users the option to opt out from there. Thus, right now, we just need a way to figure out if the user wants vim_mode enabled or not.

I think we should change the name of the environment variable to VI or VI_MODE, and the user has to assign it a boolean value.

@afh
Copy link
Contributor Author

afh commented Feb 28, 2024

I think I have an idea on how to approach this. Expect a PR soon 🙂

@afh
Copy link
Contributor Author

afh commented Feb 28, 2024

Please have a look at #53 🙂 Possibly the proposed changes are trying to be too clever, but introducing yet another configuration environment variable specific to a single program does not feel right either…

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.

2 participants