madsmtm / objc2

Bindings to Apple's frameworks in Rust
https://docs.rs/objc2/
MIT License
281 stars 35 forks source link

[Question] NSMenuItem setTarget trouble #585

Closed VisualEhrmanntraut closed 2 months ago

VisualEhrmanntraut commented 2 months ago

Hello. I am migrating my project to use objc2 from objc, and I am encountering an issue. I am building a menu bar on top of an egui/winit app, and after switching to objc2, the menu bar options are greyed out. This suggests that the internal check for whether the action exists failed. I'm not sure if there's a problem with the crate or if it's my oversight (the documentation here is sparse, and my understanding of obj-c isn't that great). Here's the relevant parts of the code

#[cfg(target_os = "macos")]
static OPENING_FILE: std::sync::Mutex<bool> = std::sync::Mutex::new(false);

#[cfg(target_os = "macos")]
static SAVING_FILE: std::sync::Mutex<bool> = std::sync::Mutex::new(false);

#[cfg(target_os = "macos")]
declare_class!(
    struct PlistOxideMenu;

    unsafe impl ClassType for PlistOxideMenu {
        type Super = NSObject;
        type Mutability = MainThreadOnly;
        const NAME: &'static str = "PlistOxideMenu";
    }

    impl DeclaredClass for PlistOxideMenu {}

    unsafe impl NSObjectProtocol for PlistOxideMenu {}

    unsafe impl PlistOxideMenu {
        #[method(openingFile)]
        unsafe fn opening_file(&self) {
            *OPENING_FILE.lock().unwrap() = true;
            (*EGUI_CTX.get()).assume_init_mut().request_repaint();
        }

        #[method(savingFile)]
        unsafe fn saving_file(&self) {
            *SAVING_FILE.lock().unwrap() = true;
            (*EGUI_CTX.get()).assume_init_mut().request_repaint();
        }
    }
);

// [...]

impl PlistOxide {
    // [...]

    #[cfg(target_os = "macos")]
    unsafe fn init_global_menu(cc: &eframe::CreationContext<'_>) {
        (*EGUI_CTX.get()).write(cc.egui_ctx.clone());
        let mtm = MainThreadMarker::new().unwrap();
        let file_menu = NSMenu::initWithTitle(mtm.alloc(), ns_string!("File"));

        let target: Id<PlistOxideMenu> = unsafe { msg_send_id![mtm.alloc(), init] };

        let file_open = NSMenuItem::initWithTitle_action_keyEquivalent(
            mtm.alloc(),
            ns_string!("Open..."),
            Some(sel!(openingFile)),
            ns_string!("o"),
        );
        file_open.setTarget(Some(&target));
        file_menu.addItem(&file_open);

        file_menu.addItem(&NSMenuItem::separatorItem(mtm));

        let file_save = NSMenuItem::initWithTitle_action_keyEquivalent(
            mtm.alloc(),
            ns_string!("Save..."),
            Some(sel!(savingFile)),
            ns_string!("s"),
        );
        file_save.setTarget(Some(&target));
        file_menu.addItem(&file_save);

        let file_item = NSMenuItem::new(mtm);
        file_item.setSubmenu(Some(&file_menu));
        NSApplication::sharedApplication(mtm)
            .mainMenu()
            .unwrap()
            .addItem(&file_item);
    }

    #[must_use]
    pub fn new(cc: &eframe::CreationContext<'_>, path: Option<PathBuf>) -> Self {
        #[cfg(target_os = "macos")]
        unsafe {
            Self::init_global_menu(cc);
        }
        // [...]
    }

    // [...]
}
madsmtm commented 2 months ago

If you take a look at the -[NSMenuItem target] property, you will see that it says "weak". This means that when you call file_open.setTarget and file_save.setTarget, the menu items don't prevent the target value from being destroyed and deallocated when it goes out of scope.

The reason this worked in objc was likely due to a memory leak somewhere in your application, objc2 avoids those, but in turn you must handle stuff like this correctly.

If there is a natural place that you can store the target at the end of the scope, I'd recommend you do that, otherwise it's probably fine to leak it manually using std::mem::forget(target).

the documentation here is sparse

There's a bit more documentation on menus in here, but yeah, it definitely is limited.

VisualEhrmanntraut commented 2 months ago

Interesting how it can tell that it got deallocated instead of crashing. Calling forget on it seems to work, thanks.

madsmtm commented 2 months ago

It's kinda like target is an std::sync::Arc<PlistOxideMenu>, and NSMenuItem stores a std::sync::Weak to that, though indeed this is something that we don't often use in the Rust world.