Allow Alt-based navigation on macOS#207
Conversation
diabloproject
changed the title
| const KBMOD_FOR_WORD_NAV: InputKeyMod = | ||
| if cfg!(target_os = "macos") { kbmod::ALT } else { kbmod::CTRL }; |
There was a problem hiding this comment.
Made the name a bit more specific so it's easier to remember what it stands for in future reviews.
| } | ||
| _ => return false, | ||
| }, | ||
| vk::F => match modifiers { |
There was a problem hiding this comment.
Pressing Alt+RightArrow opens top menu (File), instead of moving cursor one word to the right
There was a problem hiding this comment.
Writing panic! in this case does not result in the program crashing, making me think that program never reaches Alt-F
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, it works, just tested in iTerm and Terminal.app
lhecker
changed the title
| let keyboard_focus = !contains_focus | ||
| let keyboard_focus = accelerator != '\0' | ||
| && !contains_focus | ||
| && self.consume_shortcut(kbmod::ALT | InputKey::new(accelerator as u32)); |
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
Maybe your diff viewer swallowed it, because I actually added that check!
There was a problem hiding this comment.
Strange question, but does && guarantee sequential checks? I am asking because it is a known problem in languages like C/C++
There was a problem hiding this comment.
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.
Closes microsoft#180 --------- Co-authored-by: Leonard Hecker <leonard@hecker.io>
3 participants
Closes #180