Skip to content

Fix X11 KeyboardInput events sending with ibus input method. #2182

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

Closed
ashdnazg wants to merge 2 commits into from
Closed

Fix X11 KeyboardInput events sending with ibus input method. #2182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3026356
extract update_modifiers to a function.
ashdnazg Feb 4, 2022
53a0571
Fix X11 KeyboardInput events sending with ibus input method.
ashdnazg Feb 4, 2022
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
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ And please only add new entries to the top of this list, right below the `# Unre

# Unreleased

- On X11, fix for repeated event loop iteration when `ControlFlow` was `Wait`
- On X11, fix for repeated event loop iteration when `ControlFlow` was `Wait`.
- On X11, fix KeyboardInput events sending with ibus input method.
- On Wayland, report unaccelerated mouse deltas in `DeviceEvent::MouseMotion`.
- **Breaking:** Bump `ndk` version to 0.6, ndk-sys to `v0.3`, `ndk-glue` to `0.6`.
- Remove no longer needed `WINIT_LINK_COLORSYNC` environment variable.
Expand Down
243 changes: 154 additions & 89 deletions src/platform_impl/linux/x11/event_processor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use super::{
XExtension,
};

use util::modifiers::{ModifierKeyState, ModifierKeymap};
use util::modifiers::{Modifier, ModifierKeyState, ModifierKeymap};

use crate::{
dpi::{PhysicalPosition, PhysicalSize},
Expand All @@ -37,6 +37,7 @@ pub(super) struct EventProcessor<T: 'static> {
pub(super) first_touch: Option<u64>,
// Currently focused window belonging to this process
pub(super) active_window: Option<ffi::Window>,
pub(super) key_event_times: [ffi::Time; 256],
Copy link

@zesterer zesterer Mar 18, 2023

Choose a reason for hiding this comment

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

Perhaps 256 - KEYCODE_OFFSET given that we know the last 8 entries will never be used?

}

impl<T: 'static> EventProcessor<T> {
Expand Down Expand Up @@ -129,30 +130,21 @@ impl<T: 'static> EventProcessor<T> {
})
}
{
if xev.get_type() == ffi::KeyPress || xev.get_type() == ffi::KeyRelease {
Self::handle_key_event(
xev,
true,
wt,
&self.mod_keymap,
&mut self.key_event_times,
&mut self.device_mod_state,
self.active_window,
&mut callback,
);
}
return;
}

// We can't call a `&mut self` method because of the above borrow,
// so we use this macro for repeated modifier state updates.
macro_rules! update_modifiers {
( $state:expr , $modifier:expr ) => {{
match ($state, $modifier) {
(state, modifier) => {
if let Some(modifiers) =
self.device_mod_state.update_state(&state, modifier)
{
if let Some(window_id) = self.active_window {
callback(Event::WindowEvent {
window_id: mkwid(window_id),
event: WindowEvent::ModifiersChanged(modifiers),
});
}
}
}
}
}};
}

let event_type = xev.get_type();
match event_type {
ffi::MappingNotify => {
Expand Down Expand Up @@ -545,71 +537,16 @@ impl<T: 'static> EventProcessor<T> {
}

ffi::KeyPress | ffi::KeyRelease => {
use crate::event::ElementState::{Pressed, Released};

// Note that in compose/pre-edit sequences, this will always be Released.
let state = if xev.get_type() == ffi::KeyPress {
Pressed
} else {
Released
};

let xkev: &mut ffi::XKeyEvent = xev.as_mut();

let window = xkev.window;
let window_id = mkwid(window);

// Standard virtual core keyboard ID. XInput2 needs to be used to get a reliable
// value, though this should only be an issue under multiseat configurations.
let device = util::VIRTUAL_CORE_KEYBOARD;
let device_id = mkdid(device);
let keycode = xkev.keycode;

// When a compose sequence or IME pre-edit is finished, it ends in a KeyPress with
// a keycode of 0.
if keycode != 0 {
let scancode = keycode - KEYCODE_OFFSET as u32;
let keysym = wt.xconn.lookup_keysym(xkev);
let virtual_keycode = events::keysym_to_element(keysym as c_uint);

update_modifiers!(
ModifiersState::from_x11_mask(xkev.state),
self.mod_keymap.get_modifier(xkev.keycode as ffi::KeyCode)
);

let modifiers = self.device_mod_state.modifiers();

#[allow(deprecated)]
callback(Event::WindowEvent {
window_id,
event: WindowEvent::KeyboardInput {
device_id,
input: KeyboardInput {
state,
scancode,
virtual_keycode,
modifiers,
},
is_synthetic: false,
},
});
}

if state == Pressed {
let written = if let Some(ic) = wt.ime.borrow().get_context(window) {
wt.xconn.lookup_utf8(ic, xkev)
} else {
return;
};

for chr in written.chars() {
let event = Event::WindowEvent {
window_id,
event: WindowEvent::ReceivedCharacter(chr),
};
callback(event);
}
}
Self::handle_key_event(
xev,
false,
wt,
&self.mod_keymap,
&mut self.key_event_times,
&mut self.device_mod_state,
self.active_window,
&mut callback,
);
}

ffi::GenericEvent => {
Expand Down Expand Up @@ -645,7 +582,13 @@ impl<T: 'static> EventProcessor<T> {
}

let modifiers = ModifiersState::from_x11(&xev.mods);
update_modifiers!(modifiers, None);
Self::update_modifiers(
&mut self.device_mod_state,
self.active_window,
modifiers,
None,
&mut callback,
);

let state = if xev.evtype == ffi::XI_ButtonPress {
Pressed
Expand Down Expand Up @@ -722,7 +665,13 @@ impl<T: 'static> EventProcessor<T> {
let new_cursor_pos = (xev.event_x, xev.event_y);

let modifiers = ModifiersState::from_x11(&xev.mods);
update_modifiers!(modifiers, None);
Self::update_modifiers(
&mut self.device_mod_state,
self.active_window,
modifiers,
None,
&mut callback,
);

let cursor_moved = self.with_window(xev.event, |window| {
let mut shared_state_lock = window.shared_state.lock();
Expand Down Expand Up @@ -1225,6 +1174,122 @@ impl<T: 'static> EventProcessor<T> {
}
}

fn update_modifiers<F>(
device_mod_state: &mut ModifierKeyState,
active_window: Option<ffi::Window>,
state: ModifiersState,
modifier: Option<Modifier>,
callback: &mut F,
) where
F: FnMut(Event<'_, T>),
{
if let Some(modifiers) = device_mod_state.update_state(&state, modifier) {
if let Some(window_id) = active_window {
callback(Event::WindowEvent {
window_id: mkwid(window_id),
event: WindowEvent::ModifiersChanged(modifiers),
});
}
}
}

fn handle_key_event<F>(
xev: &mut ffi::XEvent,
filtered_by_im: bool,
wt: &super::EventLoopWindowTarget<T>,
mod_keymap: &ModifierKeymap,
key_event_times: &mut [ffi::Time; 256],
device_mod_state: &mut ModifierKeyState,
active_window: Option<ffi::Window>,
callback: &mut F,
) where
F: FnMut(Event<'_, T>),
{
use crate::event::ElementState::{Pressed, Released};

// Note that in compose/pre-edit sequences, this will always be Released.
let state = if xev.get_type() == ffi::KeyPress {
Pressed
} else {
Released
};

let xkev: &mut ffi::XKeyEvent = xev.as_mut();

let window = xkev.window;
let window_id = mkwid(window);

// Standard virtual core keyboard ID. XInput2 needs to be used to get a reliable
// value, though this should only be an issue under multiseat configurations.
let device = util::VIRTUAL_CORE_KEYBOARD;
let device_id = mkdid(device);
let keycode = xkev.keycode;

// When a compose sequence or IME pre-edit is finished, it ends in a KeyPress with
// a keycode of 0.
if keycode != 0 {
let signed_time = xkev.time as i64;
let time_since_previous_event =
signed_time.wrapping_sub(key_event_times[keycode as usize] as i64);

Copy link
Contributor

@rib rib May 29, 2022

Choose a reason for hiding this comment

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

this look suspicious - casting the new time and the time that it's being compared with into i64 values will defeat the purpose of using wrapping_sub to account for the 32bit timestamp wrapping around I think?

// An event is a replay of a previous event if it:
// 1) Isn't filtered by the IM.
// 2) Isn't the first time that an event with this keycode arrives.
// 3) Has a timestamp earlier or equal to the latest non-replayed event with this keycode.
Copy link
Contributor

@rib rib May 29, 2022

Choose a reason for hiding this comment

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

could maybe also be good for future readers to have some comments about why it's necessary to filter out these repeat events. This is kind of a hacky workaround really, so it feels like that should be really clearly sign posted in the code.

let replayed_event = !filtered_by_im
&& time_since_previous_event != signed_time
&& time_since_previous_event <= 0;

if !replayed_event {
key_event_times[keycode as usize] = xkev.time;
Copy link
Contributor

@rib rib May 29, 2022

Choose a reason for hiding this comment

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

considering the wrapping_sub arithmetic earlier, I guess this should be more careful to preserve the 32bit size of the X event timestamp. I guess ffi::Time is the same a time_t which I think can be 8 bytes on 64bit platforms. Could be cast back again later but still, just seems like it's important to be clear about the size if using wrapping arithmetic.

let scancode = keycode - KEYCODE_OFFSET as u32;
let keysym = wt.xconn.lookup_keysym(xkev);
let virtual_keycode = events::keysym_to_element(keysym as c_uint);

Self::update_modifiers(
device_mod_state,
active_window,
ModifiersState::from_x11_mask(xkev.state),
mod_keymap.get_modifier(xkev.keycode as ffi::KeyCode),
callback,
);

let modifiers = device_mod_state.modifiers();

#[allow(deprecated)]
callback(Event::WindowEvent {
window_id,
event: WindowEvent::KeyboardInput {
device_id,
input: KeyboardInput {
state,
scancode,
virtual_keycode,
modifiers,
},
is_synthetic: false,
},
});
}
}

if state == Pressed && !filtered_by_im {
let written = if let Some(ic) = wt.ime.borrow().get_context(window) {
wt.xconn.lookup_utf8(ic, xkev)
} else {
return;
};

for chr in written.chars() {
let event = Event::WindowEvent {
window_id,
event: WindowEvent::ReceivedCharacter(chr),
};
callback(event);
}
}
Comment on lines +1276 to +1290
Copy link

@zesterer zesterer Mar 18, 2023

Choose a reason for hiding this comment

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

This early return feels a little easy to break in a future refactor. Perhaps something a bit less brittle like the following?

if state == Pressed && !filtered_by_im {
    if let Some(ic) = wt.ime.borrow().get_context(window) {
        let written = wt.xconn.lookup_utf8(ic, xkev);
        
        for chr in written.chars() {
            let event = Event::WindowEvent {
                window_id,
                event: WindowEvent::ReceivedCharacter(chr),
            };
            callback(event);
        }
    }
}

}

fn handle_pressed_keys<F>(
wt: &super::EventLoopWindowTarget<T>,
window_id: crate::window::WindowId,
Expand Down
1 change: 1 addition & 0 deletions src/platform_impl/linux/x11/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ impl<T: 'static> EventLoop<T> {
num_touch: 0,
first_touch: None,
active_window: None,
key_event_times: [0; 256],
};

// Register for device hotplug events
Expand Down