rodrigocfd / winsafe

Windows API and GUI in safe, idiomatic Rust.
https://crates.io/crates/winsafe
MIT License
520 stars 30 forks source link

[Question] How can I set the pszText into NMLVDISPINFO correctly when handling lvn_get_disp_info event? #95

Closed QianNangong closed 1 year ago

QianNangong commented 1 year ago

I have no idea how I can do that..

rodrigocfd commented 1 year ago

Because of Rust's strict lifetime checks, you must store the strings in an external place whose lifetime extends beyond the event closure. The ovbious place is the window object itself. From the docs example, we could have:

#[derive(Clone)]
pub struct WndMain {
    lv:   gui::ListView,
    strs: Vec<WString>,
}

This would allow us to have something like this:

let self2 = self.clone();
lv.on().lvn_get_disp_info(|di| {
    let item_idx = di.item.iItem as usize;
    di.item.set_pszText(self2.ee.get_mut(item_idx));
    Ok(())
});

Just keep in mind that LVN_GETDISPINFO is tricky notification, because it's wildly unsafe. It may be hard to get it right.

QianNangong commented 1 year ago

Umm.. I need to share the Vec between each event handler, but if I use Rc or Arc, the lifetime will not meet.

rodrigocfd commented 1 year ago

but if I use Rc or Arc, the lifetime will not meet

If will if you clone it into the closure.

QianNangong commented 1 year ago
image

Nope

rodrigocfd commented 1 year ago

This question is tricker than I thought. The root cause is that, in C++, you pass a pointer to a string allocated somewhere else, and Rust's borrow checker goes crazy with such things.

The best I could come up with is to use an unsafe block to bypass the borrow checker:

let self2 = self.clone();
self.lv.on().lvn_get_disp_info(move |di| {
    let item_idx = di.item.iItem as usize;
    let mut strs = self2.strs.borrow_mut();
    let str_mut = strs.get_mut(item_idx).unwrap();
    let str_ptr = str_mut as *mut w::WString;
    di.item.set_pszText(Some(unsafe { &mut *str_ptr }));
    Ok(())
});

I can't test this right now, there's a risk of invalid memory access somehow (just like in C++). Please let me know if this approach works for you.

QianNangong commented 1 year ago

It does work but umm... If I enable LTO in the build, the code won't work.

rodrigocfd commented 1 year ago

Can you post your code here, so I can try to reproduce this?

QianNangong commented 1 year ago

main.rs

use std::{cell::RefCell, rc::Rc};

use winsafe::{
    co,
    gui::{self, Horz, Vert},
    msg,
    prelude::*,
    AnyResult, WString,
};

#[derive(Clone)]
struct DemoWindow {
    window: gui::WindowMain,
    list: gui::ListView,
    insert_button: gui::Button,
    items: Rc<RefCell<Vec<WString>>>,
}

impl DemoWindow {
    fn new() -> Self {
        let window = gui::WindowMain::new(gui::WindowMainOpts {
            title: "Demo Window".to_owned(),
            size: (1024, 768),
            ..Default::default()
        });
        let list = gui::ListView::new(
            &window,
            gui::ListViewOpts {
                position: (0, 68),
                size: (1024, 700),
                list_view_style: co::LVS::REPORT
                    | co::LVS::NOSORTHEADER
                    | co::LVS::SHOWSELALWAYS
                    | co::LVS::SHAREIMAGELISTS
                    | co::LVS::OWNERDATA,
                columns: vec![("Column 1".to_owned(), 600)],
                resize_behavior: (Horz::Resize, Vert::Resize),
                ..Default::default()
            },
        );
        let insert_button = gui::Button::new(
            &window,
            gui::ButtonOpts {
                text: "Insert new item".to_owned(),
                position: (0, 0),
                width: 100,
                ..Default::default()
            },
        );
        let demo_window = DemoWindow {
            window,
            list,
            insert_button,
            items: Rc::new(RefCell::new(Vec::new())),
        };

        demo_window.handle_events();

        demo_window
    }

