Skip to content

fix(macos): replace raw MenuChild pointer with Rc #361

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.

Sign up for GitHub

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

Open
divanshu-go wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

fix(macos): replace raw MenuChild pointer with Rc #361

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 37 additions & 33 deletions src/platform_impl/macos/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ use std::{
cell::{Cell, RefCell},
collections::HashMap,
ffi::c_void,
ptr,
rc::Rc,
};

Expand Down Expand Up @@ -804,7 +803,7 @@ impl MenuChild {
let id = COUNTER.next();

for item in self.children.as_ref().unwrap() {
let ns_item = item.borrow_mut().make_ns_item_for_menu(id)?;
let ns_item = item.borrow_mut().make_ns_item_for_menu(item.clone(), id)?;
ns_submenu.addItem(&ns_item);
}

Expand All @@ -825,6 +824,7 @@ impl MenuChild {

pub fn create_ns_item_for_menu_item(
&mut self,
owner: Rc<RefCell<MenuChild>>,
menu_id: u32,
) -> crate::Result<Retained<NSMenuItem>> {
let mtm = MainThreadMarker::new().expect("can only create menu item on the main thread");
Expand All @@ -837,13 +837,11 @@ impl MenuChild {

unsafe {
ns_menu_item.setTarget(Some(&ns_menu_item));

// Store a raw pointer to the `MenuChild` as an instance variable on the native menu item
ns_menu_item.ivars().set(&*self);

ns_menu_item.setEnabled(self.enabled);
}

ns_menu_item.ivars().replace(Some(owner));

self.ns_menu_items
.entry(menu_id)
.or_default()
Expand All @@ -854,6 +852,7 @@ impl MenuChild {

pub fn create_ns_item_for_predefined_menu_item(
&mut self,
owner: Rc<RefCell<MenuChild>>,
menu_id: u32,
) -> crate::Result<Retained<NSMenuItem>> {
let mtm = MainThreadMarker::new().expect("can only create menu item on the main thread");
Expand All @@ -867,10 +866,8 @@ impl MenuChild {
if let PredefinedMenuItemType::About(_) = item_type {
unsafe {
ns_menu_item.setTarget(Some(&ns_menu_item));

// Store a raw pointer to the `MenuChild` as an instance variable on the native menu item
ns_menu_item.ivars().set(&*self);
}
ns_menu_item.ivars().replace(Some(owner));
}

Retained::into_super(ns_menu_item)
Expand Down Expand Up @@ -898,6 +895,7 @@ impl MenuChild {

pub fn create_ns_item_for_check_menu_item(
&mut self,
owner: Rc<RefCell<MenuChild>>,
menu_id: u32,
) -> crate::Result<Retained<NSMenuItem>> {
let mtm = MainThreadMarker::new().expect("can only create menu item on the main thread");
Expand All @@ -910,16 +908,14 @@ impl MenuChild {

unsafe {
ns_menu_item.setTarget(Some(&ns_menu_item));

// Store a raw pointer to the `MenuChild` as an instance variable on the native menu item
ns_menu_item.ivars().set(&*self);

ns_menu_item.setEnabled(self.enabled);
if self.checked.get() {
ns_menu_item.setState(NSControlStateValueOn);
}
}

ns_menu_item.ivars().replace(Some(owner));

self.ns_menu_items
.entry(menu_id)
.or_default()
Expand All @@ -930,6 +926,7 @@ impl MenuChild {

pub fn create_ns_item_for_icon_menu_item(
&mut self,
owner: Rc<RefCell<MenuChild>>,
menu_id: u32,
) -> crate::Result<Retained<NSMenuItem>> {
let mtm = MainThreadMarker::new().expect("can only create menu item on the main thread");
Expand All @@ -942,10 +939,6 @@ impl MenuChild {

unsafe {
ns_menu_item.setTarget(Some(&ns_menu_item));

// Store a raw pointer to the `MenuChild` as an instance variable on the native menu item
ns_menu_item.ivars().set(&*self);

ns_menu_item.setEnabled(self.enabled);

if self.icon.is_some() {
Expand All @@ -955,6 +948,8 @@ impl MenuChild {
}
}

ns_menu_item.ivars().replace(Some(owner));

self.ns_menu_items
.entry(menu_id)
.or_default()
Expand All @@ -963,13 +958,19 @@ impl MenuChild {
Ok(Retained::into_super(ns_menu_item))
}

fn make_ns_item_for_menu(&mut self, menu_id: u32) -> crate::Result<Retained<NSMenuItem>> {
fn make_ns_item_for_menu(
&mut self,
owner: Rc<RefCell<MenuChild>>,
menu_id: u32,
) -> crate::Result<Retained<NSMenuItem>> {
match self.item_type {
MenuItemType::Submenu => self.create_ns_item_for_submenu(menu_id),
MenuItemType::MenuItem => self.create_ns_item_for_menu_item(menu_id),
MenuItemType::Predefined => self.create_ns_item_for_predefined_menu_item(menu_id),
MenuItemType::Check => self.create_ns_item_for_check_menu_item(menu_id),
MenuItemType::Icon => self.create_ns_item_for_icon_menu_item(menu_id),
MenuItemType::MenuItem => self.create_ns_item_for_menu_item(owner, menu_id),
MenuItemType::Predefined => {
self.create_ns_item_for_predefined_menu_item(owner, menu_id)
}
MenuItemType::Check => self.create_ns_item_for_check_menu_item(owner, menu_id),
MenuItemType::Icon => self.create_ns_item_for_icon_menu_item(owner, menu_id),
}
}
}
Expand Down Expand Up @@ -1005,19 +1006,22 @@ impl dyn IsMenuItem + '_ {
fn make_ns_item_for_menu(&self, menu_id: u32) -> crate::Result<Retained<NSMenuItem>> {
match self.kind() {
MenuItemKind::Submenu(i) => i.inner.borrow_mut().create_ns_item_for_submenu(menu_id),
MenuItemKind::MenuItem(i) => i.inner.borrow_mut().create_ns_item_for_menu_item(menu_id),
MenuItemKind::MenuItem(i) => i
.inner
.borrow_mut()
.create_ns_item_for_menu_item(i.inner.clone(), menu_id),
MenuItemKind::Predefined(i) => i
.inner
.borrow_mut()
.create_ns_item_for_predefined_menu_item(menu_id),
.create_ns_item_for_predefined_menu_item(i.inner.clone(), menu_id),
MenuItemKind::Check(i) => i
.inner
.borrow_mut()
.create_ns_item_for_check_menu_item(menu_id),
.create_ns_item_for_check_menu_item(i.inner.clone(), menu_id),
MenuItemKind::Icon(i) => i
.inner
.borrow_mut()
.create_ns_item_for_icon_menu_item(menu_id),
.create_ns_item_for_icon_menu_item(i.inner.clone(), menu_id),
}
}
}
Expand All @@ -1026,8 +1030,7 @@ define_class!(
#[unsafe(super(NSMenuItem))]
#[name = "MudaMenuItem"]
#[thread_kind = MainThreadOnly]
// FIXME: Use `Rc` or something else to access the MenuChild.
#[ivars = Cell<*const MenuChild>]
#[ivars = RefCell<Option<Rc<RefCell<MenuChild>>>>]
struct MenuItem;

impl MenuItem {
Expand All @@ -1045,18 +1048,19 @@ impl MenuItem {
action: Option<Sel>,
key_equivalent: &NSString,
) -> Retained<Self> {
let this = mtm.alloc().set_ivars(Cell::new(ptr::null()));
let this = mtm.alloc().set_ivars(RefCell::new(None));
unsafe {
msg_send![super(this), initWithTitle: title, action: action, keyEquivalent: key_equivalent]
}
}

fn fire_menu_item_click(&self) {
let mtm = MainThreadMarker::from(self);
// Create a reference to the `MenuChild` from the raw pointer
// stored as an instance variable on the native menu item
let item =
unsafe { self.ivars().get().as_ref() }.expect("MenuItem's MenuChild pointer was unset");
let item = self.ivars().borrow();
let item = item
.as_ref()
.expect("MenuItem's MenuChild pointer was unset")
.borrow();

if let Some(PredefinedMenuItemType::About(about_meta)) = &item.predefined_item_type {
match about_meta {
Expand Down
Loading