refctor!: mark functions as unsafe and document its safety (#227)

* refctor!: mark functions as unsafe and document its safety

* fix examples

* fix doctests

* fix example in README.md

.changes/acccelerator-typo.md

@@ -1,5 +1,5 @@
 ---
-"muda": "patch"
+"muda": "minor"
 ---
 
 **Breaking change** Renamed the `acccelerator` method (which has an extra `c`) on `MenuItemBuilder`, `CheckMenuItemBuilder`, and `IconMenuItemBuilder` to `accelerator`.

.changes/unsafe.md

@@ -0,0 +1,16 @@
+---
+"muda": "minor"
+---
+
+**Breaking change** Marked a few methods with `unsafe` to better represent the safety guarantees:
+
+- `ContextMenu::show_context_menu_for_hwnd`
+- `ContextMenu::attach_menu_subclass_for_hwnd`
+- `ContextMenu::detach_menu_subclass_from_hwnd`
+- `Menu::init_for_hwnd`
+- `Menu::init_for_hwnd_with_theme`
+- `Menu::set_theme_for_hwnd`
+- `Menu::remove_for_hwnd`
+- `Menu::hide_for_hwnd`
+- `Menu::show_for_hwnd`
+- `Menu::is_visible_on_hwnd`

.changes/use-objc2.md

@@ -1,7 +1,5 @@
 ---
-"muda": patch
+"muda": minor
 ---
 
-Use `objc2` internally, leading to much better memory safety.
-
-The crate will panic now if used from a thread that is not the main thread.
+Use `objc2` internally, leading to much better memory safety. The crate will panic now if used from a thread that is not the main thread.

README.md

@@ -66,7 +66,7 @@ or use it as your global app menu on macOS
 ```rs
 // --snip--
 #[cfg(target_os = "windows")]
-menu.init_for_hwnd(window.hwnd() as isize);
+unsafe { menu.init_for_hwnd(window.hwnd() as isize) };
 #[cfg(target_os = "linux")]
 menu.init_for_gtk_window(&gtk_window, Some(&vertical_gtk_box));
 #[cfg(target_os = "macos")]
@@ -81,7 +81,7 @@ You can also use a [`Menu`] or a [`Submenu`] show a context menu.
 // --snip--
 let position = muda::PhysicalPosition { x: 100., y: 120. };
 #[cfg(target_os = "windows")]
-menu.show_context_menu_for_hwnd(window.hwnd() as isize, Some(position.into()));
+unsafe { menu.show_context_menu_for_hwnd(window.hwnd() as isize, Some(position.into())) };
 #[cfg(target_os = "linux")]
 menu.show_context_menu_for_gtk_window(&gtk_window, Some(position.into()));
 #[cfg(target_os = "macos")]

examples/tao.rs

@@ -138,7 +138,7 @@ fn main() {
     edit_m.append_items(&[&copy_i, &PredefinedMenuItem::separator(), &paste_i]);
 
     #[cfg(target_os = "windows")]
-    {
+    unsafe {
         menu_bar.init_for_hwnd(window.hwnd() as _);
         menu_bar.init_for_hwnd(window2.hwnd() as _);
     }
@@ -221,7 +221,9 @@ fn main() {
 fn show_context_menu(window: &Window, menu: &dyn ContextMenu, position: Option<Position>) {
     println!("Show context menu at position {position:?}");
     #[cfg(target_os = "windows")]
-    menu.show_context_menu_for_hwnd(window.hwnd() as _, position);
+    unsafe {
+        menu.show_context_menu_for_hwnd(window.hwnd() as _, position);
+    }
     #[cfg(target_os = "linux")]
     menu.show_context_menu_for_gtk_window(window.gtk_window().as_ref(), position);
     #[cfg(target_os = "macos")]

examples/winit.rs

@@ -131,10 +131,10 @@ fn main() {
     {
         use winit::raw_window_handle::*;
         if let RawWindowHandle::Win32(handle) = window.window_handle().unwrap().as_raw() {
-            menu_bar.init_for_hwnd(handle.hwnd.get());
+            unsafe { menu_bar.init_for_hwnd(handle.hwnd.get()) };
         }
         if let RawWindowHandle::Win32(handle) = window2.window_handle().unwrap().as_raw() {
-            menu_bar.init_for_hwnd(handle.hwnd.get());
+            unsafe { menu_bar.init_for_hwnd(handle.hwnd.get()) };
         }
     }
     #[cfg(target_os = "macos")]
@@ -206,7 +206,7 @@ fn show_context_menu(window: &Window, menu: &dyn ContextMenu, position: Option<P
     {
         use winit::raw_window_handle::*;
         if let RawWindowHandle::Win32(handle) = window.window_handle().unwrap().as_raw() {
-            menu.show_context_menu_for_hwnd(handle.hwnd.get(), position);
+            unsafe { menu.show_context_menu_for_hwnd(handle.hwnd.get(), position) };
         }
     }
     #[cfg(target_os = "macos")]

examples/wry.rs

@@ -155,7 +155,7 @@ fn main() -> wry::Result<()> {
         .unwrap();
 
     #[cfg(target_os = "windows")]
-    {
+    unsafe {
         menu_bar.init_for_hwnd(window.hwnd() as _).unwrap();
         menu_bar.init_for_hwnd(window2.hwnd() as _).unwrap();
     }
@@ -306,7 +306,9 @@ fn main() -> wry::Result<()> {
 fn show_context_menu(window: &Window, menu: &dyn ContextMenu, position: Option<Position>) {
     println!("Show context menu at position {position:?}");
     #[cfg(target_os = "windows")]
-    menu.show_context_menu_for_hwnd(window.hwnd() as _, position);
+    unsafe {
+        menu.show_context_menu_for_hwnd(window.hwnd() as _, position);
+    }
     #[cfg(target_os = "linux")]
     menu.show_context_menu_for_gtk_window(window.gtk_window().as_ref(), position);
     #[cfg(target_os = "macos")]

src/items/submenu.rs

@@ -217,19 +217,19 @@ impl ContextMenu for Submenu {
     }
 
     #[cfg(target_os = "windows")]
-    fn show_context_menu_for_hwnd(&self, hwnd: isize, position: Option<Position>) {
+    unsafe fn show_context_menu_for_hwnd(&self, hwnd: isize, position: Option<Position>) {
         self.inner
             .borrow_mut()
             .show_context_menu_for_hwnd(hwnd, position)
     }
 
     #[cfg(target_os = "windows")]
-    fn attach_menu_subclass_for_hwnd(&self, hwnd: isize) {
+    unsafe fn attach_menu_subclass_for_hwnd(&self, hwnd: isize) {
         self.inner.borrow().attach_menu_subclass_for_hwnd(hwnd)
     }
 
     #[cfg(target_os = "windows")]
-    fn detach_menu_subclass_from_hwnd(&self, hwnd: isize) {
+    unsafe fn detach_menu_subclass_from_hwnd(&self, hwnd: isize) {
         self.inner.borrow().detach_menu_subclass_from_hwnd(hwnd)
     }
 
@@ -251,11 +251,9 @@ impl ContextMenu for Submenu {
         view: *const std::ffi::c_void,
         position: Option<Position>,
     ) {
-        unsafe {
-            self.inner
-                .borrow_mut()
-                .show_context_menu_for_nsview(view, position)
-        }
+        self.inner
+            .borrow_mut()
+            .show_context_menu_for_nsview(view, position)
     }
 
     #[cfg(target_os = "macos")]

src/lib.rs

@@ -83,7 +83,7 @@
 //! # let vertical_gtk_box = gtk::Box::new(gtk::Orientation::Vertical, 0);
 //! // --snip--
 //! #[cfg(target_os = "windows")]
-//! menu.init_for_hwnd(window_hwnd);
+//! unsafe { menu.init_for_hwnd(window_hwnd) };
 //! #[cfg(target_os = "linux")]
 //! menu.init_for_gtk_window(&gtk_window, Some(&vertical_gtk_box));
 //! #[cfg(target_os = "macos")]
@@ -105,7 +105,7 @@
 //! // --snip--
 //! let position = muda::dpi::PhysicalPosition { x: 100., y: 120. };
 //! #[cfg(target_os = "windows")]
-//! menu.show_context_menu_for_hwnd(window_hwnd, Some(position.into()));
+//! unsafe { menu.show_context_menu_for_hwnd(window_hwnd, Some(position.into())) };
 //! #[cfg(target_os = "linux")]
 //! menu.show_context_menu_for_gtk_window(&gtk_window, Some(position.into()));
 //! #[cfg(target_os = "macos")]
@@ -301,26 +301,42 @@ impl Default for MenuItemType {
 pub trait ContextMenu {
     /// Get the popup [`HMENU`] for this menu.
     ///
+    /// The returned [`HMENU`] is valid as long as the `ContextMenu` is.
+    ///
     /// [`HMENU`]: windows_sys::Win32::UI::WindowsAndMessaging::HMENU
     #[cfg(target_os = "windows")]
     fn hpopupmenu(&self) -> isize;
 
     /// Shows this menu as a context menu inside a win32 window.
     ///
     /// - `position` is relative to the window top-left corner, if `None`, the cursor position is used.
+    ///
+    /// # Safety
+    ///
+    /// The `hwnd` must be a valid window HWND.
     #[cfg(target_os = "windows")]
-    fn show_context_menu_for_hwnd(&self, hwnd: isize, position: Option<dpi::Position>);
+    unsafe fn show_context_menu_for_hwnd(&self, hwnd: isize, position: Option<dpi::Position>);
 
     /// Attach the menu subclass handler to the given hwnd
     /// so you can recieve events from that window using [MenuEvent::receiver]
     ///
     /// This can be used along with [`ContextMenu::hpopupmenu`] when implementing a tray icon menu.
+    ///
+    /// # Safety
+    ///
+    /// The `hwnd` must be a valid window HWND.
     #[cfg(target_os = "windows")]
-    fn attach_menu_subclass_for_hwnd(&self, hwnd: isize);
+    unsafe fn attach_menu_subclass_for_hwnd(&self, hwnd: isize);
 
     /// Remove the menu subclass handler from the given hwnd
+    ///
+    /// The view must be a pointer to a valid `NSView`.
+    ///
+    /// # Safety
+    ///
+    /// The `hwnd` must be a valid window HWND.
     #[cfg(target_os = "windows")]
-    fn detach_menu_subclass_from_hwnd(&self, hwnd: isize);
+    unsafe fn detach_menu_subclass_from_hwnd(&self, hwnd: isize);
 
     /// Shows this menu as a context menu inside a [`gtk::Window`]
     ///
@@ -329,6 +345,8 @@ pub trait ContextMenu {
     fn show_context_menu_for_gtk_window(&self, w: &gtk::Window, position: Option<dpi::Position>);
 
     /// Get the underlying gtk menu reserved for context menus.
+    ///
+    /// The returned [`gtk::Menu`] is valid as long as the `ContextMenu` is.
     #[cfg(target_os = "linux")]
     fn gtk_context_menu(&self) -> gtk::Menu;
 

src/menu.rs

@@ -196,6 +196,10 @@ impl Menu {
 
     /// Adds this menu to a win32 window.
     ///
+    /// # Safety
+    ///
+    /// The `hwnd` must be a valid window HWND.
+    ///
     /// ##  Note about accelerators:
     ///
     /// For accelerators to work, the event loop needs to call
@@ -219,7 +223,7 @@ impl Menu {
     /// }
     /// ```
     #[cfg(target_os = "windows")]
-    pub fn init_for_hwnd(&self, hwnd: isize) -> crate::Result<()> {
+    pub unsafe fn init_for_hwnd(&self, hwnd: isize) -> crate::Result<()> {
         self.inner.borrow_mut().init_for_hwnd(hwnd)
     }
 
@@ -228,8 +232,16 @@ impl Menu {
     /// See [Menu::init_for_hwnd] for more info.
     ///
     /// Note that the theme only affects the menu bar itself and not submenus or context menu.
+    ///
+    /// # Safety
+    ///
+    /// The `hwnd` must be a valid window HWND.
     #[cfg(target_os = "windows")]
-    pub fn init_for_hwnd_with_theme(&self, hwnd: isize, theme: MenuTheme) -> crate::Result<()> {
+    pub unsafe fn init_for_hwnd_with_theme(
+        &self,
+        hwnd: isize,
+        theme: MenuTheme,
+    ) -> crate::Result<()> {
         self.inner
             .borrow_mut()
             .init_for_hwnd_with_theme(hwnd, theme)
@@ -238,14 +250,20 @@ impl Menu {
     /// Set a theme for the menu bar on this window.
     ///
     /// Note that the theme only affects the menu bar itself and not submenus or context menu.
+    ///
+    /// # Safety
+    ///
+    /// The `hwnd` must be a valid window HWND.
     #[cfg(target_os = "windows")]
-    pub fn set_theme_for_hwnd(&self, hwnd: isize, theme: MenuTheme) -> crate::Result<()> {
+    pub unsafe fn set_theme_for_hwnd(&self, hwnd: isize, theme: MenuTheme) -> crate::Result<()> {
         self.inner.borrow().set_theme_for_hwnd(hwnd, theme)
     }
 
     /// Returns The [`HACCEL`](windows_sys::Win32::UI::WindowsAndMessaging::HACCEL) associated with this menu
     /// It can be used with [`TranslateAcceleratorW`](windows_sys::Win32::UI::WindowsAndMessaging::TranslateAcceleratorW)
     /// in the event loop to enable accelerators
+    ///
+    /// The returned [`HACCEL`](windows_sys::Win32::UI::WindowsAndMessaging::HACCEL) is valid as long as the [Menu] is.
     #[cfg(target_os = "windows")]
     pub fn haccel(&self) -> isize {
         self.inner.borrow_mut().haccel()
@@ -261,8 +279,12 @@ impl Menu {
     }
 
     /// Removes this menu from a win32 window
+    ///
+    /// # Safety
+    ///
+    /// The `hwnd` must be a valid window HWND.
     #[cfg(target_os = "windows")]
-    pub fn remove_for_hwnd(&self, hwnd: isize) -> crate::Result<()> {
+    pub unsafe fn remove_for_hwnd(&self, hwnd: isize) -> crate::Result<()> {
         self.inner.borrow_mut().remove_for_hwnd(hwnd)
     }
 
@@ -276,8 +298,12 @@ impl Menu {
     }
 
     /// Hides this menu from a win32 window
+    ///
+    /// # Safety
+    ///
+    /// The `hwnd` must be a valid window HWND.
     #[cfg(target_os = "windows")]
-    pub fn hide_for_hwnd(&self, hwnd: isize) -> crate::Result<()> {
+    pub unsafe fn hide_for_hwnd(&self, hwnd: isize) -> crate::Result<()> {
         self.inner.borrow().hide_for_hwnd(hwnd)
     }
 
@@ -291,8 +317,12 @@ impl Menu {
     }
 
     /// Shows this menu on a win32 window
+    ///
+    /// # Safety
+    ///
+    /// The `hwnd` must be a valid window HWND.
     #[cfg(target_os = "windows")]
-    pub fn show_for_hwnd(&self, hwnd: isize) -> crate::Result<()> {
+    pub unsafe fn show_for_hwnd(&self, hwnd: isize) -> crate::Result<()> {
         self.inner.borrow().show_for_hwnd(hwnd)
     }
 
@@ -316,8 +346,12 @@ impl Menu {
     }
 
     /// Returns whether this menu visible on a on a win32 window
+    ///
+    /// # Safety
+    ///
+    /// The `hwnd` must be a valid window HWND.
     #[cfg(target_os = "windows")]
-    pub fn is_visible_on_hwnd(&self, hwnd: isize) -> bool {
+    pub unsafe fn is_visible_on_hwnd(&self, hwnd: isize) -> bool {
         self.inner.borrow().is_visible_on_hwnd(hwnd)
     }
 
@@ -341,19 +375,19 @@ impl ContextMenu for Menu {
     }
 
     #[cfg(target_os = "windows")]
-    fn show_context_menu_for_hwnd(&self, hwnd: isize, position: Option<Position>) {
+    unsafe fn show_context_menu_for_hwnd(&self, hwnd: isize, position: Option<Position>) {
         self.inner
             .borrow_mut()
             .show_context_menu_for_hwnd(hwnd, position)
     }
 
     #[cfg(target_os = "windows")]
-    fn attach_menu_subclass_for_hwnd(&self, hwnd: isize) {
+    unsafe fn attach_menu_subclass_for_hwnd(&self, hwnd: isize) {
         self.inner.borrow().attach_menu_subclass_for_hwnd(hwnd)
     }
 
     #[cfg(target_os = "windows")]
-    fn detach_menu_subclass_from_hwnd(&self, hwnd: isize) {
+    unsafe fn detach_menu_subclass_from_hwnd(&self, hwnd: isize) {
         self.inner.borrow().detach_menu_subclass_from_hwnd(hwnd)
     }
 
@@ -375,12 +409,9 @@ impl ContextMenu for Menu {
         view: *const std::ffi::c_void,
         position: Option<Position>,
     ) {
-        // SAFETY: Upheld by caller
-        unsafe {
-            self.inner
-                .borrow_mut()
-                .show_context_menu_for_nsview(view, position)
-        }
+        self.inner
+            .borrow_mut()
+            .show_context_menu_for_nsview(view, position)
     }
 
     #[cfg(target_os = "macos")]

src/platform_impl/macos/mod.rs

@@ -177,7 +177,7 @@ impl Menu {
         position: Option<Position>,
     ) {
         // SAFETY: Upheld by caller
-        unsafe { show_context_menu(&self.ns_menu.1, view, position) }
+        show_context_menu(&self.ns_menu.1, view, position)
     }
 
     pub fn ns_menu(&self) -> *mut std::ffi::c_void {
@@ -661,8 +661,7 @@ impl MenuChild {
         view: *const c_void,
         position: Option<Position>,
     ) {
-        // SAFETY: Upheld by caller
-        unsafe { show_context_menu(&self.ns_menu.as_ref().unwrap().1, view, position) }
+        show_context_menu(&self.ns_menu.as_ref().unwrap().1, view, position)
     }
 
     pub fn set_as_windows_menu_for_nsapp(&self) {