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

Implemented option_as_alt to allow for ignoring option key characters #2576

Merged
merged 1 commit into from
Jan 31, 2023

Conversation

ayax79
Copy link
Contributor

@ayax79 ayax79 commented Nov 30, 2022

Based off the Fix Alt Modifier on Mac OS PR.

I have tested by rebuilding alacritty here and testing to ensure that alt bindings in zellij work correctly. I have also tested that IME works.

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

@ayax79 ayax79 requested a review from madsmtm as a code owner November 30, 2022 01:34
@ayax79 ayax79 mentioned this pull request Nov 30, 2022
8 tasks
@ayax79
Copy link
Contributor Author

ayax79 commented Dec 2, 2022

I reverted back to the version that is roughly the same port as @kjmph's branch. I then disabled IME and everything works correctly. This makes sense as all the IME handling was added after the original fix was created. Now to figure out why...

My test case is:

  • Recompile alacritty
  • Run Zellij
  • Alt+N creates a new pane

@ayax79
Copy link
Contributor Author

ayax79 commented Dec 2, 2022

This call unsafe { self.interpretKeyEvents(&events_for_nsview) }; seems to be the problem.. Looking to see if can build a modified event for it.

@ayax79 ayax79 changed the title Implement Option Alternative Input on macOS Implemented option_as_alt to allow for ignoring option key characters Dec 2, 2022
@ayax79
Copy link
Contributor Author

ayax79 commented Dec 2, 2022

This version works and does not break IME (as far I can tell).
To use set option_as_alt to true

Copy link
Contributor

@chrisduerr chrisduerr left a comment

Choose a reason for hiding this comment

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

Are you still working on this patch? If it's done you should probably ping one of the macOS maintainers, I don't think it's on their radar.

CHANGELOG.md Outdated Show resolved Hide resolved
@ayax79
Copy link
Contributor Author

ayax79 commented Dec 17, 2022

@madsmtm, this is complete minus updating the feature matrix and documentation. I am not sure this is relevant for the feature matrix, and I was unsure if anymore was needed for documentation beyond the documentation on the trait. thoughts?

Copy link
Member

@madsmtm madsmtm 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 ping, it was on my radar, I just have a rather long backlog atm.

Haven't looked much at the actual implementation, see my last comment; I'm unsure of what precisely the intended effect is, so I'm unable to ensure it works as expected ;)

CHANGELOG.md Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
src/platform/macos.rs Show resolved Hide resolved
src/platform/macos.rs Outdated Show resolved Hide resolved
src/platform/macos.rs Outdated Show resolved Hide resolved
src/platform/macos.rs Outdated Show resolved Hide resolved
src/platform_impl/macos/window.rs Outdated Show resolved Hide resolved
src/platform_impl/macos/appkit/event.rs Outdated Show resolved Hide resolved
examples/window_option_as_alt.rs Outdated Show resolved Hide resolved
src/platform/macos.rs Show resolved Hide resolved
@ucabgao
Copy link

ucabgao commented Jan 15, 2023

Thanks for taking a look at this!! How about the use case and where I have utilized the fix...

Basically, when enabled, option should NEVER send an extended character, giving the same behavior as the ALT key on other operating systems. I imagine this feature is really only critical in apps like Alacritty, but it is huge for Alacritty.

I picked up this issue because I use the combination Alacritty and Zellij on linux. I would like to use the same combination on my mac work machine. Zellij is pretty unusable on Alacritty on the mac due to the option key's default behavior of sending extended characters versus just acting as an ALT key. This actually causes a pretty poor experience in a wide range of terminal apps -- anything that requires an ALT key.

Every other terminal on the mac (Terminal.app, Iterm.app, Kitty) have the ability to disable sending extended characters, like this fix implements.

I have been using this fix daily for a while now. If you would like try it, my fork of Alacritty where I have utilized this fix is here: https://github.com/ayax79/alacritty/tree/option_as_alt

Here is my alacritty config: https://github.com/ayax79/config/blob/main/alacritty/alacritty.yml The relevant section is option_as_alt: true under window:

When enabled if you run zellij, OPT+n should open a new terminal pane. When disabled OPT+n should give you ˜.

This has been an outstanding issue in alacritty for a long time: alacritty/alacritty#62

Any update on this PR? Have been waiting for this feature for a long time as an Alacritty user.

@kchibisov kchibisov mentioned this pull request Jan 17, 2023
11 tasks
@ayax79
Copy link
Contributor Author

ayax79 commented Jan 17, 2023

