Skip to content

Use x11rb for event handling#3122

Closed
notgull wants to merge 6 commits intomasterfrom
notgull/ime
Closed

Use x11rb for event handling#3122
notgull wants to merge 6 commits intomasterfrom
notgull/ime

Conversation

@notgull
Copy link
Copy Markdown
Member

@notgull notgull commented Sep 30, 2023 •
edited
Loading

  • 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

Switches out the previous event handling logic for x11rb-based logic. The last large hurdle before getting rid of Xlib. As part of the process, also gets rid of Xlib's IME and replaces it with the xim crate.

Draft because I have not tested this in the slightest.

@daxpedda daxpedda added the DS - x11 Affects the X11 backend, or generally free Unix platforms label Sep 30, 2023
@notgull notgull added the C - waiting on testing Does this PR work? label Sep 30, 2023
@notgull notgull marked this pull request as ready for review October 8, 2023 03:57
@notgull notgull requested a review from kchibisov as a code owner October 8, 2023 03:57
@notgull
Copy link
Copy Markdown
Member Author

notgull commented Oct 8, 2023

Event handling functionality looks sound to me. I haven't tested IME yet.

@notgull
Copy link
Copy Markdown
Member Author

notgull commented Oct 14, 2023

Tested IME on Fedora 38

@notgull notgull added C - waiting on maintainer A maintainer must review this code and removed C - waiting on testing Does this PR work? labels Oct 14, 2023
@notgull notgull mentioned this pull request Oct 28, 2023
Open
@notgull
Copy link
Copy Markdown
Member Author

notgull commented Nov 8, 2023

@Riey Can you review this and make sure I'm using XIM right?

@Riey
Copy link
Copy Markdown

Riey commented Nov 9, 2023

@notgull Is there an example code to check preedit and position?

@notgull
Copy link
Copy Markdown
Member Author

notgull commented Nov 9, 2023

I'm not sure what you're asking for, but here is the code responsible for handling pre-edits:

winit/src/platform_impl/linux/x11/ime.rs

Lines 648 to 767 in 46bb9e5

fn handle_preedit_start(
&mut self,
_client: &mut C,
input_method_id: u16,
input_context_id: u16,
) -> Result<(), ClientError> {
debug_assert_eq!(self.input_method, Some(input_method_id));
if let Some(ic_data) = self.input_contexts.get_mut(&input_context_id) {
// Start a pre-edit.
ic_data.text.clear();
ic_data.cursor = 0;
// Indicate the start.
self.ime_events
.push_back((ic_data.window.id, ImeEvent::Start));
}
Ok(())
}
fn handle_preedit_draw(
&mut self,
_client: &mut C,
input_method_id: u16,
input_context_id: u16,
caret: i32,
chg_first: i32,
chg_len: i32,
_status: xim::PreeditDrawStatus,
preedit_string: &str,
_feedbacks: Vec<xim::Feedback>,
) -> Result<(), ClientError> {
debug_assert_eq!(self.input_method, Some(input_method_id));
if let Some(ic_data) = self.input_contexts.get_mut(&input_context_id) {
// Set the cursor.
ic_data.cursor = caret as usize;
// Figure out the range of text to change.
let change_range = chg_first as usize..(chg_first + chg_len) as usize;
// If the range doesn't fit our current text, warn and return.
if change_range.start > ic_data.text.len() || change_range.end > ic_data.text.len() {
warn!(
"Preedit draw range {}..{} doesn't fit text of length {}",
change_range.start,
change_range.end,
ic_data.text.len()
);
return Ok(());
}
// Update the text in the changed range.
{
let text = &mut ic_data.text;
let mut old_text_tail = text.split_off(change_range.end);
text.truncate(change_range.start);
text.extend(preedit_string.chars());
text.append(&mut old_text_tail);
}
// Send the event.
let cursor_byte_pos = calc_byte_position(&ic_data.text, ic_data.cursor);
let event = ImeEvent::Update(ic_data.text.iter().collect(), Some(cursor_byte_pos));
self.ime_events.push_back((ic_data.window.id, event));
}
Ok(())
}
fn handle_preedit_caret(
&mut self,
_client: &mut C,
input_method_id: u16,
input_context_id: u16,
position: &mut i32,
direction: xim::CaretDirection,
_style: xim::CaretStyle,
) -> Result<(), ClientError> {
// We only care about absolute position.
if matches!(direction, xim::CaretDirection::AbsolutePosition) {
debug_assert_eq!(self.input_method, Some(input_method_id));
if let Some(ic_data) = self.input_contexts.get_mut(&input_context_id) {
ic_data.cursor = *position as usize;
// Send the event
let event =
ImeEvent::Update(ic_data.text.iter().collect(), Some(*position as usize));
self.ime_events.push_back((ic_data.window.id, event));
}
}
Ok(())
}
fn handle_preedit_done(
&mut self,
_client: &mut C,
input_method_id: u16,
input_context_id: u16,
) -> Result<(), ClientError> {
debug_assert_eq!(self.input_method, Some(input_method_id));
// Get the client data.
if let Some(ic_data) = self.input_contexts.get_mut(&input_context_id) {
// We're done with a preedit.
ic_data.text.clear();
ic_data.cursor = 0;
// Send a message to the window.
let window = ic_data.window.id;
self.ime_events.push_back((window, ImeEvent::End));
}
Ok(())
}

