Replace parts of the Xlib backend with X11RB #2825

notgull · 2023-05-29T01:30:59Z

Supersedes #2614, closes #5

This PR replaces most of the core Xlib functionality used in winit with x11rb. The goal is to have a more incremental approach than #2614. I've shied away from most of the extensions for now.

Tested on all platforms changed (Tested on Linux X11. I haven't tested on the BSDs or Solaris yet).
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

notgull · 2023-05-31T04:16:16Z

This is ready now. I've avoided event processing for now, since reimplementing event processing will involve re-implementing XIM, which would be a bit too far for this PR.

kchibisov

Well, this hasn't gone far.

thread 'main' panicked at 'Failed to get WM hints reply: ConnectionError(ParseError(InvalidValue))', /home/kchibisov/src/rust/winit-workspace/fork/src/platform_impl/linux/x11/window.rs:1728:18
stack backtrace:
   0:     0x561011a2e00a - std::backtrace_rs::backtrace::libunwind::trace::ha9053a9a07ca49cb
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/../../backtrace/src/backtrace/libunwind.rs:93:5
   1:     0x561011a2e00a - std::backtrace_rs::backtrace::trace_unsynchronized::h9c2852a457ad564e
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
   2:     0x561011a2e00a - std::sys_common::backtrace::_print_fmt::h457936fbfaa0070f
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/sys_common/backtrace.rs:65:5
   3:     0x561011a2e00a - <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt::h5779d7bf7f70cb0c
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/sys_common/backtrace.rs:44:22
   4:     0x561011a567ce - core::fmt::write::h5a4baaff1bcd3eb5
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/core/src/fmt/mod.rs:1232:17
   5:     0x561011a29985 - std::io::Write::write_fmt::h4bc1f301cb9e9cce
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/io/mod.rs:1684:15
   6:     0x561011a2ddd5 - std::sys_common::backtrace::_print::h5fcdc36060f177e8
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/sys_common/backtrace.rs:47:5
   7:     0x561011a2ddd5 - std::sys_common::backtrace::print::h54ca9458b876c8bf
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/sys_common/backtrace.rs:34:9
   8:     0x561011a2f72f - std::panicking::default_hook::{{closure}}::hbe471161c7664ed6
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/panicking.rs:271:22
   9:     0x561011a2f46b - std::panicking::default_hook::ha3500da57aa4ac4f
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/panicking.rs:290:9
  10:     0x561011a2fcd8 - std::panicking::rust_panic_with_hook::h50c09d000dc561d2
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/panicking.rs:692:13
  11:     0x561011a2fbd9 - std::panicking::begin_panic_handler::{{closure}}::h9e2b2176e00e0d9c
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/panicking.rs:583:13
  12:     0x561011a2e476 - std::sys_common::backtrace::__rust_end_short_backtrace::h5739b8e512c09d02
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/sys_common/backtrace.rs:150:18
  13:     0x561011a2f8e2 - rust_begin_unwind
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/panicking.rs:579:5
  14:     0x561010d46593 - core::panicking::panic_fmt::hf33a1475b4dc5c3e
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/core/src/panicking.rs:64:14
  15:     0x561010d46a43 - core::result::unwrap_failed::hdff5465d74574b44
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/core/src/result.rs:1750:5
  16:     0x56101168637f - core::result::Result<T,E>::expect::h8ee9b60fc9639958
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/core/src/result.rs:1047:23
  17:     0x56101167a9ce - winit::platform_impl::platform::x11::window::UnownedWindow::request_user_attention::h8c3d7ad81f99d38c
                               at /home/kchibisov/src/rust/winit-workspace/fork/src/platform_impl/linux/x11/window.rs:1725:13
  18:     0x56101166a220 - winit::platform_impl::platform::Window::request_user_attention::hbbf1ecbf29e7acb9
                               at /home/kchibisov/src/rust/winit-workspace/fork/src/platform_impl/linux/mod.rs:546:33
  19:     0x561010fab8a9 - winit::window::Window::request_user_attention::h6b24f94211e43481
                               at /home/kchibisov/src/rust/winit-workspace/fork/src/window.rs:1127:9
  20:     0x561010e92b19 - alacritty::display::window::Window::set_urgent::ha77783b6fef0f020
                               at /home/kchibisov/src/rust/winit-workspace/alacritty/alacritty/src/display/window.rs:337:9
  21:     0x561010e29b2b - alacritty::event::<impl alacritty::input::Processor<alacritty::event::EventProxy,alacritty::event::ActionContext<alacritty_terminal::event_loop::Notifier,alacritty::event::EventProxy>>>::handle_event::h8ee8c73e2eccbc7c
                               at /home/kchibisov/src/rust/winit-workspace/alacritty/alacritty/src/event.rs:1319:29
  22:     0x561010e6794e - alacritty::window_context::WindowContext::handle_event::h7a98e6808134fc70
                               at /home/kchibisov/src/rust/winit-workspace/alacritty/alacritty/src/window_context.rs:457:13
  23:     0x561010da365d - alacritty::event::Processor::run::{{closure}}::h413ceac7fd4d53a8
                               at /home/kchibisov/src/rust/winit-workspace/alacritty/alacritty/src/event.rs:1566:25
  24:     0x561010d845eb - winit::platform_impl::platform::sticky_exit_callback::h746f29c87a798d4e
                               at /home/kchibisov/src/rust/winit-workspace/fork/src/platform_impl/linux/mod.rs:922:9
  25:     0x561010dea89d - winit::platform_impl::platform::x11::EventLoop<T>::run_return::single_iteration::hff297779b8e2246d
                               at /home/kchibisov/src/rust/winit-workspace/fork/src/platform_impl/linux/x11/mod.rs:433:17
  26:     0x561010deb0f7 - winit::platform_impl::platform::x11::EventLoop<T>::run_return::h310c06d11d09dccb
                               at /home/kchibisov/src/rust/winit-workspace/fork/src/platform_impl/linux/x11/mod.rs:531:27
  27:     0x561010d8392d - winit::platform_impl::platform::EventLoop<T>::run_return::h56c38db684b45681
                               at /home/kchibisov/src/rust/winit-workspace/fork/src/platform_impl/linux/mod.rs:823:56
  28:     0x561010de8caa - <winit::event_loop::EventLoop<T> as winit::platform::run_return::EventLoopExtRunReturn>::run_return::h9f413bf6c34c2bcb
                               at /home/kchibisov/src/rust/winit-workspace/fork/src/platform/run_return.rs:51:9
  29:     0x561010f416a0 - alacritty::event::Processor::run::h10d921f84948aebd
                               at /home/kchibisov/src/rust/winit-workspace/alacritty/alacritty/src/event.rs:1496:25
  30:     0x561010e941fa - alacritty::alacritty::hc725771ab9ce96b5
                               at /home/kchibisov/src/rust/winit-workspace/alacritty/alacritty/src/main.rs:189:18
  31:     0x561010e933a4 - alacritty::main::h484819d3c92aef36
                               at /home/kchibisov/src/rust/winit-workspace/alacritty/alacritty/src/main.rs:83:17
  32:     0x561010d677ab - core::ops::function::FnOnce::call_once::h95a0da61e7575432
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/core/src/ops/function.rs:250:5
  33:     0x561010fab15e - std::sys_common::backtrace::__rust_begin_short_backtrace::hb235145e4148a4f9
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/sys_common/backtrace.rs:134:18
  34:     0x561010f44351 - std::rt::lang_start::{{closure}}::h6ad83f5a54864f36
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/rt.rs:166:18
  35:     0x561011a22d7c - core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once::hd6efcd3bec896f2c
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/core/src/ops/function.rs:287:13
  36:     0x561011a22d7c - std::panicking::try::do_call::hce04e543bb1f4cbb
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/panicking.rs:487:40
  37:     0x561011a22d7c - std::panicking::try::h3342dd4e1f680968
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/panicking.rs:451:19
  38:     0x561011a22d7c - std::panic::catch_unwind::h148ce1e59ac0cee7
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/panic.rs:140:14
  39:     0x561011a22d7c - std::rt::lang_start_internal::{{closure}}::h25f9dda2057a67fe
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/rt.rs:148:48
  40:     0x561011a22d7c - std::panicking::try::do_call::h7caaaeaf9401650b
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/panicking.rs:487:40
  41:     0x561011a22d7c - std::panicking::try::he7d15285746cbbc2
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/panicking.rs:451:19
  42:     0x561011a22d7c - std::panic::catch_unwind::h89fb4f50c0301fe0
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/panic.rs:140:14
  43:     0x561011a22d7c - std::rt::lang_start_internal::h078acd489417d3c1
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/rt.rs:148:20
  44:     0x561010f4432a - std::rt::lang_start::h4aba5fff8caf7fef
                               at /rustc/84c898d65adf2f39a5a98507f1fe0ce10a2b8dbc/library/std/src/rt.rs:165:17
  45:     0x561010e9480e - main
  46:     0x7f7454e1aace - __libc_start_call_main
  47:     0x7f7454e1ab89 - __libc_start_main@@GLIBC_2.34
  48:     0x561010d46c25 - _start
  49:                0x0 - <unknown>

I'm using alacritty/alacritty#6958

Will look at other stuff later.

psychon · 2023-06-01T16:10:07Z

'Failed to get WM hints reply: ConnectionError(ParseError(InvalidValue))'

Reproducible by executing WmHints::get() against a window which does not have that property.

Diff for x11rb for a reproducer:

diff --git a/x11rb/examples/simple_window.rs b/x11rb/examples/simple_window.rs
index 4f81f99..31355b9 100644
--- a/x11rb/examples/simple_window.rs
+++ b/x11rb/examples/simple_window.rs
@@ -111,6 +111,9 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
             .as_bytes(),
     )?;
 
