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

Use x11rb for event handling #3122

Closed
wants to merge 6 commits into from
Closed

Use x11rb for event handling #3122

wants to merge 6 commits into from

Conversation

notgull
Copy link
Member

@notgull notgull commented Sep 30, 2023

  • 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.

@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
Copy link
Member Author

notgull commented Oct 8, 2023

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

@notgull
Copy link
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
@notgull
Copy link
Member Author

notgull commented Nov 8, 2023

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

@Riey
Copy link

Riey commented Nov 9, 2023

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

@notgull
Copy link
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:

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
Member

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

@Riey
Copy link

Riey commented Nov 10, 2023

LGTM.

This is broken until we start using x11rb for event lookups.

Signed-off-by: John Nunley <[email protected]>
Signed-off-by: John Nunley <[email protected]>
I don't know what this actually does, but it can't hurt, right?

Signed-off-by: John Nunley <[email protected]>
Signed-off-by: John Nunley <[email protected]>
Signed-off-by: John Nunley <[email protected]>
display,
x11_dl::xlib_xcb::XEventQueueOwner::XCBOwnsEventQueue,
);
}
Copy link
Contributor

@psychon psychon Dec 20, 2023

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
Member Author

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.

ime.set_ime_allowed(window_id, allowed);
}
X11Event::RandrNotify(_) => {
self.process_dpi_change(&mut callback);
Copy link
Contributor

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
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
Labels
C - waiting on maintainer A maintainer must review this code DS - x11
Development

Successfully merging this pull request may close these issues.

5 participants