    fn handle_events(&self) {
        let self_cloned = self.clone();
        self.insert_button.on().bn_clicked(move || {
            let count = self_cloned.list.items().count();
            self_cloned
                .items
                .borrow_mut()
                .push(WString::from_str(count.to_string()));
            self_cloned
                .list
                .hwnd()
                .SendMessage(msg::lvm::SetItemCount {
                    count: count + 1,
                    behavior: co::LVSICF::NoValue,
                })?;
            Ok(())
        });

        let self_cloned = self.clone();
        self.list.on().lvn_get_disp_info(move |info| {
            let index = info.item.iItem as usize;
            let mut strs = self_cloned.items.borrow_mut();
            let str_mut = strs.get_mut(index).unwrap();
            let ptr = str_mut as *mut WString;
            info.item.set_pszText(Some(unsafe { &mut *ptr }));
            Ok(())
        });
    }

    fn run(&self) -> AnyResult<i32> {
        self.window.run_main(None)
    }
}

fn main() {
    let app = DemoWindow::new();
    let _ = app.run();
}

Cargo.toml

[package]
name = "virtual-list-demo"
version = "0.1.0"
edition = "2021"

[dependencies]
winsafe = { git = "https://github.com/rodrigocfd/winsafe", features = [ "gui" ] }

[profile.release]
opt-level = 3
lto = true        # <- comment out this line otherwise nothing will be shown in the list view
rodrigocfd commented 1 year ago

According to this article, we must copy the characters to the buffer pointed by LVITEM, instead of setting the pointer. Until now this was not possible, because the pointer was protected.

So I implemented a new method, raw_pszText, which returns the raw pointer. With it, we can write the following:

let self_cloned = self.clone();
self.list.on().lvn_get_disp_info(move |info| {
    if info.item.mask.has(co::LVIF::TEXT) { // is this a text request?
        let index = info.item.iItem as usize;
        let strs = self_cloned.items.borrow();
        let str_ref = strs.get(index).unwrap(); // string for the requested item

        let (ptr, cch) = info.item.raw_pszText(); // retrieve raw pointer
        let out_slice = unsafe { std::slice::from_raw_parts_mut(ptr, cch as _) };
        out_slice.iter_mut()
            .zip(str_ref.as_slice())
            .for_each(|(dest, src)| *dest = *src); // copy from our string to their buffer
    }
    Ok(())
});

Let me know if this works for you.

Note that you don't necessarily need to keep a Vec<WString>. If more practical, you can keep a Vec<String> instead, and perform the conversion String -> WString when processing lvn_get_disp_info.

QianNangong commented 1 year ago

Yes, it works, thank you!

QianNangong commented 1 year ago

I reference comctl32 implements from Wine and some blahblah source codes from some blahblah Microsoft partners, both of them will allocate buffer to pszText with cchTextMax * sizeof(WCHAR) length and then free it after use, so it's safe to copy string to pszText. It's so weird the MSDN document is misleading... But if WString is much larger than cchTextMax, it may overflow...

rodrigocfd commented 1 year ago

I made a small improvement for the LVM_SETITEMCOUNT message, which now can be sent straight from the control itself.

Instead of:

self_cloned
    .list
    .hwnd()
    .SendMessage(msg::lvm::SetItemCount {
        count: count + 1,
        behavior: co::LVSICF::NoValue,
    })?;

you can do just:

self_cloned.list.items()
    .set_item_count(count + 1, None);
QianNangong commented 1 year ago

Neat!

rodrigocfd commented 1 year ago

But if WString is much larger than cchTextMax, it may overflow...

In my example it won't, because I'm copying with zip.

QianNangong commented 1 year ago

Won't there be something other than \0 in the last element of out_slice if str_ref is too long?

rodrigocfd commented 1 year ago

Minor nitpick: the method is called set_count, not set_item_count.

Anyway,

Won't there be something other than \0 in the last element of out_slice if str_ref is too long?

yes, indeed. I've ran into this in the past, so I forgot I already implemented a safe way to do this, the WString::copy_to_slice method, which takes care of the terminating null.

So, instead of

out_slice.iter_mut()
    .zip(str_ref.as_slice())
    .for_each(|(dest, src)| *dest = *src); // copy from our string to their buffer

just use

str_ref.copy_to_slice(out_slice); // copy from our string to their buffer
QianNangong commented 1 year ago

okay...

QianNangong commented 1 year ago

Umm..Will there be a safer method to set text?

rodrigocfd commented 1 year ago

Umm..Will there be a safer method to set text?

Even if I write a wrapper to set the text, we don't own that pointer (Windows does), so it could still trigger UB, so it would have to be unsafe as well.

The only way I see to provide a safe wrapper would be to add another level of wrappers over all events. That would be very time-consuming and potentially slower, so I don't really think it's a good idea for now.