Skip to content

Conversation

vinaybantupalli
Copy link
Contributor

@vinaybantupalli vinaybantupalli commented Aug 26, 2025

This pr addresses the fix for #2713

Changes:

  • Added key handler for Alt+Ctrl+Hdelete_backward_word()
  • Added test coverage in delete_backward_word_alt_keys() that verifies both:
    • Standard Alt+Backspace binding continues to work
    • New Alt+Ctrl+H binding works correctly for backward word deletion

Testing:

The test ensures both key combinations produce identical behavior:

  • Delete the previous word from "hello world" → "hello "
  • Cursor positioned correctly after deletion

Backward Compatibility:

This change is backward compatible - existing Alt+Backspace functionality remains unchanged while adding support for the terminal-specific Alt+Ctrl+H variant

Copy link

github-actions bot commented Aug 26, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@vinaybantupalli
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Aug 26, 2025
@vinaybantupalli vinaybantupalli changed the title adding support for alt+ctrl+h to delete backward word fix issue #2713: adding support for alt+ctrl+h to delete backward word Aug 26, 2025
Comment on lines +236 to +238
modifiers,
..
} if modifiers == (KeyModifiers::CONTROL | KeyModifiers::ALT) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
modifiers,
..
} if modifiers == (KeyModifiers::CONTROL | KeyModifiers::ALT) => {
modifiers: KeyModifiers::CONTROL | KeyModifiers::ALT,
..
} => {

Copy link
Contributor Author

@vinaybantupalli vinaybantupalli Aug 26, 2025

Choose a reason for hiding this comment

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

@nornagon-openai tried that.

compiler is not treating that as a bitwise OR, but as a pattern alternation operator (basically OR).
So, either ctrl+h, or alt+h can trigger it, which we don't want.
The match guard is a comparison to bitwise OR, so it works for us.

warning: unreachable pattern
   --> tui/src/bottom_pane/textarea.rs:249:15
    |
234 | /             KeyEvent {
235 | |                 code: KeyCode::Char('h'),
236 | |                 modifiers: KeyModifiers::CONTROL | KeyModifiers::ALT,
237 | |                 ..
238 | |             } => self.delete_backward_word(),
    | |_____________- matches all the relevant values
...
249 |               | KeyEvent {
    |  _______________^
250 | |                 code: KeyCode::Char('h'),
251 | |                 modifiers: KeyModifiers::CONTROL,
252 | |                 ..
253 | |             } => self.delete_backward(1),
    | |_____________^ no value can reach this
    |
    = note: `#[warn(unreachable_patterns)]` on by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added another test (ctrl_h_backspace) for simple CTRL+H to backspace conversion, both these tests are failing if I do modifiers: KeyModifiers::CONTROL | KeyModifiers::ALT, using match's pattern matching

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, makes sense. thanks for checking!

@vinaybantupalli vinaybantupalli force-pushed the feat-2713-option-backspace branch from bdd30f1 to f016d62 Compare August 26, 2025 22:02
Copy link
Collaborator

@nornagon-openai nornagon-openai left a comment

Choose a reason for hiding this comment

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

overall happy w/ this change, just a couple of nits.

Comment on lines 1183 to 1185
use crossterm::event::KeyCode;
use crossterm::event::KeyEvent;
use crossterm::event::KeyModifiers;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think these aren't required as they're already imported at the top level, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah. removed.
I just copied from the test above :P


// Test Ctrl+H as backspace
let mut t = ta_with("hello");
t.set_cursor(3); // cursor after 'l'
Copy link
Collaborator

Choose a reason for hiding this comment

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

this test would be better if the "l" wasn't repeated, maybe change the text to be "abcdef" instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah. makes sense.

@vinaybantupalli vinaybantupalli force-pushed the feat-2713-option-backspace branch from 4dbb45f to d9b02fc Compare August 26, 2025 22:27
@nornagon-openai nornagon-openai merged commit fb3f645 into openai:main Aug 26, 2025
15 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 26, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants