emoon / rust_minifb

Cross platfrom window and framebuffer crate for Rust
MIT License
980 stars 92 forks source link

Window::set_title no longer works on Linux since minifb 0.24 #322

Closed royaltm closed 12 months ago

royaltm commented 12 months ago

How to recreate: on Linux (X11 or Wayland) modify the example noise.rb adding somewhere below the line with let mut window = Window::new(...:

window.set_title("Foo bar!");

and run the example cargo run --example noise.

Expected behavior: The window title should be "Foo bar!" Actual behavior: The window title remains: "Noise Test - Press ESC to exit"

I reverted to minifb 0.23 and the modified noise.rb example with set_title works fine. No problem on Windows with minifb 0.24 either.

Just in case, my Linux os is:

$ cat /etc/os-release
NAME="Pop!_OS"
VERSION="22.04 LTS"
ID=pop
ID_LIKE="ubuntu debian"
PRETTY_NAME="Pop!_OS 22.04 LTS"
VERSION_ID="22.04"
VERSION_CODENAME=jammy
UBUNTU_CODENAME=jammy
LOGO=distributor-logo-pop-os
royaltm commented 12 months ago

I bisected the latest commits (didn't take too long), and it seems that the commit that was supposed to fix UTF-8 names on x11 somehow broke it. (At least on my box). https://github.com/emoon/rust_minifb/commit/3da7b7b22e3686b20803c100da9c9dec53ec2f3a

royaltm commented 12 months ago

Ok, I know how to fix it.

The same XChangeProperty treatment is required to be inserted [here]: https://github.com/emoon/rust_minifb/blob/3da7b7b22e3686b20803c100da9c9dec53ec2f3a/src/os/posix/x11.rs#L597

    pub fn set_title(&mut self, title: &str) {
        match CString::new(title) {
            Err(_) => {
                println!("Unable to convert {} to c_string", title);
            }

            Ok(t) => unsafe {
                (self.d.lib.XStoreName)(self.d.display, self.handle, t.as_ptr());
                if let Ok(name_len) = c_int::try_from(t.to_bytes().len()) {
                    let net_wm_name = self.d.intern_atom("_NET_WM_NAME", false);
                    let utf8_string = self.d.intern_atom("UTF8_STRING", false);
                    (self.d.lib.XChangeProperty)(
                        self.d.display,
                        self.handle,
                        net_wm_name,
                        utf8_string,
                        8,
                        xlib::PropModeReplace,
                        t.as_ptr() as *const c_uchar,
                        name_len,
                    );
                } else {
                    println!("Window name too long");
                }
            },
        };
    }

Inserting the above fixes it.

Considering the size of the snippet I think it deserves the separate function called from both places.

Do you want a PR?

emoon commented 12 months ago

Hey! Yes, a PR would be great. Thanks for looking into the issue.

royaltm commented 12 months ago

Not sure if I can create a regression test case for that. Perhaps an example with set_title? That would require manual check though, but at least there will be some trace of an issue.

royaltm commented 12 months ago

I can modify an example title_cursor, to change a title along with the cursor.

emoon commented 12 months ago

Adding an example like you suggested sounds good to me.