+    let hints = x11rb::properties::WmHints::get(conn, win_id)?.reply().unwrap();
+    println!("{:?}", hints);
+
     let reply = conn
         .get_property(false, win_id, AtomEnum::WM_NAME, AtomEnum::STRING, 0, 1024)?
         .reply()?;

Running this fails with ConnectionError(ParseError(InvalidValue)). xtrace says

000:<:0141: 24: Request(20): GetProperty delete=false(0x00) window=0x03000000 property=0x23("WM_HINTS") type=0x23("WM_HINTS") long-offset=0x00000000 long-length=0x00000009
000:>:0141:32: Reply to GetProperty: type=None(0x0) bytes-after=0x00000000

This error comes from here: https://github.com/psychon/x11rb/blob/26512622d3f0657a982252fe124b4fde59ebfe11/x11rb/src/properties.rs#L548-L549

Edit: psychon/x11rb#844

notgull · 2023-06-03T04:25:44Z

I've fixed the WmHints error for now by just not doing anything if we can't get our mitts on it. I pulled your alacritty branch and running it works on my machine with this code.

psychon · 2023-06-03T09:44:51Z

src/platform_impl/linux/x11/window.rs

-            .ignore_error();
+                .ok()
+                .and_then(|cookie| cookie.reply().ok())
+        {


Doesn't this just mean that this function doesn't do anything anymore?

Since things previously failed due to a missing WM hints property, I guess that nothing sets that property by default. So, this function also won't.

Instead of if let Some(mut wm_hints) = ...., I suggest to do let mut wm_hints = ....or_default(). That matches more closely what the old C code does (assuming I found the correct piece of code and assuming that alloc_wm_hints just calls XAllocWMHints which zero-initializes the structure):

impl XConnection { pub fn get_wm_hints( &self, window: ffi::Window, ) -> Result<XSmartPointer<'_, ffi::XWMHints>, XError> { let wm_hints = unsafe { (self.xlib.XGetWMHints)(self.display, window) }; self.check_errors()?; let wm_hints = if wm_hints.is_null() { self.alloc_wm_hints() } else { XSmartPointer::new(self, wm_hints).unwrap() }; Ok(wm_hints) }

That sounds like a plan

src/platform_impl/linux/mod.rs

kchibisov · 2023-06-30T12:23:15Z

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

+    let atoms = xconn.atoms();
+    let servers_atom = atoms[XIM_SERVERS];


I think we can merge both? Or it doesn't live long enough, because we get some sort of ref?

I just find the code xconn.atoms()[_NET_WM_WINDOW] to be very displeasing to the eye. The parentheses next to the brackets isn't very clean.

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

src/platform_impl/linux/x11/event_processor.rs

kchibisov · 2023-06-30T12:30:33Z

src/platform_impl/linux/x11/util/client_msg.rs

+        data: impl Into<xproto::ClientMessageData>,
+    ) -> Result<VoidCookie<'_>, X11Error> {
+        let event = xproto::ClientMessageEvent {


Can't we use this in window.rs instead of manually building ClientMessageEvent?

The data for WM_NET_PING has to be exactly the same, and the method exposed here doesn't allow for that. Adding that functionality would make it harder to use, I feel.

src/platform_impl/linux/x11/xdisplay.rs

src/platform_impl/linux/x11/window.rs

notgull · 2023-07-11T13:45:32Z

Rebased on the latest master!

notgull force-pushed the x11rb branch from 823a744 to f4997af Compare May 31, 2023 04:10

notgull marked this pull request as ready for review May 31, 2023 04:11

notgull requested a review from kchibisov as a code owner May 31, 2023 04:11

notgull mentioned this pull request May 31, 2023

internal: Add libxcb and replace some Xlib functions #2614

Closed

5 tasks

kchibisov requested changes Jun 1, 2023

View reviewed changes

notgull force-pushed the x11rb branch from f4997af to 21d29ea Compare June 2, 2023 03:05

psychon reviewed Jun 3, 2023

View reviewed changes

kchibisov requested changes Jun 30, 2023

View reviewed changes

notgull force-pushed the x11rb branch 2 times, most recently from c2e2d78 to 5634305 Compare July 1, 2023 01:36

notgull requested a review from kchibisov July 1, 2023 03:28

kchibisov mentioned this pull request Jul 2, 2023

Fix conversion from u32 to c_int #2919

Closed

kchibisov approved these changes Jul 11, 2023

View reviewed changes

notgull added 3 commits July 11, 2023 06:44

Replace some parts of the Xlib impl w/ x11rb

4e8fc9e

Remaining review comments

431edcb

Clamp dimension values instead of truncating

961e2da

notgull force-pushed the x11rb branch from a653b04 to 961e2da Compare July 11, 2023 13:45

kchibisov merged commit d7ec899 into rust-windowing:master Jul 12, 2023
55 checks passed

notgull deleted the x11rb branch July 12, 2023 12:39

notgull mentioned this pull request Jul 13, 2023

Implement activation tokens for X11 kchibisov/winit#4

Merged

5 tasks

notgull mentioned this pull request Aug 14, 2023

[Big endian] Linux - select_xinput_events is not endian agnostic #1880

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace parts of the Xlib backend with X11RB #2825

Replace parts of the Xlib backend with X11RB #2825

notgull commented May 29, 2023 •

edited

Loading

notgull commented May 31, 2023

kchibisov left a comment

psychon commented Jun 1, 2023 •

edited

Loading

notgull commented Jun 3, 2023

psychon Jun 3, 2023

notgull Jun 10, 2023

kchibisov Jun 30, 2023

notgull Jun 30, 2023

kchibisov Jun 30, 2023

notgull Jul 1, 2023

notgull commented Jul 11, 2023

		let atoms = xconn.atoms();
		let servers_atom = atoms[XIM_SERVERS];

Replace parts of the Xlib backend with X11RB #2825

Replace parts of the Xlib backend with X11RB #2825

Conversation

notgull commented May 29, 2023 • edited Loading

notgull commented May 31, 2023

kchibisov left a comment

Choose a reason for hiding this comment

psychon commented Jun 1, 2023 • edited Loading

notgull commented Jun 3, 2023

psychon Jun 3, 2023

Choose a reason for hiding this comment

notgull Jun 10, 2023

Choose a reason for hiding this comment

kchibisov Jun 30, 2023

Choose a reason for hiding this comment

notgull Jun 30, 2023

Choose a reason for hiding this comment

kchibisov Jun 30, 2023

Choose a reason for hiding this comment

notgull Jul 1, 2023

Choose a reason for hiding this comment

notgull commented Jul 11, 2023

notgull commented May 29, 2023 •

edited

Loading

psychon commented Jun 1, 2023 •

edited

Loading