Any update on this PR? Have been waiting for this feature for a long time as an Alacritty user.

I think we are just waiting for feedback from @madsmtm

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

What I was mostly worried about was how this interacts with IME, and as far as I can tell it seems like it doesn't negatively impact that at all, so that's nice!

I looked through the implementation now, looks good, only a few concerns.

Thanks for your patience!

src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved
src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@kchibisov
Copy link
Member

I'll add one more note, that such behavior is desired for only e.g. one of the Option modifiers keeping the other one as before. Would it be possible to adjust for that, like say Left as Alt or Right as Alt, so the user could use both at the same time.

@madsmtm
Copy link
Member

madsmtm commented Jan 19, 2023

So something like:

enum OptionAsAlt {
    All,
    OnlyRight,
    OnlyLeft,
    None,
}

Though I suspect it will be a bit tricky to implement, since macOS doesn't easily expose which OPTION button is being held down. (So I'd be fine with deferring that).

@kchibisov
Copy link
Member

Though I suspect it will be a bit tricky to implement, since macOS doesn't easily expose which OPTION button is being held down. (So I'd be fine with deferring that).

I think you have that via the raw key event? At least in the past we were able to determine left/right alt?

@madsmtm
Copy link
Member

madsmtm commented Jan 21, 2023

Yeah, it's definitely possible to know when the left or the right ALT button has been pressed, but (at least as far as I know) macOS doesn't track that information for us (like it otherwise does), hence why it might be a bit tricky.

Perhaps we can leave the user to implement it themselves on top of set_option_as_alt? Something like:

let mut set_option_as_lalt = true;

match event {
    WindowEvent::KeyboardInput {
        input: KeyboardInput {
            virtual_keycode: Some(VirtualKeyCode::LAlt),
            state,
            ..
        },
        ..
    } => {
        if set_option_as_lalt {
            window.set_option_as_alt(state == ElementState::Pressed);
        }
    }
    _ => {}
}

(Assuming it works? Would be nice to have the example updated with it if so.)

@saamalik
Copy link

@ayax79 - would be great to get this merged in. Sorry, I can't help much =)

@ayax79
Copy link
Contributor Author

ayax79 commented Jan 27, 2023

Sorry... I just started a new job and I have been pretty slammed. Hoping to get back to this soon.

@kchibisov kchibisov force-pushed the fix-alt-modifier-new branch from 18f645d to 5d9d3ca Compare January 30, 2023 21:35
@kchibisov
Copy link
Member

I've taken the liberty to update this branch and add the requested functionality. I've tested on my macOS machine with the example in the repo, and the events are correct for all left/right/both/none variations.

@madsmtm could you take a look?

@kchibisov kchibisov requested a review from madsmtm January 30, 2023 21:37
@kchibisov kchibisov force-pushed the fix-alt-modifier-new branch 4 times, most recently from c800089 to 7bb64d4 Compare January 31, 2023 06:09
Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

Implementation looks fine, only a few nits wrt. documentation and such

examples/window_option_as_alt.rs Outdated Show resolved Hide resolved
src/platform_impl/macos/appkit/event.rs Outdated Show resolved Hide resolved
src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved
This adds an ability to control left and right `Option` keys to be
treated as `Alt`, thus not producing diacritical marks.
@kchibisov kchibisov force-pushed the fix-alt-modifier-new branch from 7bb64d4 to e5da6c6 Compare January 31, 2023 09:22
@kchibisov kchibisov merged commit 180a4c7 into rust-windowing:master Jan 31, 2023
@kchibisov
Copy link
Member

Thanks, @ayax79

@madsmtm
Copy link
Member

madsmtm commented Jan 31, 2023

Perfect, thanks!

@chrisduerr
Copy link
Contributor

Yeah this has been a long standing issue on macOS that wasn't particularly difficult but simply didn't get resolved because there was no macOS developer interested in stepping up and fixing it. So thanks for stepping up.

@madsmtm madsmtm mentioned this pull request Jan 31, 2023
5 tasks
@kjmph
Copy link

kjmph commented Jan 31, 2023

Thanks everyone! I can't wait to try this out!

@saamalik
Copy link

Woot woot! this is awesome! Thanks team!

@jkp
Copy link

jkp commented Feb 2, 2023

Just pulled and built the latest - am unsure how to actually enable this from the config though?

Awesome! read the code and found I I needed this in my config:
window:
  option_as_alt: OnlyLeft

@kchibisov
Copy link
Member

Yes, the alacritty.yml has the documentation for that stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

9 participants