Fix Alt modifier on macOS #1449
Conversation
The complications between insertText and keyDown were causing difficulties, especially when diacritics are used. This commit cleans out the duplication of events and the state that needs to be captured between event handlers.
goddessfreya
added
DS - macos
C - needs investigation
There was a problem hiding this comment.
I'm not the one to review this thing tbh, but just some comments to make sure we're not breaking things. I'd recommend to test IME and different keyboard layouts. As well as key repeat handling with different characters, like Enter, Backspace and with different modifiers.
| @@ -131,6 +131,7 @@ pub trait WindowBuilderExtMacOS { | |||
| /// Build window with `resizeIncrements` property. Values must not be 0. | |||
| fn with_resize_increments(self, increments: LogicalSize<f64>) -> WindowBuilder; | |||
| fn with_disallow_hidpi(self, disallow_hidpi: bool) -> WindowBuilder; | |||
| fn with_ignore_alt_modifier(self, ignore_alt_modifier: bool) -> WindowBuilder; | |||
There was a problem hiding this comment.
I feel like it should be with_option_as_alt, since it's more clear to the users. Getting inspiration from other apps could be a good idea.
There was a problem hiding this comment.
Yes, we should make this flag as descriptive as possible.
There was a problem hiding this comment.
This should still be changed.
There was a problem hiding this comment.
Okay, I'll make the change to with_option_as_alt
| extern "C" fn insert_text(this: &Object, _sel: Sel, string: id, _replacement_range: NSRange) { | ||
| trace!("Triggered `insertText`"); | ||
| unsafe { | ||
| let state_ptr: *mut c_void = *this.get_ivar("winitState"); | ||
| let state = &mut *(state_ptr as *mut ViewState); |
There was a problem hiding this comment.
I'm not sure about this change tbh. However I can't prove it myself that this thing is requited. So I need a time to go through the history to understand what this thing is about. I'd suggest you to do the same, since there should be a reason for it.
There was a problem hiding this comment.
I've been going through old issues, and seeing that I can't reproduce any of them. I'll keep digging.
| extern "C" fn do_command_by_selector(this: &Object, _sel: Sel, command: Sel) { | ||
| trace!("Triggered `doCommandBySelector`"); |
There was a problem hiding this comment.
Same as for insert text here.
|
Also could you compare |
|
Do you mean this? Without: With: |
Yeah, I was a bit sleepy during reading your log from #1436 (comment) , I've somehow assumed that you pressed |
goddessfreya
added
C - waiting on maintainer
|
Especially for Alacritty's live config reload it would be interesting to change this at runtime. Do you think it would be possible to add a |
|
Very interesting, as the boolean |
|
I think that would be very worthwhile and is not uncommon with window parameters. Especially for Alacritty it would be excellent. |
|
@kjmph are you still working on this PR? |
|
Yes, I'll pull it together, the day job is busy at the moment. I'll dedicate time tonight. |
|
Okay, it works now.. Shall I commit the changes for winit to this pull request / branch? I know there were a couple of users testing this pull request, just curious what is the process around here. The changes are a bit more extensive to alacritty, as this needs to be processed by the Here is the patch to alacritty: diff --git a/alacritty.yml b/alacritty.yml
index e552166..63c11b5 100644
--- a/alacritty.yml
+++ b/alacritty.yml
@@ -347,6 +347,9 @@
# Send ESC (\x1b) before characters when alt is pressed.
#alt_send_esc: true
+# (macOS Only) When Option is pressed treat as alt modifier. Don't send diacritical marks.
+#option_as_alt: true
+
#debug:
# Display the time it takes to redraw each frame.
#render_timer: false
diff --git a/alacritty/src/event.rs b/alacritty/src/event.rs
index cd4f82a..b22e3f1 100644
--- a/alacritty/src/event.rs
+++ b/alacritty/src/event.rs
@@ -504,6 +504,10 @@ impl<N: Notify + OnResize> Processor<N> {
processor.ctx.display_update_pending.font = Some(font);
}
+ if cfg!(target_os = "macos") && processor.ctx.config.option_as_alt() != config.option_as_alt() {
+ processor.ctx.window.set_option_as_alt(config.option_as_alt());
+ }
+
*processor.ctx.config = config;
processor.ctx.terminal.dirty = true;
diff --git a/alacritty/src/window.rs b/alacritty/src/window.rs
index 71327ba..6cd9041 100644
--- a/alacritty/src/window.rs
+++ b/alacritty/src/window.rs
@@ -151,6 +151,11 @@ impl Window {
// Set OpenGL symbol loader. This call MUST be after window.make_current on windows.
gl::load_with(|symbol| windowed_context.get_proc_address(symbol) as *const _);
+ // get_platform_window could be invoked with Config instead of
+ // WindowConfig if this call is misplaced.
+ #[cfg(target_os = "macos")]
+ windowed_context.window().set_option_as_alt(config.option_as_alt());
+
// On X11, embed the window inside another if the parent ID has been set
#[cfg(not(any(target_os = "macos", windows)))]
{
@@ -356,6 +361,11 @@ impl Window {
self.window().set_simple_fullscreen(simple_fullscreen);
}
+ #[cfg(target_os = "macos")]
+ pub fn set_option_as_alt(&mut self, option_as_alt: bool) {
+ self.window().set_option_as_alt(option_as_alt);
+ }
+
#[cfg(not(any(target_os = "macos", target_os = "windows")))]
pub fn wayland_display(&self) -> Option<*mut c_void> {
self.window().wayland_display()
diff --git a/alacritty_terminal/src/config/mod.rs b/alacritty_terminal/src/config/mod.rs
index fd049af..a9dabc4 100644
--- a/alacritty_terminal/src/config/mod.rs
+++ b/alacritty_terminal/src/config/mod.rs
@@ -117,6 +117,10 @@ pub struct Config<T> {
#[serde(default, deserialize_with = "failure_default")]
alt_send_esc: DefaultTrueBool,
+ /// Option on macOS is treated as Alt modifier
+ #[serde(default, deserialize_with = "failure_default")]
+ option_as_alt: DefaultTrueBool,
+
/// Shell startup directory
#[serde(default, deserialize_with = "option_explicit_none")]
pub working_directory: Option<PathBuf>,
@@ -197,6 +201,12 @@ impl<T> Config<T> {
self.alt_send_esc.0
}
+ /// Option on macOS is treated as Alt modifier
+ #[inline]
+ pub fn option_as_alt(&self) -> bool {
+ self.option_as_alt.0
+ }
+
/// Keep the log file after quitting Alacritty
#[inline]
pub fn persistent_logging(&self) -> bool { |
|
Note, for reference, here is the commit to modify this branch to the new config style: diff --git a/src/platform/macos.rs b/src/platform/macos.rs
index c600d37..2cd10a1 100644
--- a/src/platform/macos.rs
+++ b/src/platform/macos.rs
@@ -56,6 +56,14 @@ pub trait WindowExtMacOS {
/// And allows the user to have a fullscreen window without using another
/// space or taking control over the entire monitor.
fn set_simple_fullscreen(&self, fullscreen: bool) -> bool;
+
+ /// Toggles the `Option` key being interpretted as an `Alt` modifier.
+ ///
+ /// This will ignore diacritical marks and accent characters from
+ /// being processed as received characters. Instead, the input
+ /// device's raw character will be placed in event queues with the
+ /// Alt modifier set.
+ fn set_option_as_alt(&self, option_as_alt: bool);
}
impl WindowExtMacOS for Window {
@@ -83,6 +91,11 @@ impl WindowExtMacOS for Window {
fn set_simple_fullscreen(&self, fullscreen: bool) -> bool {
self.window.set_simple_fullscreen(fullscreen)
}
+
+ #[inline]
+ fn set_option_as_alt(&self, option_as_alt: bool) {
+ self.window.set_option_as_alt(option_as_alt)
+ }
}
/// Corresponds to `NSApplicationActivationPolicy`.
@@ -131,7 +144,6 @@ pub trait WindowBuilderExtMacOS {
/// Build window with `resizeIncrements` property. Values must not be 0.
fn with_resize_increments(self, increments: LogicalSize<f64>) -> WindowBuilder;
fn with_disallow_hidpi(self, disallow_hidpi: bool) -> WindowBuilder;
- fn with_ignore_alt_modifier(self, ignore_alt_modifier: bool) -> WindowBuilder;
}
impl WindowBuilderExtMacOS for WindowBuilder {
@@ -191,12 +203,6 @@ impl WindowBuilderExtMacOS for WindowBuilder {
self.platform_specific.disallow_hidpi = disallow_hidpi;
self
}
-
- #[inline]
- fn with_ignore_alt_modifier(mut self, ignore_alt_modifier: bool) -> WindowBuilder {
- self.platform_specific.ignore_alt_modifier = ignore_alt_modifier;
- self
- }
}
/// Additional methods on `MonitorHandle` that are specific to MacOS.
diff --git a/src/platform_impl/macos/view.rs b/src/platform_impl/macos/view.rs
index 3a48991..6857d7f 100644
--- a/src/platform_impl/macos/view.rs
+++ b/src/platform_impl/macos/view.rs
@@ -31,7 +31,7 @@ use crate::{
ffi::*,
util::{self, IdRef},
window::get_window_id,
- PlatformSpecificWindowBuilderAttributes, DEVICE_ID,
+ DEVICE_ID,
},
window::WindowId,
};
@@ -65,10 +65,7 @@ impl ViewState {
}
}
-pub fn new_view(
- ns_window: id,
- pl_attribs: &PlatformSpecificWindowBuilderAttributes,
-) -> (IdRef, Weak<Mutex<CursorState>>) {
+pub fn new_view(ns_window: id) -> (IdRef, Weak<Mutex<CursorState>>) {
let cursor_state = Default::default();
let cursor_access = Arc::downgrade(&cursor_state);
let state = ViewState {
@@ -77,7 +74,7 @@ pub fn new_view(
ime_spot: None,
modifiers: Default::default(),
tracking_rect: None,
- ignore_alt_modifier: pl_attribs.ignore_alt_modifier,
+ ignore_alt_modifier: false,
};
unsafe {
// This is free'd in `dealloc`
@@ -101,6 +98,12 @@ pub unsafe fn set_ime_position(ns_view: id, input_context: id, x: f64, y: f64) {
let _: () = msg_send![input_context, invalidateCharacterCoordinates];
}
+pub unsafe fn set_option_as_alt(ns_view: id, option_as_alt: bool) {
+ let state_ptr: *mut c_void = *(*ns_view).get_mut_ivar("winitState");
+ let state = &mut *(state_ptr as *mut ViewState);
+ state.ignore_alt_modifier = option_as_alt;
+}
+
struct ViewClass(*const Class);
unsafe impl Send for ViewClass {}
unsafe impl Sync for ViewClass {}
diff --git a/src/platform_impl/macos/window.rs b/src/platform_impl/macos/window.rs
index 9577511..7412033 100644
--- a/src/platform_impl/macos/window.rs
+++ b/src/platform_impl/macos/window.rs
@@ -23,7 +23,7 @@ use crate::{
monitor::{self, MonitorHandle, VideoMode},
util::{self, IdRef},
view::CursorState,
- view::{self, new_view},
+ view::{self, new_view, set_option_as_alt},
window_delegate::new_delegate,
OsError,
},
@@ -95,7 +95,7 @@ unsafe fn create_view(
ns_window: id,
pl_attribs: &PlatformSpecificWindowBuilderAttributes,
) -> Option<(IdRef, Weak<Mutex<CursorState>>)> {
- let (ns_view, cursor_state) = new_view(ns_window, pl_attribs);
+ let (ns_view, cursor_state) = new_view(ns_window);
ns_view.non_nil().map(|ns_view| {
if !pl_attribs.disallow_hidpi {
ns_view.setWantsBestResolutionOpenGLSurface_(YES);
@@ -1088,6 +1088,13 @@ impl WindowExtMacOS for UnownedWindow {
}
}
}
+
+ #[inline]
+ fn set_option_as_alt(&self, option_as_alt: bool) {
+ unsafe {
+ set_option_as_alt(*self.ns_view, option_as_alt);
+ }
+ }
}
impl Drop for UnownedWindow { |
|
Note, |
|
As the downstream issue reported successful testing, I'll push the changes to this branch now. |
While reviewing diacritical mark fixes on macOS by extending WindowBuilder, it was requested that the patch would be modified to a 'set_option_as_alt' method on the Window. This allows for downstream users to enable/disable while the Window has already been initialized.
|
Is there anything I can do to advance this? |
|
well, we're waiting on macOS winit maintainers to take a look at this code. I'd recommend to test IME/virtual keyboards, since it seems to me that |
|
Ok! That sounds good. I'll test that. |
|
I tested with a Virtual Keyboard, and it appears to send proper key codes. However, I do notice with Input modified to Chinese, things like popups suggesting character combinations don't show up in Alacritty. This occurs irrespective of this patch. I'd still love to hear if this patch works for Chinese/Cangjie users (or any input modifier user), yet it produces valid output. |
@chrisduerr earlier offered to take over this PR. I'm guessing that the offer still stands and I think they'd be more than happy to take over if you don't feel like you have sufficient time working on this. Is that correct @chrisduerr? Otherwise I can take over if you'd like that @kjmph. |
|
Yes, I'm not into macOS development either but this issue causes a lot of trouble downstream. So the sooner this is resolved the less work I have to put into support tickets, so there's definitely some time I can invest to resolve that. Of course every time I pick up one thing I can't do another which is why outside contributions are always very helpful. |
|
I've decided to take a little peak at what one would have to do to get this working and have some questions about this implementation. Looking at iTerm, it seems like it does some special checking for the modifier key and skips the https://gitlab.com/gnachman/iterm2/-/blob/master/sources/iTermStandardKeyMapper.m#L435 Winit already seems to be doing something similar with key repetition: https://github.com/rust-windowing/winit/blob/master/src/platform_impl/macos/view.rs#L673 So is there a reason why this part of winit's code wasn't extended, to handle unmodified alt keys without propagation to PatchLink: https://paste.rs/Joa diff --git a/examples/window.rs b/examples/window.rs
index 783578de..e5b01b83 100644
--- a/examples/window.rs
+++ b/examples/window.rs
@@ -1,6 +1,7 @@
+use winit::platform::macos::WindowExtMacOS;
use simple_logger::SimpleLogger;
use winit::{
- event::{Event, WindowEvent},
+ event::{Event, WindowEvent, ElementState, VirtualKeyCode, KeyboardInput},
event_loop::{ControlFlow, EventLoop},
window::WindowBuilder,
};
@@ -14,6 +15,7 @@ fn main() {
.with_inner_size(winit::dpi::LogicalSize::new(128.0, 128.0))
.build(&event_loop)
.unwrap();
+ let mut option_alternative_input = true;
event_loop.run(move |event, _, control_flow| {
*control_flow = ControlFlow::Wait;
@@ -24,6 +26,25 @@ fn main() {
event: WindowEvent::CloseRequested,
window_id,
} if window_id == window.id() => *control_flow = ControlFlow::Exit,
+ Event::WindowEvent { event, .. } => match event {
+ WindowEvent::KeyboardInput {
+ input:
+ KeyboardInput {
+ virtual_keycode: Some(virtual_code),
+ state,
+ ..
+ },
+ ..
+ } => match (virtual_code, state) {
+ (VirtualKeyCode::X, ElementState::Pressed) => {
+ option_alternative_input = !option_alternative_input;
+ println!("ReceivedCharacter: TOGGLING TO: {}", option_alternative_input);
+ window.set_option_alternative_input(option_alternative_input);
+ },
+ _ => (),
+ },
+ _ => (),
+ },
Event::MainEventsCleared => {
window.request_redraw();
}
diff --git a/src/platform/macos.rs b/src/platform/macos.rs
index c96f176a..3f4c8f62 100644
--- a/src/platform/macos.rs
+++ b/src/platform/macos.rs
@@ -63,13 +63,13 @@ pub trait WindowExtMacOS {
/// Sets whether or not the window has shadow.
fn set_has_shadow(&self, has_shadow: bool);
- /// Toggles the `Option` key being interpretted as an `Alt` modifier.
+ /// Sets whether the Option key can be used for alternative keyboard input.
///
- /// This will ignore diacritical marks and accent characters from
- /// being processed as received characters. Instead, the input
- /// device's raw character will be placed in event queues with the
- /// Alt modifier set.
- fn set_option_as_alt(&self, option_as_alt: bool);
+ /// When set to `true`, the option key can be used like AltGr to input
+ /// additional characters and dead keys.
+ ///
+ /// By default, alternative keyboard input is enabled.
+ fn set_option_alternative_input(&self, option_alternative_input: bool);
}
impl WindowExtMacOS for Window {
@@ -109,8 +109,8 @@ impl WindowExtMacOS for Window {
}
#[inline]
- fn set_option_as_alt(&self, option_as_alt: bool) {
- self.window.set_option_as_alt(option_as_alt)
+ fn set_option_alternative_input(&self, option_alternative_input: bool) {
+ self.window.set_option_alternative_input(option_alternative_input)
}
}
@@ -161,7 +161,11 @@ pub trait WindowBuilderExtMacOS {
fn with_resize_increments(self, increments: LogicalSize<f64>) -> WindowBuilder;
fn with_disallow_hidpi(self, disallow_hidpi: bool) -> WindowBuilder;
fn with_has_shadow(self, has_shadow: bool) -> WindowBuilder;
- fn with_ignore_alt_modifier(self, ignore_alt_modifier: bool) -> WindowBuilder;
+
+ /// Sets whether the Option key can be used for alternative keyboard input.
+ ///
+ /// See [`WindowExtMacOs::set_option_alternative_input`] for details.
+ fn with_option_alternative_input(self, option_alternative_input: bool) -> WindowBuilder;
}
impl WindowBuilderExtMacOS for WindowBuilder {
@@ -229,8 +233,8 @@ impl WindowBuilderExtMacOS for WindowBuilder {
}
#[inline]
- fn with_ignore_alt_modifier(mut self, ignore_alt_modifier: bool) -> WindowBuilder {
- self.platform_specific.ignore_alt_modifier = ignore_alt_modifier;
+ fn with_option_alternative_input(mut self, option_alternative_input: bool) -> WindowBuilder {
+ self.platform_specific.option_alternative_input = option_alternative_input;
self
}
}
diff --git a/src/platform_impl/macos/view.rs b/src/platform_impl/macos/view.rs
index ad83db91..d61b28d1 100644
--- a/src/platform_impl/macos/view.rs
+++ b/src/platform_impl/macos/view.rs
@@ -54,9 +54,11 @@ pub(super) struct ViewState {
ns_window: id,
pub cursor_state: Arc<Mutex<CursorState>>,
ime_spot: Option<(f64, f64)>,
+ raw_characters: Option<String>,
+ is_key_down: bool,
pub(super) modifiers: ModifiersState,
tracking_rect: Option<NSInteger>,
- ignore_alt_modifier: bool,
+ option_alternative_input: bool,
}
impl ViewState {
@@ -75,9 +77,11 @@ pub fn new_view(
ns_window,
cursor_state,
ime_spot: None,
+ raw_characters: None,
+ is_key_down: false,
modifiers: Default::default(),
tracking_rect: None,
- ignore_alt_modifier: pl_attribs.ignore_alt_modifier,
+ option_alternative_input: pl_attribs.option_alternative_input,
};
unsafe {
// This is free'd in `dealloc`
@@ -101,10 +105,10 @@ pub unsafe fn set_ime_position(ns_view: id, input_context: id, x: f64, y: f64) {
let _: () = msg_send![input_context, invalidateCharacterCoordinates];
}
-pub unsafe fn set_option_as_alt(ns_view: id, option_as_alt: bool) {
+pub unsafe fn set_option_alternative_input(ns_view: id, option_alternative_input: bool) {
let state_ptr: *mut c_void = *(*ns_view).get_mut_ivar("winitState");
let state = &mut *(state_ptr as *mut ViewState);
- state.ignore_alt_modifier = option_as_alt;
+ state.option_alternative_input = option_alternative_input;
}
struct ViewClass(*const Class);
@@ -166,6 +170,10 @@ lazy_static! {
attributed_substring_for_proposed_range
as extern "C" fn(&Object, Sel, NSRange, *mut c_void) -> id,
);
+ decl.add_method(
+ sel!(insertText:replacementRange:),
+ insert_text as extern "C" fn(&Object, Sel, id, NSRange),
+ );
decl.add_method(
sel!(characterIndexForPoint:),
character_index_for_point as extern "C" fn(&Object, Sel, NSPoint) -> NSUInteger,
@@ -175,6 +183,10 @@ lazy_static! {
first_rect_for_character_range
as extern "C" fn(&Object, Sel, NSRange, *mut c_void) -> NSRect,
);
+ decl.add_method(
+ sel!(doCommandBySelector:),
+ do_command_by_selector as extern "C" fn(&Object, Sel, Sel),
+ );
decl.add_method(sel!(keyDown:), key_down as extern "C" fn(&Object, Sel, id));
decl.add_method(sel!(keyUp:), key_up as extern "C" fn(&Object, Sel, id));
decl.add_method(
@@ -493,6 +505,81 @@ extern "C" fn first_rect_for_character_range(
}
}
+extern "C" fn insert_text(this: &Object, _sel: Sel, string: id, _replacement_range: NSRange) {
+ println!("ReceivedCharacter: Insert Text");
+ trace!("Triggered `insertText`");
+ unsafe {
+ let state_ptr: *mut c_void = *this.get_ivar("winitState");
+ let state = &mut *(state_ptr as *mut ViewState);
+
+ let has_attr = msg_send![string, isKindOfClass: class!(NSAttributedString)];
+ let characters = if has_attr {
+ // This is a *mut NSAttributedString
+ msg_send![string, string]
+ } else {
+ // This is already a *mut NSString
+ string
+ };
+
+ let slice =
+ slice::from_raw_parts(characters.UTF8String() as *const c_uchar, characters.len());
+ let string = str::from_utf8_unchecked(slice);
+ state.is_key_down = true;
+
+ // We don't need this now, but it's here if that changes.
+ //let event: id = msg_send![NSApp(), currentEvent];
+
+ let mut events = VecDeque::with_capacity(characters.len());
+ for character in string.chars().filter(|c| !is_corporate_character(*c)) {
+ events.push_back(EventWrapper::StaticEvent(Event::WindowEvent {
+ window_id: WindowId(get_window_id(state.ns_window)),
+ event: WindowEvent::ReceivedCharacter(character),
+ }));
+ }
+
+ AppState::queue_events(events);
+ }
+ trace!("Completed `insertText`");
+}
+
+extern "C" fn do_command_by_selector(this: &Object, _sel: Sel, command: Sel) {
+ println!("ReceivedCharacter: Do Command");
+ trace!("Triggered `doCommandBySelector`");
+ // Basically, we're sent this message whenever a keyboard event that doesn't generate a "human readable" character
+ // happens, i.e. newlines, tabs, and Ctrl+C.
+ unsafe {
+ let state_ptr: *mut c_void = *this.get_ivar("winitState");
+ let state = &mut *(state_ptr as *mut ViewState);
+
+ let mut events = VecDeque::with_capacity(1);
+ if command == sel!(insertNewline:) {
+ // The `else` condition would emit the same character, but I'm keeping this here both...
+ // 1) as a reminder for how `doCommandBySelector` works
+ // 2) to make our use of carriage return explicit
+ events.push_back(EventWrapper::StaticEvent(Event::WindowEvent {
+ window_id: WindowId(get_window_id(state.ns_window)),
+ event: WindowEvent::ReceivedCharacter('\r'),
+ }));
+ } else {
+ let raw_characters = state.raw_characters.take();
+ if let Some(raw_characters) = raw_characters {
+ for character in raw_characters
+ .chars()
+ .filter(|c| !is_corporate_character(*c))
+ {
+ events.push_back(EventWrapper::StaticEvent(Event::WindowEvent {
+ window_id: WindowId(get_window_id(state.ns_window)),
+ event: WindowEvent::ReceivedCharacter(character),
+ }));
+ }
+ }
+ };
+
+ AppState::queue_events(events);
+ }
+ trace!("Completed `doCommandBySelector`");
+}
+
fn get_characters(event: id, ignore_modifiers: bool) -> String {
unsafe {
let characters: id = if ignore_modifiers {
@@ -563,20 +650,26 @@ fn update_potentially_stale_modifiers(state: &mut ViewState, event: id) {
}
extern "C" fn key_down(this: &Object, _sel: Sel, event: id) {
+ println!("ReceivedCharacter: DOWN DOWN DOWN");
trace!("Triggered `keyDown`");
unsafe {
let state_ptr: *mut c_void = *this.get_ivar("winitState");
let state = &mut *(state_ptr as *mut ViewState);
let window_id = WindowId(get_window_id(state.ns_window));
- let ev_mods = event_mods(event);
- let characters = get_characters(
- event,
- state.ignore_alt_modifier && ev_mods.alt() && !ev_mods.ctrl(),
- );
+ let mods = event_mods(event);
+ let ignore_modifiers = !state.option_alternative_input && mods.alt() && !mods.ctrl();
+ println!("ReceivedCharacter: Ignoring: {}", ignore_modifiers);
+ let characters = get_characters(event, ignore_modifiers);
+ println!("ReceivedCharacter: String: {}", characters);
+
+ // TODO: Do we always want with mods here?
+ state.raw_characters = Some(characters.clone());
let scancode = get_scancode(event) as u32;
let virtual_keycode = retrieve_keycode(event);
+ let is_repeat = msg_send![event, isARepeat];
+
update_potentially_stale_modifiers(state, event);
#[allow(deprecated)]
@@ -588,18 +681,35 @@ extern "C" fn key_down(this: &Object, _sel: Sel, event: id) {
state: ElementState::Pressed,
scancode,
virtual_keycode,
- modifiers: ev_mods,
+ modifiers: event_mods(event),
},
is_synthetic: false,
},
};
- AppState::queue_event(EventWrapper::StaticEvent(window_event));
- for character in characters.chars().filter(|c| !is_corporate_character(*c)) {
- AppState::queue_event(EventWrapper::StaticEvent(Event::WindowEvent {
- window_id,
- event: WindowEvent::ReceivedCharacter(character),
- }));
+ let pass_along = {
+ AppState::queue_event(EventWrapper::StaticEvent(window_event));
+ // Emit `ReceivedCharacter` directly for repeated keys and keys ignoring the modifier
+ // to skip the `input_text` processing.
+ if (is_repeat && state.is_key_down) || ignore_modifiers {
+ for character in characters.chars().filter(|c| !is_corporate_character(*c)) {
+ AppState::queue_event(EventWrapper::StaticEvent(Event::WindowEvent {
+ window_id,
+ event: WindowEvent::ReceivedCharacter(character),
+ }));
+ }
+ false
+ } else {
+ true
+ }
+ };
+
+ if pass_along {
+ // Some keys (and only *some*, with no known reason) don't trigger `insertText`, while others do...
+ // So, we don't give repeats the opportunity to trigger that, since otherwise our hack will cause some
+ // keys to generate twice as many characters.
+ let array: id = msg_send![class!(NSArray), arrayWithObject: event];
+ let _: () = msg_send![this, interpretKeyEvents: array];
}
}
trace!("Completed `keyDown`");
@@ -611,6 +721,8 @@ extern "C" fn key_up(this: &Object, _sel: Sel, event: id) {
let state_ptr: *mut c_void = *this.get_ivar("winitState");
let state = &mut *(state_ptr as *mut ViewState);
+ state.is_key_down = false;
+
let scancode = get_scancode(event) as u32;
let virtual_keycode = retrieve_keycode(event);
diff --git a/src/platform_impl/macos/window.rs b/src/platform_impl/macos/window.rs
index 598980da..5961801b 100644
--- a/src/platform_impl/macos/window.rs
+++ b/src/platform_impl/macos/window.rs
@@ -24,7 +24,7 @@ use crate::{
monitor::{self, MonitorHandle, VideoMode},
util::{self, IdRef},
view::CursorState,
- view::{self, new_view, set_option_as_alt},
+ view::{self, new_view, set_option_alternative_input},
window_delegate::new_delegate,
OsError,
},
@@ -72,7 +72,7 @@ pub struct PlatformSpecificWindowBuilderAttributes {
pub resize_increments: Option<LogicalSize<f64>>,
pub disallow_hidpi: bool,
pub has_shadow: bool,
- pub ignore_alt_modifier: bool,
+ pub option_alternative_input: bool,
}
impl Default for PlatformSpecificWindowBuilderAttributes {
@@ -89,7 +89,7 @@ impl Default for PlatformSpecificWindowBuilderAttributes {
resize_increments: None,
disallow_hidpi: false,
has_shadow: true,
- ignore_alt_modifier: false,
+ option_alternative_input: true,
}
}
}
@@ -1147,9 +1147,9 @@ impl WindowExtMacOS for UnownedWindow {
}
#[inline]
- fn set_option_as_alt(&self, option_as_alt: bool) {
+ fn set_option_alternative_input(&self, option_alternative_input: bool) {
unsafe {
- set_option_as_alt(*self.ns_view, option_as_alt);
+ set_option_alternative_input(*self.ns_view, option_alternative_input);
}
}
} |
|
@chrisduerr I've applied this patch but it looks like it reverts the fixed behavior of ignoring modifiers present in the pre-patched commit. In my use case, My test environment is this patch and |
|
@saysjonathan Did you fix all the compilation issues? I've changed a bunch of stuff in winit so Alacritty will not just compile with my patch. I've tested it with winit's |
|
Intersting, I did not have any compilation issues with Alacritty. I have @kjmph's change merged against 38fcceb, with your patch applied on top. You can see it in this branch. Here's a patch of my Alacritty changes, applied to f19cbca diff --git a/Cargo.toml b/Cargo.toml
index 7a6dec8..5bd8497 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -9,3 +9,6 @@ members = [
lto = true
debug = 1
incremental = false
+
+[patch.crates-io]
+winit = { git = "https://github.com/saysjonathan/winit", branch = "alt-fix-new" }
diff --git a/alacritty/src/window.rs b/alacritty/src/window.rs
index 418994e..e43d527 100644
--- a/alacritty/src/window.rs
+++ b/alacritty/src/window.rs
@@ -327,7 +330,8 @@ impl Window {
.with_visible(false)
.with_transparent(true)
.with_maximized(window_config.maximized())
- .with_fullscreen(window_config.fullscreen());
+ .with_fullscreen(window_config.fullscreen())
+ .with_option_alternative_input(true);
match window_config.decorations {
Decorations::Full => window,With those changes I did not experience any issues with compilation. I'll try the |
|
Doesn't work for me with the Shouldn't the |
|
I mean if you want alternative input disabled for your option, you'll have to pass |
|
Ah! Helps to fully read the code. Trying now with |
|
I get the same output in from the |
With my patched window example you can press |
|
Verified |
|
@kjmph considering that you haven't shown any activity in more than a month and you haven't responded to my previous question, I'm assuing you don't have the time to continue working on this, so @chrisduerr I'd like to ask you to take over this PR. If you can't push to this branch I'm not sure what's the best way to do it, but I'm open to suggestions. |
|
My apologies, to be more clear, I have limited time to take on this work. Please feel free to take over this PR @ArturKovacs. Let me know how I may be of service in transitioning the work to you. I think I succeeded in showing that this can be fixed on MacOS, yet as I’m not as involved in the community; I could use the help getting these changes available to the benefit of all other users. |
|
@chrisduerr It's been quite a while, but are you still interested in taking this over the finish line based on your approach in #1449 (comment)? |
|
Considering that I don't even have access to a macOS machine, I couldn't finish it up even if I was interested. |
|
Hi, I would love to help get this over the line. I'm new to winnit, but I have a mac and I am proficient in rust. What is left besides fixing the compilation errors, merging in master changes, and testing? |
|
@ayax79 See the comment history. IIRC it broke some IME stuff like emoji input, I'm not sure if my suggested patch is a proper solution but it might be. So you'll probably have to do a bunch of testing and validate that it works properly and then clean it all up. Considering this patch is ancient, it might be easier to start from scratch and just yoink the new code you need from the patches. |
|
I agree with @chrisduerr ; this patch is out of date as the underlying code was rewritten, best to start a new change lifting the pieces as needed. Let me know what I can do that's best for the project. Note, the patch did improve input handling with the Alt modifier. Releases already have IME issues and this patch doesn't degrade any capabilities. It may be easier to breakout a second issue to address IME handling, and let a fix through for the Alt handling. Cheers. |
|
I compared the patch to master. It won't take me long to reimplement. |
|
I reimplemented the fix here: https://github.com/ayax79/winit/tree/fix-alt-modifier-new I verified with a debugger that everything is working the same as the original fix. There still seems to be some oddness. For instance, I recompiled alacritty with my version. However, once I load up zellij, I still see characters. When I have a few minutes, I am going to continue debugging with showkey. |
Feel free to open a draft PR, makes it easy for others to follow your progress |
|
I'll try to by end of the day. After rereading this thread, I realized that kjmph's repo didn't have the patch from @chrisduerr. I will be implementing that patch. I'll pull request once it is in. |
|
WIP pull request. This is should have all the patches applied, isn't working yet. Still seeing characters with modifiers: #2576 |
|
Closing in favour of #2576 then. |
The complications between insertText and keyDown were causing
difficulties, especially when diacritics are used. This commit cleans
out the duplication of events and the state that needs to be captured
between event handlers.
cargo fmthas been run on this branchcargo docbuilds successfullyCHANGELOG.mdif knowledge of this change could be valuable to users