@kchibisov
Copy link
Copy Markdown
Member

kchibisov commented Nov 9, 2023

you can also, always try to do things with alacritty if you want to test with typing experience.

@Riey
Copy link
Copy Markdown

Riey commented Nov 10, 2023

LGTM.

@notgull notgull mentioned this pull request Nov 14, 2023
Open
8 tasks
@notgull
Switch to using xim for IME
5c9f1b2 
This is broken until we start using x11rb for event lookups.

Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Use x11rb for event handling
9477514 
Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Use XSetEventQueueOwner to indicate XCB
39b5437 
I don't know what this actually does, but it can't hurt, right?

Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Remove superseded TODO comment
4a30d0e 
Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Update for 0.29.3 changes
1c65f70 
Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Update to newest master
d5240c4 
Signed-off-by: John Nunley <dev@notgull.net>
psychon
psychon reviewed Dec 20, 2023
View reviewed changes
src/platform_impl/linux/x11/xdisplay.rs
display,
x11_dl::xlib_xcb::XEventQueueOwner::XCBOwnsEventQueue,
);
}
Copy link
Copy Markdown
Contributor

@psychon psychon Dec 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Since the commit message for this one says

I don't know what this actually does, but it can't hurt, right?

"Normally", all events and errors go through Xlib. It does some gymnastics internally to ensure that before the reply for a request is handled, everything that happens before is done: Events are in its internal event queue and errors were passed to the error handler. This has the effect of removing all events/errors from XCB's queue.

Thus, normally you cannot use xcb_poll/wait_for_event() when Xlib is involved, since Xlib might already have "eaten" an event and put it into its own queue.

The call you are doing here just tells Xlib "do not mess with the event queue, kthxbye". It still will use the flag XCB_REQUEST_CHECKED flags when sending requests to that any errors caused "through" Xlib are reported to Xlib via xcb_wait_for_reply() and do not end up in XCB's event queue, but it will no longer call xcb_poll_for_event() or xcb_wait_for_event().

Copy link
Copy Markdown
Member Author

@notgull notgull Dec 20, 2023

Choose a reason for hiding this comment

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

Great, thanks for clarifying! Yeah I figured it was somehow telling Xlib to not interfere with libxcb. Glad to confirm it.

psychon
psychon suggested changes Feb 3, 2024
View reviewed changes
src/platform_impl/linux/x11/event_processor.rs
ime.set_ime_allowed(window_id, allowed);
}
X11Event::RandrNotify(_) => {
self.process_dpi_change(&mut callback);
Copy link
Copy Markdown
Contributor

@psychon psychon Feb 3, 2024

Choose a reason for hiding this comment

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

I came here for another reason, but then noticed this bit. I think this is wrong.

The old code does:

if event_type == self.randr_event_offset as c_int {

Imagine a + 0 in there and check that "event zero" from randr is RRScreenChangeNotify: https://docs.rs/x11-dl/latest/x11_dl/xrandr/constant.RRScreenChangeNotify.html

RandrNotify is event 1: https://docs.rs/x11-dl/latest/x11_dl/xrandr/constant.RRNotify.html

@notgull
Copy link
Copy Markdown
Member Author

notgull commented Feb 9, 2024

Closing as per chat discussion

@notgull notgull closed this Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kchibisov kchibisov Awaiting requested review from kchibisov

1 more reviewer

@psychon psychon psychon requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

C - waiting on maintainer A maintainer must review this code DS - x11 Affects the X11 backend, or generally free Unix platforms

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@notgull @Riey @kchibisov @psychon @daxpedda