Skip to content

feat: support CMD as prefix for keybindings in macOS#8

Open
erickgnavar wants to merge 1 commit into
roramirez:mainfrom
erickgnavar:support-cmd-as-prefix-for-keybinding-in-macos
Open

feat: support CMD as prefix for keybindings in macOS#8
erickgnavar wants to merge 1 commit into
roramirez:mainfrom
erickgnavar:support-cmd-as-prefix-for-keybinding-in-macos

Conversation

@erickgnavar

Copy link
Copy Markdown

No description provided.

@roramirez roramirez left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I appreciate the contribution! A few things needed before this can merge:

Tests - please add test cases covering:

  • Cmd+TAction::NewTab (happy path)
  • Cmd+<unmapped key>Action::None (guard fires)
  • Ctrl+Cmd+TAction::NewTab (both modifiers held)
  • Cmd+<key> in InputMode::Insert vs InputMode::Normal

CHANGELOG - add an entry under ## [Unreleased]:

Added

  • support Cmd key as Ctrl equivalaent for keybindings on macOS

Comment thread src/input/keybindings.rs
);

// macOS: Cmd+key that doesn't match a shortcut is consumed, not forwarded to PTY.
if cmd && !ctrl && matches!(action, Action::SendToPty(_)) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Suggested change
if cmd && !ctrl && matches!(action, Action::SendToPty(_)) {
// macOS: Consume any Cmd+key that has no binding, even when Ctrl is also held.
// Without this, Cmd+Ctrl+<unmapped> would leak a control byte to the PTY.
if cmd && matches!(action, Action::SendToPty(_)) {

These two modifiers are orthogonal — CTRL and SUPER should be independent bits with no exclusion logic between them. The !ctrl guard should be dropped.

Also, I would change the comment mentioned before

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