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 Alt-Left/Right and Alt-Backspace editing #9

Merged
merged 2 commits into from
Jan 17, 2024

Conversation

samuela
Copy link
Contributor

@samuela samuela commented Jan 11, 2024

Fix #5.

This PR is built off of #8, so it would be easiest to merge that one first. To review this diff, it may be easiest to look at the changes from 7e9b848 only.

Copy link
Owner

@fadeevab fadeevab left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Super appreciate! There are a few changes requested!

src/prompt/cursor.rs Outdated Show resolved Hide resolved
let ix = jumps
.binary_search(&self.cursor)
.map_or_else(|i| i, |i| i + 1);
self.cursor = jumps[std::cmp::min(ix, jumps.len() - 1)];
Copy link
Owner

Choose a reason for hiding this comment

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

Well, jump.len() will never be zero inside of this if condition, however, saturating_sub is just safer.

Suggested change
self.cursor = jumps[std::cmp::min(ix, jumps.len() - 1)];
self.cursor = jumps[std::cmp::min(ix, jumps.len().saturating_sub(1))];

src/prompt/cursor.rs Outdated Show resolved Hide resolved
// Alt-Backspace
Key::Char('\u{17}') if word_editing => cursor.delete_word_left(),

// Alt-ArrowLeft
Copy link
Owner

Choose a reason for hiding this comment

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

Basically, Ctrl-Left/Ctrl-Right behave similarly in Bash, could Ctrl be enabled as well? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes... but i propose doing so in a separate PR :)

@samuela
Copy link
Contributor Author

samuela commented Jan 12, 2024

Thanks for the review @fadeevab -- taking a look now!

I also noticed that while alt+{left, right, backspace} work locally, they don't seem to work over SSH. I'll debug...

@fadeevab
Copy link
Owner

@samuela what are you going to do with this? Does it work as expected? Need to rebase it also.

Care has been taken to disable these features for `Password` interactions via the introduced `allow_word_editing(&self) -> bool` function on `PromptInteraction`. A default `true` implementation has been provided.
@samuela samuela force-pushed the samuela/word-editing branch from 7e9b848 to 6ba53f3 Compare January 17, 2024 04:01
@samuela
Copy link
Contributor Author

samuela commented Jan 17, 2024

@samuela what are you going to do with this? Does it work as expected? Need to rebase it also.

Yup, rebase done and comments addressed. I just spent a few hours tracking down the SSH issues. Turns out that it's not really an SSH issue, it's an iTerm configuration issue. So I think this should be ready for merging now.

@fadeevab fadeevab merged commit 6952b1b into fadeevab:main Jan 17, 2024
@samuela samuela deleted the samuela/word-editing branch January 17, 2024 16:19
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.

[feature request] support for keyboard shortcuts like alt+{left, right, backspace}
2 participants