Skip to content

Allow Alt-based navigation on macOS #207

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.

Sign up for GitHub

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

Merged
lhecker merged 4 commits into from
May 27, 2025
Merged

Allow Alt-based navigation on macOS #207

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e44d971
read literal forward/back-word and allow alt-based navigation on macOS
diabloproject May 22, 2025
1e383a1
Merge remote-tracking branch 'origin/main' into alt-arrows-should-jum…
lhecker May 27, 2025
b4f0dd8
Rename WORD_KEY, Move keybinds to tui.rs
lhecker May 27, 2025
197b00f
Disable menubar shortcuts on macOS
lhecker May 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions src/tui.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,8 @@ use crate::{apperr, arena_format, input, unicode};

const ROOT_ID: u64 = 0x14057B7EF767814F; // Knuth's MMIX constant
const SHIFT_TAB: InputKey = vk::TAB.with_modifiers(kbmod::SHIFT);
const KBMOD_FOR_WORD_NAV: InputKeyMod =
if cfg!(target_os = "macos") { kbmod::ALT } else { kbmod::CTRL };
Comment on lines +164 to +165
Copy link
Member

@lhecker lhecker May 27, 2025

Choose a reason for hiding this comment

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

Made the name a bit more specific so it's easier to remember what it stands for in future reviews.


type Input<'input> = input::Input<'input>;
type InputKey = input::InputKey;
Expand Down Expand Up @@ -2495,7 +2497,7 @@ impl<'a> Context<'a, '_> {
}
}
vk::LEFT => {
let granularity = if modifiers.contains(kbmod::CTRL) {
let granularity = if modifiers.contains(KBMOD_FOR_WORD_NAV) {
CursorMovement::Word
} else {
CursorMovement::Grapheme
Expand Down Expand Up @@ -2553,7 +2555,7 @@ impl<'a> Context<'a, '_> {
}
}
vk::RIGHT => {
let granularity = if modifiers.contains(kbmod::CTRL) {
let granularity = if modifiers.contains(KBMOD_FOR_WORD_NAV) {
CursorMovement::Word
} else {
CursorMovement::Grapheme
Expand Down Expand Up @@ -2634,6 +2636,22 @@ impl<'a> Context<'a, '_> {
kbmod::CTRL => tb.select_all(),
_ => return false,
},
vk::B => match modifiers {
kbmod::ALT if cfg!(target_os = "macos") => {
// On macOS, terminals commonly emit the Emacs style
// Alt+B (ESC b) sequence for Alt+Left.
tb.cursor_move_delta(CursorMovement::Word, -1);
}
_ => return false,
},
vk::F => match modifiers {
Copy link
Contributor Author

@diabloproject diabloproject May 27, 2025

Choose a reason for hiding this comment

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

Pressing Alt+RightArrow opens top menu (File), instead of moving cursor one word to the right

Copy link
Contributor Author

@diabloproject diabloproject May 27, 2025

Choose a reason for hiding this comment

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

Writing panic! in this case does not result in the program crashing, making me think that program never reaches Alt-F

Copy link
Member

@lhecker lhecker May 27, 2025

Choose a reason for hiding this comment

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

Right, because Alt+F is the shortcut for the "File" menu. The menubar shortcuts are global and so they steal it before it arrives here. This also means that your PR likely broke Alt+F for opening the "File" menu, right?

Not sure how to resolve this conflict.

Copy link
Contributor Author

@diabloproject diabloproject May 27, 2025

Choose a reason for hiding this comment

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

I actually did not know that edit had this feature...
We probably can reassign the Alt-F to Alt-Shift-F on mac, but at this point we actually need to start thinking about separating actions and shortcuts, so we can define two separate layouts for mac and win/linux

Copy link
Member

@lhecker lhecker May 27, 2025

Choose a reason for hiding this comment

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

Apparently menubar shortcuts aren't even a thing on macOS? I'll just disable them for now. You can use F10 to focus the menubar alternatively. (In the future we could add a different way to focus it on macOS.)

Copy link
Contributor Author

@diabloproject diabloproject May 27, 2025

Choose a reason for hiding this comment

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

I think we should abandon this PR in favour of making more general input system. I can do it today/tomorrow, if it can be included in the release.

Copy link
Member

@lhecker lhecker May 27, 2025

Choose a reason for hiding this comment

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

No, I think this PR is fine. Building a generalized system would be greatly appreciated though of course. I do think it would be a rather complex PR though, as it would need to be a full input-to-action mapper with knowledge of the context, etc.

Copy link
Member

@lhecker lhecker May 27, 2025

Choose a reason for hiding this comment

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

If you'd like to test the latest state, please let me know if it works for you. 🙂
Otherwise, I'll merge it as-is, because this logic is better than nothing for sure.

Copy link
Contributor Author

@diabloproject diabloproject May 27, 2025

Choose a reason for hiding this comment

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

Yeah, it works, just tested in iTerm and Terminal.app

kbmod::ALT if cfg!(target_os = "macos") => {
// On macOS, terminals commonly emit the Emacs style
// Alt+F (ESC f) sequence for Alt+Right.
tb.cursor_move_delta(CursorMovement::Word, 1);
}
_ => return false,
},
vk::H => match modifiers {
kbmod::CTRL => tb.delete(CursorMovement::Word, -1),
_ => return false,
Expand Down Expand Up @@ -3069,6 +3087,7 @@ impl<'a> Context<'a, '_> {
///
/// Returns true if the menu is open. Continue appending items to it in that case.
pub fn menubar_menu_begin(&mut self, text: &str, accelerator: char) -> bool {
let accelerator = if cfg!(target_os = "macos") { '\0' } else { accelerator };
let mixin = self.tree.current_node.borrow().child_count as u64;
self.next_block_id_mixin(mixin);

Expand All @@ -3081,7 +3100,8 @@ impl<'a> Context<'a, '_> {
self.attr_padding(Rect::two(0, 1));

let contains_focus = self.contains_focus();
let keyboard_focus = !contains_focus
let keyboard_focus = accelerator != '\0'
&& !contains_focus
&& self.consume_shortcut(kbmod::ALT | InputKey::new(accelerator as u32));
Copy link
Member

@DHowett DHowett May 27, 2025

Choose a reason for hiding this comment

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

@lhecker it's safe to consume_shortcut(alt|0)? don't we still want the accelerator != '\0' bit? we did expressly set it to \0

Copy link
Member

@lhecker lhecker May 27, 2025

Choose a reason for hiding this comment

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

Maybe your diff viewer swallowed it, because I actually added that check!

Copy link
Member

@DHowett DHowett May 27, 2025

Choose a reason for hiding this comment

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

i think I am blind or broken

Copy link
Contributor Author

@diabloproject diabloproject May 27, 2025

Choose a reason for hiding this comment

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

Strange question, but does && guarantee sequential checks? I am asking because it is a known problem in languages like C/C++

Copy link
Member

@lhecker lhecker May 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I believe you're referring to short-circuit evaluation. Both C++ and Rust support it to my knowledge. In this case it guarantees that the function is not executed on macOS, because accelerator will be 0.


if contains_focus || keyboard_focus {
Expand Down