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: add common default keybindings #719

Merged
merged 4 commits into from Mar 5, 2023

Conversation

stevenxxiu
Copy link
Contributor

@stevenxxiu stevenxxiu force-pushed the feat/common-default-keybindings branch 2 times, most recently from 7e4dd30 to b7e7b97 Compare February 17, 2023 08:35
@stevenxxiu stevenxxiu marked this pull request as draft February 17, 2023 18:16
@stevenxxiu stevenxxiu force-pushed the feat/common-default-keybindings branch 3 times, most recently from ebacfb8 to 9776be3 Compare February 18, 2023 05:25
@stevenxxiu stevenxxiu marked this pull request as ready for review February 18, 2023 05:28
@stevenxxiu stevenxxiu force-pushed the feat/common-default-keybindings branch from 9776be3 to 248f7de Compare February 18, 2023 05:31
@trygveaa
Copy link
Contributor

I think it would be beneficial if this behaved in the same way as bash and zsh. The differences I see is that they always move to the beginning/end of the next word with ctrl-left/right. So with e.g. test()-test and the cursor at the end, ctrl-left first moves after - and then to the beginning. And then ctrl-right moves before ( and then to the end. ctrl-delete and alt-backspace does the same movements, but deletes the words instead (ctrl-backspace doesn't do anything in me for bash or zsh, and just moves the cursor in this branch for me).

Another difference is that _ is counted as a word separator in bash and zsh (I haven't checked all other symbols).

(I'm not affiliated with atuin btw, so these are just my personal opinions).

I also notice that if I press ctrl-right or ctrl-delete on an empty prompt, atuin panics with this branch.

@stevenxxiu stevenxxiu force-pushed the feat/common-default-keybindings branch from 248f7de to 5722687 Compare February 18, 2023 09:57
@stevenxxiu
Copy link
Contributor Author

stevenxxiu commented Feb 18, 2023

I also notice that if I press ctrl-right or ctrl-delete on an empty prompt, atuin panics with this branch.

Thanks for testing. Should be fixed now. I added a test case for that.

I think it would be beneficial if this behaved in the same way as bash and zsh.

I made the word jumping action identical to Sublime Text, which is what I'm used to. In fact, I configured my Zsh line editor to behave the same way. I use vi-backward-word and vi-forward-word in my configuration with a custom WORDCHARS variable. (I'm thinking about switching to Nushell soon.)

I tried your example of test()-test. The current behavior also matches my browser (Vivaldi).

I think word jumping behavior is up to personal preference. It makes sense to add some configuration option for this. Would a configurable WORD_SEPARATORS suit?

@trygveaa
Copy link
Contributor

Ah, right, emacs-forward/backward-word and vi-forward/backward-word behave differently in zsh and none of them are the default. I think bash behaves the way I said by default though. I think it would make sense to stick close to the default behavior of common shells, since this is a tool for shell history. I'll defer to the maintainers here to decide this and to decide whether a configuration option should be added.

@conradludgate
Copy link
Collaborator

We already have a mechanism for deleting words: Ctrl-W. I've just tested it and it treats test()-test as a single word.

As I don't use this feature in any shell and I'm not an emacs/vim keybinding user I can't really comment on which behaviour works best here

@ellie
Copy link
Member

ellie commented Feb 26, 2023

Ah, right, emacs-forward/backward-word and vi-forward/backward-word behave differently in zsh and none of them are the default. I think bash behaves the way I said by default though. I think it would make sense to stick close to the default behavior of common shells, since this is a tool for shell history. I'll defer to the maintainers here to decide this and to decide whether a configuration option should be added.

There doesn't really seem to be an obvious answer, so I think going for the default bash behaviour is the best option - I imagine most people will be used to this!

Otherwise, making things configurable would be 💯

@stevenxxiu stevenxxiu force-pushed the feat/common-default-keybindings branch from 5722687 to 3bb70e1 Compare February 28, 2023 12:36
@vercel
Copy link

vercel bot commented Feb 28, 2023

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

Name Status Preview Comments Updated
atuin ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 1, 2023 at 7:56AM (UTC)

@stevenxxiu
Copy link
Contributor Author

stevenxxiu commented Feb 28, 2023

I added configurable word_jump_mode (emacs/subl for Sublime Text) and word_chars options now.

Hope this matches your expectations of Emacs behavior. Please let me know if it's still incorrect in some way.

@stevenxxiu stevenxxiu force-pushed the feat/common-default-keybindings branch from 0236b2f to cbbde13 Compare March 1, 2023 07:55
@stevenxxiu
Copy link
Contributor Author

@ellie Sorry about the nag but have time to look at this?

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.

I'm happy for this to go in, thank you for the work! 🚀 And apologies for the slower review :/

@ellie ellie merged commit 2e79e73 into atuinsh:main Mar 5, 2023
@stevenxxiu stevenxxiu deleted the feat/common-default-keybindings branch March 6, 2023 03:51
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