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

feat(ui): vim mode #1553

Merged
merged 14 commits into from
Jan 13, 2024
Merged

feat(ui): vim mode #1553

merged 14 commits into from
Jan 13, 2024

Conversation

YummyOreo
Copy link
Contributor

@YummyOreo YummyOreo commented Jan 12, 2024

This adds a vim mode: j and k to go up and down the list and i to enter insert mode. has to be enabled in config by adding vim = true

Possible other stuff

  • possibly add a color indication?
  • things to manipulate the text?

Copy link

vercel bot commented Jan 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
atuin-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 13, 2024 5:00pm

ellie
ellie previously approved these changes Jan 13, 2024
Copy link
Member

@ellie ellie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

I'd love to flesh this out some more, but let's not do it here. That match is already huge, it would be great to handle keys in a more flexible way

@ellie
Copy link
Member

ellie commented Jan 13, 2024

Just gonna need a format

@akinomyoga
Copy link
Contributor

akinomyoga commented Jan 13, 2024

I'm not sure if we should modify it in this PR, but as far as I understand, the setting of this PR defines the common behavior of all the three keymaps (emacs, vi-insert, and vi-command, or similar ones) at the same time. However, it is natural to behave like emacs when Atuin is called from the emacs keymap, like vi-insert when from the vi-insert keymap, and like vi-normal when from the vi-normal keymap. Wouldn't it be possible to switch the behavior through a command-line option depending on the keymap where the keybinding is defined?

@ellie
Copy link
Member

ellie commented Jan 13, 2024

I'm not sure if we should modify it in this PR

100%, I'm happy to leave this as-is for now, as it's a super-minimal vim mode

Wouldn't it be possible to switch the behavior through a command-line option depending on the keymap where the keybinding is defined?

Possibly. I'm not sure how I feel about the idea

  1. People may not expect vim mode to be automatically enabled, so this could be a surprise. I know there are many people who use Vim, but do not use vim mode in the shell - many who don't even know this exists. Assuming someone would want the same mode everywhere may not be correct. Either way, we'd want to be able to override it
  2. Our Vim integration may not be full, or may not match. It could be more jarring to switch from one vim implementation to another automatically, when they differ in details
  3. Should Atuin open in insert or normal mode? Should this depend on the mode of the shell? I know you ask this question, but given that the Atuin TUI's primary purpose is not editing text, I think it's not that clear cut

I'd love to explore this in the future when we have more vim parity, but for now, keeping the switch explicit is the best approach imo.

@akinomyoga
Copy link
Contributor

Thank you for your thoughts!

Actually, when I adjusted the keybindings of Bash, I also had the same feeling as #391 (comment) about the current keybinding of k in Bash's vi-command keymap. My request is simply to make the behavior of history selection consistent with the shell's behavior.

In my understanding of the mental model, the up and k keybindings try to somehow mimic the shell's behavior of recalling the command history. Inside the shell's vi-command mode, k means to move to the previous entry in the command history. However, this differs from the current behavior of atuin search. This PR enables to change the behavior, but the suggested config also affects the behavior in the shell's vi-insert mode. What I would like (and I believe the other would like) is to make it consistent with the shell's keybinding, i.e., k in vi-normal goes to the previous entry, and k in vi-insert inserts the letter.

Note that when I use the vi mode in Bash, I switch between vi-insert and vi-command. The current PR could be consistent for people who only use the vi-command mode and never enter the vi-inert mode of the shell, but I cannot imagine how to use the shell in that way.

Can I open an independent PR after this is merged?

Wouldn't it be possible to switch the behavior through a command-line option depending on the keymap where the keybinding is defined?

Possibly. I'm not sure how I feel about the idea

  1. People may not expect vim mode to be automatically enabled, so this could be a surprise.

Makes sense. Can we have a global option vim in addition to a command-line option to atuin search that tells the shell's keymap? When the global option vim is enabled, we can switch the behavior depending on the shell's keymap.

I know there are many people who use Vim, but do not use vim mode in the shell - many who don't even know this exists.

I think those who don't use the vim mode in the shell wouldn't be affected when the behavior is determined by the shell's keymap because the shell's keymap doesn't become the vim mode for those people.

  1. Our Vim integration may not be full, or may not match. It could be more jarring to switch from one vim implementation to another automatically, when they differ in details

I agree that we don't have to try to implement the real Vim mode. But I feel it's a matter independent of the current discussion.

  1. Should Atuin open in insert or normal mode? Should this depend on the mode of the shell? I know you ask this question, but given that the Atuin TUI's primary purpose is not editing text, I think it's not that clear cut

I wouldn't request to make it an editor. As far as we bind Atuin's search to k, pressing multiple k should behave similarly to the shell's binding because we typically press k multiple times to move around in the history if we are in the vi-normal mode of the shell. However, it shouldn't cause the history movement inside the vi-insert mode.

Another option might be to remove Atuin's keybinding to k from the default ones. That confuses people. People who would like it can define the keybindings by themselves. Even in that case, at least I would like to have a way to switch the behavior in the shell's vi-normal and vi-insert.

I'd love to explore this in the future when we have more vim parity, but for now, keeping the switch explicit is the best approach imo.

@ellie
Copy link
Member

ellie commented Jan 13, 2024

My request is simply to make the behavior of history selection consistent with the shell's behavior.

You're welcome to PR, I'm happy with those suggestions - I wasn't disagreeing with you at all fwiw, my only concern is automatically enabling vi mode.

I'm happy for us to open in normal mode when invoked from normal, and vice-versa for insert. Just I'd rather avoid doing so unless vim = true is in our config.

ellie
ellie previously approved these changes Jan 13, 2024
@ellie ellie merged commit a56085f into atuinsh:main Jan 13, 2024
10 checks passed
@Nemo157
Copy link
Contributor

Nemo157 commented Jan 15, 2024

I'm happy for us to open in normal mode when invoked from normal, and vice-versa for insert.

This doesn't match my intuition for how the modes should start, I use zsh vim keybindings and I tried using this along with

bindkey -a / _atuin_search_widget
bindkey -a k _atuin_up_search_widget

instead of the default bindings. Both of those are triggered from normal mode, but I would expect k to open atuin in normal mode while / opens atuin in insert mode, to match how those work in zsh's implementation.

EDIT: Ah, I see there's a new PR for it, I'll test out its behavior and post there.

@akinomyoga
Copy link
Contributor

akinomyoga commented Jan 15, 2024

Thanks, I forgot to mention this PR in the new PR. (edit: Now I added it.)

The new PR doesn't switch the behavior based on the key, but you can suggest a detailed behavior there. I can update the PR if we come up with a reasonable interface.

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

4 participants