refactor(gtk): accept a container param instead of creating it (#75)

* refactor(gtk): accept a container param instead of creating it

* fix build

* fix clippy

.changes/gtk-optional-container.md

@@ -0,0 +1,5 @@
+---
+"muda": "minor"
+---
+
+Changed `Menu::init_for_gtk_window` to accept a second argument for the container to which the menu bar should be added, if none was provided it will add it to the window directly. The method will no longer create a `gtk::Box` and append it to the window, instead you should add the box to the window yourself, then pass a reference to it the method so it can be used as the container for the menu bar.

.github/workflows/clippy-fmt.yml

@@ -11,8 +11,8 @@ on:
   pull_request:
 
 concurrency:
-   group: ${{ github.workflow }}-${{ github.ref }}
+  group: ${{ github.workflow }}-${{ github.ref }}
-   cancel-in-progress: true
+  cancel-in-progress: true
 
 jobs:
   clippy:
@@ -22,7 +22,7 @@ jobs:
       - name: install system deps
         run: |
           sudo apt-get update
-          sudo apt-get install -y libgtk-3-dev libxdo-dev libayatana-appindicator3-dev
+          sudo apt-get install -y libgtk-3-dev libxdo-dev libwebkit2gtk-4.1-dev
       - name: install stable
         uses: actions-rs/toolchain@v1
         with:
@@ -51,4 +51,4 @@ jobs:
       - uses: actions-rs/cargo@v1
         with:
           command: fmt
-          args: --all -- --check
+          args: --all -- --check

.github/workflows/test.yml

@@ -33,7 +33,7 @@ jobs:
         if: contains(matrix.os, 'ubuntu')
         run: |
           sudo apt-get update
-          sudo apt-get install -y libgtk-3-dev libxdo-dev libayatana-appindicator3-dev
+          sudo apt-get install -y libgtk-3-dev libxdo-dev libwebkit2gtk-4.1-dev
 
       - name: install stable
         uses: actions-rs/toolchain@v1

Cargo.toml

@@ -47,4 +47,5 @@ png = "0.17"
 [dev-dependencies]
 winit = "0.28"
 tao = { git = "https://github.com/tauri-apps/tao", branch = "muda" }
+wry = { git = "https://github.com/tauri-apps/wry", branch = "tao-v0.22" }
 image = "0.24"

examples/tao.rs

@@ -137,8 +137,8 @@ fn main() {
     }
     #[cfg(target_os = "linux")]
     {
-        menu_bar.init_for_gtk_window(window.gtk_window());
+        menu_bar.init_for_gtk_window(window.gtk_window(), window.default_vbox());
-        menu_bar.init_for_gtk_window(window2.gtk_window());
+        menu_bar.init_for_gtk_window(window2.gtk_window(), window2.default_vbox());
     }
     #[cfg(target_os = "macos")]
     {

examples/wry.rs

@@ -0,0 +1,240 @@
+// Copyright 2022-2022 Tauri Programme within The Commons Conservancy
+// SPDX-License-Identifier: Apache-2.0
+// SPDX-License-Identifier: MIT
+
+#![allow(unused)]
+use muda::{
+    accelerator::{Accelerator, Code, Modifiers},
+    AboutMetadata, CheckMenuItem, ContextMenu, IconMenuItem, Menu, MenuEvent, MenuItem,
+    PredefinedMenuItem, Submenu,
+};
+#[cfg(target_os = "macos")]
+use tao::platform::macos::WindowExtMacOS;
+#[cfg(target_os = "linux")]
+use tao::platform::unix::WindowExtUnix;
+#[cfg(target_os = "windows")]
+use tao::platform::windows::{EventLoopBuilderExtWindows, WindowExtWindows};
+use tao::{
+    event::{ElementState, Event, MouseButton, WindowEvent},
+    event_loop::{ControlFlow, EventLoopBuilder},
+    window::{Window, WindowBuilder},
+};
+use wry::webview::WebViewBuilder;
+
+fn main() -> wry::Result<()> {
+    let mut event_loop_builder = EventLoopBuilder::new();
+
+    let menu_bar = Menu::new();
+
+    #[cfg(target_os = "windows")]
+    {
+        let menu_bar = menu_bar.clone();
+        event_loop_builder.with_msg_hook(move |msg| {
+            use windows_sys::Win32::UI::WindowsAndMessaging::{TranslateAcceleratorW, MSG};
+            unsafe {
+                let msg = msg as *const MSG;
+                let translated = TranslateAcceleratorW((*msg).hwnd, menu_bar.haccel(), msg);
+                translated == 1
+            }
+        });
+    }
+
+    let event_loop = event_loop_builder.build();
+
+    let window = WindowBuilder::new()
+        .with_title("Window 1")
+        .build(&event_loop)
+        .unwrap();
+
+    let window2 = WindowBuilder::new()
+        .with_title("Window 2")
+        .build(&event_loop)
+        .unwrap();
+    let window2_id = window2.id();
+
+    #[cfg(target_os = "macos")]
+    {
+        let app_m = Submenu::new("App", true);
+        menu_bar.append(&app_m);
+        app_m.append_items(&[
+            &PredefinedMenuItem::about(None, None),
+            &PredefinedMenuItem::separator(),
+            &PredefinedMenuItem::services(None),
+            &PredefinedMenuItem::separator(),
+            &PredefinedMenuItem::hide(None),
+            &PredefinedMenuItem::hide_others(None),
+            &PredefinedMenuItem::show_all(None),
+            &PredefinedMenuItem::separator(),
+            &PredefinedMenuItem::quit(None),
+        ]);
+    }
+
+    let file_m = Submenu::new("&File", true);
+    let edit_m = Submenu::new("&Edit", true);
+    let window_m = Submenu::new("&Window", true);
+
+    menu_bar.append_items(&[&file_m, &edit_m, &window_m]);
+
+    let custom_i_1 = MenuItem::new(
+        "C&ustom 1",
+        true,
+        Some(Accelerator::new(Some(Modifiers::ALT), Code::KeyC)),
+    );
+
+    let path = concat!(env!("CARGO_MANIFEST_DIR"), "/examples/icon.png");
+    let icon = load_icon(std::path::Path::new(path));
+    let image_item = IconMenuItem::new(
+        "Image custom 1",
+        true,
+        Some(icon),
+        Some(Accelerator::new(Some(Modifiers::CONTROL), Code::KeyC)),
+    );
+
+    let check_custom_i_1 = CheckMenuItem::new("Check Custom 1", true, true, None);
+    let check_custom_i_2 = CheckMenuItem::new("Check Custom 2", false, true, None);
+    let check_custom_i_3 = CheckMenuItem::new(
+        "Check Custom 3",
+        true,
+        true,
+        Some(Accelerator::new(Some(Modifiers::SHIFT), Code::KeyD)),
+    );
+
+    let copy_i = PredefinedMenuItem::copy(None);
+    let cut_i = PredefinedMenuItem::cut(None);
+    let paste_i = PredefinedMenuItem::paste(None);
+
+    file_m.append_items(&[
+        &custom_i_1,
+        &image_item,
+        &window_m,
+        &PredefinedMenuItem::separator(),
+        &check_custom_i_1,
+        &check_custom_i_2,
+    ]);
+
+    window_m.append_items(&[
+        &PredefinedMenuItem::minimize(None),
+        &PredefinedMenuItem::maximize(None),
+        &PredefinedMenuItem::close_window(Some("Close")),
+        &PredefinedMenuItem::fullscreen(None),
+        &PredefinedMenuItem::about(
+            None,
+            Some(AboutMetadata {
+                name: Some("tao".to_string()),
+                version: Some("1.2.3".to_string()),
+                copyright: Some("Copyright tao".to_string()),
+                ..Default::default()
+            }),
+        ),
+        &check_custom_i_3,
+        &image_item,
+        &custom_i_1,
+    ]);
+
+    edit_m.append_items(&[&copy_i, &PredefinedMenuItem::separator(), &paste_i]);
+
+    #[cfg(target_os = "windows")]
+    {
+        menu_bar.init_for_hwnd(window.hwnd() as _);
+        menu_bar.init_for_hwnd(window2.hwnd() as _);
+    }
+    #[cfg(target_os = "linux")]
+    {
+        menu_bar.init_for_gtk_window(window.gtk_window(), window.default_vbox());
+        menu_bar.init_for_gtk_window(window2.gtk_window(), window2.default_vbox());
+    }
+    #[cfg(target_os = "macos")]
+    {
+        menu_bar.init_for_nsapp();
+        window_m.set_windows_menu_for_nsapp();
+    }
+
+    const HTML: &str = r#"
+    <html>
+    <body>
+        <style>
+            main {
+                width: 100vw;
+                height: 100vh;
+            }
+        </style>
+        <main>
+            <h4> WRYYYYYYYYYYYYYYYYYYYYYY! </h4>
+            <input />
+            <button> Hi </button>
+        </main>
+        <script>
+            window.addEventListener('contextmenu', (e) => {
+                e.preventDefault();
+                window.ipc.postMessage(`showContextMenu:${e.clientX},${e.clientY}`);
+            })
+        </script>
+    </body>
+    </html>
+  "#;
+
+    let handler = move |window: &Window, req: String| {
+        if let Some(rest) = req.strip_prefix("showContextMenu:") {
+            let (x, y) = rest
+                .split_once(',')
+                .map(|(x, y)| (x.parse::<f64>().unwrap(), y.parse::<f64>().unwrap()))
+                .unwrap();
+            if window.id() == window2_id {
+                show_context_menu(window, &window_m, x, y)
+            }
+        }
+    };
+
+    let webview = WebViewBuilder::new(window)?
+        .with_html(HTML)?
+        .with_ipc_handler(handler.clone())
+        .build()?;
+    let webview2 = WebViewBuilder::new(window2)?
+        .with_html(HTML)?
+        .with_ipc_handler(handler)
+        .build()?;
+
+    let menu_channel = MenuEvent::receiver();
+
+    event_loop.run(move |event, _, control_flow| {
+        *control_flow = ControlFlow::Wait;
+
+        if let Event::WindowEvent {
+            event: WindowEvent::CloseRequested,
+            ..
+        } = event
+        {
+            *control_flow = ControlFlow::Exit;
+        }
+
+        if let Ok(event) = menu_channel.try_recv() {
+            if event.id == custom_i_1.id() {
+                custom_i_1
+                    .set_accelerator(Some(Accelerator::new(Some(Modifiers::SHIFT), Code::KeyF)));
+                file_m.insert(&MenuItem::new("New Menu Item", true, None), 2);
+            }
+            println!("{event:?}");
+        }
+    })
+}
+
+fn show_context_menu(window: &Window, menu: &dyn ContextMenu, x: f64, y: f64) {
+    #[cfg(target_os = "windows")]
+    menu.show_context_menu_for_hwnd(window.hwnd() as _, x, y);
+    #[cfg(target_os = "linux")]
+    menu.show_context_menu_for_gtk_window(window.gtk_window(), x, y);
+    #[cfg(target_os = "macos")]
+    menu.show_context_menu_for_nsview(window.ns_view() as _, x, y);
+}
+
+fn load_icon(path: &std::path::Path) -> muda::icon::Icon {
+    let (icon_rgba, icon_width, icon_height) = {
+        let image = image::open(path)
+            .expect("Failed to open icon path")
+            .into_rgba8();
+        let (width, height) = image.dimensions();
+        let rgba = image.into_raw();
+        (rgba, width, height)
+    };
+    muda::icon::Icon::from_rgba(icon_rgba, icon_width, icon_height).expect("Failed to open icon")
+}

src/lib.rs

@@ -76,11 +76,13 @@
 //! # let window_hwnd = 0;
 //! # #[cfg(target_os = "linux")]
 //! # let gtk_window = gtk::ApplicationWindow::builder().build();
+//! # #[cfg(target_os = "linux")]
+//! # let vertical_gtk_box = gtk::Box::new(gtk::Orientation::Vertical, 0);
 //! // --snip--
 //! #[cfg(target_os = "windows")]
 //! menu.init_for_hwnd(window_hwnd);
 //! #[cfg(target_os = "linux")]
-//! menu.init_for_gtk_window(&gtk_window);
+//! menu.init_for_gtk_window(&gtk_window, Some(&vertical_gtk_box));
 //! #[cfg(target_os = "macos")]
 //! menu.init_for_nsapp();
 //! ```
@@ -268,17 +270,18 @@ pub trait ContextMenu {
     /// `x` and `y` are relative to the window's top-left corner.
     #[cfg(target_os = "linux")]
     fn show_context_menu_for_gtk_window(&self, w: &gtk::ApplicationWindow, x: f64, y: f64);
+
     /// Get the underlying gtk menu reserved for context menus.
     #[cfg(target_os = "linux")]
     fn gtk_context_menu(&self) -> gtk::Menu;
 
     /// Shows this menu as a context menu for the specified `NSView`.
     ///
-    /// The menu will be shown at the coordinates of the current event
+    /// `x` and `y` are relative to the window's top-left corner.
-    /// (the click which triggered the menu to be shown).
     #[cfg(target_os = "macos")]
     fn show_context_menu_for_nsview(&self, view: cocoa::base::id, x: f64, y: f64);
 
+    /// Get the underlying NSMenu reserved for context menus.
     #[cfg(target_os = "macos")]
     fn ns_menu(&self) -> *mut std::ffi::c_void;
 }

src/menu.rs

@@ -123,25 +123,32 @@ impl Menu {
 
     /// Adds this menu to a [`gtk::ApplicationWindow`]
     ///
-    /// This method adds a [`gtk::Box`] then adds a [`gtk::MenuBar`] as its first child and returns the [`gtk::Box`].
+    /// - `container`: this is an optional paramter to specify a container for the [`gtk::MenuBar`],
-    /// So if more widgets need to be added, then [`gtk::prelude::BoxExt::pack_start`] or
+    /// it is highly recommended to pass a container, otherwise the menubar will be added directly to the window,
-    /// similiar methods should be used on the returned [`gtk::Box`].
+    /// which is usually not the desired behavior.
     ///
-    /// ## Safety:
+    /// ## Example:
-    ///
+    /// ```no_run
-    /// This should be called before anything is added to the window.
+    /// let window = gtk::ApplicationWindow::builder().build();
+    /// let vbox = gtk::Box::new(gtk::Orientation::Vertical, 0);
+    /// let menu = muda::Menu::new();
+    /// // -- snip, add your menu items --
+    /// menu.init_for_gtk_window(&window, Some(&vbox));
+    /// // then proceed to add your widgets to the `vbox`
+    /// ```
     ///
     /// ## Panics:
     ///
     /// Panics if the gtk event loop hasn't been initialized on the thread.
     #[cfg(target_os = "linux")]
-    pub fn init_for_gtk_window<W>(&self, w: &W) -> crate::Result<gtk::Box>
+    pub fn init_for_gtk_window<W, C>(&self, window: &W, container: Option<&C>) -> crate::Result<()>
     where
         W: gtk::prelude::IsA<gtk::ApplicationWindow>,
-        W: gtk::prelude::IsA<gtk::Container>,
         W: gtk::prelude::IsA<gtk::Window>,
+        W: gtk::prelude::IsA<gtk::Container>,
+        C: gtk::prelude::IsA<gtk::Container>,
     {
-        self.0.borrow_mut().init_for_gtk_window(w)
+        self.0.borrow_mut().init_for_gtk_window(window, container)
     }
 
     /// Adds this menu to a win32 window.
@@ -183,12 +190,12 @@ impl Menu {
 
     /// Removes this menu from a [`gtk::ApplicationWindow`]
     #[cfg(target_os = "linux")]
-    pub fn remove_for_gtk_window<W>(&self, w: &W) -> crate::Result<()>
+    pub fn remove_for_gtk_window<W>(&self, window: &W) -> crate::Result<()>
     where
         W: gtk::prelude::IsA<gtk::ApplicationWindow>,
         W: gtk::prelude::IsA<gtk::Window>,
     {
-        self.0.borrow_mut().remove_for_gtk_window(w)
+        self.0.borrow_mut().remove_for_gtk_window(window)
     }
 
     /// Removes this menu from a win32 window
@@ -199,11 +206,11 @@ impl Menu {
 
     /// Hides this menu from a [`gtk::ApplicationWindow`]
     #[cfg(target_os = "linux")]
-    pub fn hide_for_gtk_window<W>(&self, w: &W) -> crate::Result<()>
+    pub fn hide_for_gtk_window<W>(&self, window: &W) -> crate::Result<()>
     where
         W: gtk::prelude::IsA<gtk::ApplicationWindow>,
     {
-        self.0.borrow_mut().hide_for_gtk_window(w)
+        self.0.borrow_mut().hide_for_gtk_window(window)
     }
 
     /// Hides this menu from a win32 window
@@ -214,11 +221,11 @@ impl Menu {
 
     /// Shows this menu on a [`gtk::ApplicationWindow`]
     #[cfg(target_os = "linux")]
-    pub fn show_for_gtk_window<W>(&self, w: &W) -> crate::Result<()>
+    pub fn show_for_gtk_window<W>(&self, window: &W) -> crate::Result<()>
     where
         W: gtk::prelude::IsA<gtk::ApplicationWindow>,
     {
-        self.0.borrow_mut().show_for_gtk_window(w)
+        self.0.borrow_mut().show_for_gtk_window(window)
     }
 
     /// Shows this menu on a win32 window
@@ -229,11 +236,11 @@ impl Menu {
 
     /// Returns whether this menu visible on a [`gtk::ApplicationWindow`]
     #[cfg(target_os = "linux")]
-    pub fn is_visible_on_gtk_window<W>(&self, w: &W) -> bool
+    pub fn is_visible_on_gtk_window<W>(&self, window: &W) -> bool
     where
         W: gtk::prelude::IsA<gtk::ApplicationWindow>,
     {
-        self.0.borrow().is_visible_on_gtk_window(w)
+        self.0.borrow().is_visible_on_gtk_window(window)
     }
 
     /// Returns whether this menu visible on a on a win32 window
@@ -277,10 +284,10 @@ impl ContextMenu for Menu {
     }
 
     #[cfg(target_os = "linux")]
-    fn show_context_menu_for_gtk_window(&self, w: &gtk::ApplicationWindow, x: f64, y: f64) {
+    fn show_context_menu_for_gtk_window(&self, window: &gtk::ApplicationWindow, x: f64, y: f64) {
         self.0
             .borrow_mut()
-            .show_context_menu_for_gtk_window(w, x, y)
+            .show_context_menu_for_gtk_window(window, x, y)
     }
 
     #[cfg(target_os = "linux")]

src/platform_impl/gtk/mod.rs

@@ -55,7 +55,7 @@ macro_rules! return_if_predefined_item_not_supported {
 pub struct Menu {
     id: u32,
     children: Vec<Rc<RefCell<MenuChild>>>,
-    gtk_menubars: HashMap<u32, (Option<gtk::MenuBar>, gtk::Box)>,
+    gtk_menubars: HashMap<u32, gtk::MenuBar>,
     accel_group: Option<gtk::AccelGroup>,
     gtk_menu: (u32, Option<gtk::Menu>), // dedicated menu for tray or context menus
 }
@@ -78,16 +78,13 @@ impl Menu {
     pub fn add_menu_item(&mut self, item: &dyn crate::IsMenuItem, op: AddOp) -> crate::Result<()> {
         return_if_predefined_item_not_supported!(item);
 
-        for (menu_id, (menu_bar, _)) in &self.gtk_menubars {
+        for (menu_id, menu_bar) in &self.gtk_menubars {
-            if let Some(menu_bar) = menu_bar {
+            let gtk_item = item.make_gtk_menu_item(*menu_id, self.accel_group.as_ref(), true)?;
-                let gtk_item =
+            match op {
-                    item.make_gtk_menu_item(*menu_id, self.accel_group.as_ref(), true)?;
+                AddOp::Append => menu_bar.append(&gtk_item),
-                match op {
+                AddOp::Insert(position) => menu_bar.insert(&gtk_item, position as i32),
-                    AddOp::Append => menu_bar.append(&gtk_item),
-                    AddOp::Insert(position) => menu_bar.insert(&gtk_item, position as i32),
-                }
-                gtk_item.show();
             }
+            gtk_item.show();
         }
 
         {
@@ -114,13 +111,10 @@ impl Menu {
     fn add_menu_item_with_id(&self, item: &dyn crate::IsMenuItem, id: u32) -> crate::Result<()> {
         return_if_predefined_item_not_supported!(item);
 
-        for (menu_id, (menu_bar, _)) in self.gtk_menubars.iter().filter(|m| *m.0 == id) {
+        for (menu_id, menu_bar) in self.gtk_menubars.iter().filter(|m| *m.0 == id) {
-            if let Some(menu_bar) = menu_bar {
+            let gtk_item = item.make_gtk_menu_item(*menu_id, self.accel_group.as_ref(), true)?;
-                let gtk_item =
+            menu_bar.append(&gtk_item);
-                    item.make_gtk_menu_item(*menu_id, self.accel_group.as_ref(), true)?;
+            gtk_item.show();
-                menu_bar.append(&gtk_item);
-                gtk_item.show();
-            }
         }
 
         Ok(())
@@ -176,18 +170,16 @@ impl Menu {
             }
         }
 
-        for (menu_id, (menu_bar, _)) in &self.gtk_menubars {
+        for (menu_id, menu_bar) in &self.gtk_menubars {
             if id.map(|i| i == *menu_id).unwrap_or(true) {
-                if let Some(menu_bar) = menu_bar {
+                if let Some(items) = child
-                    if let Some(items) = child
+                    .borrow_mut()
-                        .borrow_mut()
+                    .gtk_menu_items
-                        .gtk_menu_items
+                    .borrow_mut()
-                        .borrow_mut()
+                    .remove(menu_id)
-                        .remove(menu_id)
+                {
-                    {
+                    for item in items {
-                        for item in items {
+                        menu_bar.remove(&item);
-                            menu_bar.remove(&item);
-                        }
                     }
                 }
             }
@@ -218,11 +210,16 @@ impl Menu {
             .collect()
     }
 
-    pub fn init_for_gtk_window<W>(&mut self, window: &W) -> crate::Result<gtk::Box>
+    pub fn init_for_gtk_window<W, C>(
+        &mut self,
+        window: &W,
+        container: Option<&C>,
+    ) -> crate::Result<()>
     where
         W: IsA<gtk::ApplicationWindow>,
-        W: IsA<gtk::Container>,
         W: IsA<gtk::Window>,
+        W: IsA<gtk::Container>,
+        C: IsA<gtk::Container>,
     {
         let id = window.as_ptr() as u32;
 
@@ -234,25 +231,13 @@ impl Menu {
         // so we need to create the menubar and its parent box
         if self.gtk_menubars.get(&id).is_none() {
             let menu_bar = gtk::MenuBar::new();
-            let vbox = gtk::Box::new(Orientation::Vertical, 0);
+            self.gtk_menubars.insert(id, menu_bar);
-            window.add(&vbox);
+        } else {
-            vbox.show();
+            return Err(crate::Error::AlreadyInitialized);
-            self.gtk_menubars.insert(id, (Some(menu_bar), vbox));
-        } else if let Some((menu_bar, _)) = self.gtk_menubars.get_mut(&id) {
-            // This is NOT the first time this method has been called on a window.
-            // So it already contains a [`gtk::Box`] but it doesn't have a [`gtk::MenuBar`]
-            // because it was probably removed using [`Menu::remove_for_gtk_window`]
-            // so we only need to create the menubar
-            if menu_bar.is_none() {
-                menu_bar.replace(gtk::MenuBar::new());
-            } else {
-                return Err(crate::Error::AlreadyInitialized);
-            }
         }
 
         // Construct the entries of the menubar
-        let (menu_bar, vbox) = self.gtk_menubars.get(&id).cloned().unwrap();
+        let menu_bar = &self.gtk_menubars[&id];
-        let menu_bar = menu_bar.as_ref().unwrap();
 
         window.add_accel_group(self.accel_group.as_ref().unwrap());
 
@@ -260,11 +245,17 @@ impl Menu {
             self.add_menu_item_with_id(item.as_ref(), id)?;
         }
 
-        // Show the menubar on the window
+        // add the menubar to the specified widget, otherwise to the window
-        vbox.pack_start(menu_bar, false, false, 0);
+        if let Some(container) = container {
+            container.add(menu_bar);
+        } else {
+            window.add(menu_bar);
+        }
+
+        // Show the menubar
         menu_bar.show();
 
-        Ok(vbox)
+        Ok(())
     }
 
     pub fn remove_for_gtk_window<W>(&mut self, window: &W) -> crate::Result<()>
@@ -273,51 +264,44 @@ impl Menu {
         W: IsA<gtk::Window>,
     {
         let id = window.as_ptr() as u32;
+
+        // Remove from our cache
         let menu_bar = self
             .gtk_menubars
             .remove(&id)
             .ok_or(crate::Error::NotInitialized)?;
 
-        if let (Some(menu_bar), vbox) = menu_bar {
+        for item in self.items() {
-            for item in self.items() {
+            let _ = self.remove_inner(item.as_ref(), false, Some(id));
-                let _ = self.remove_inner(item.as_ref(), false, Some(id));
-            }
-
-            // Remove the [`gtk::Menubar`] from the widget tree
-            unsafe { menu_bar.destroy() };
-            // Detach the accelerators from the window
-            window.remove_accel_group(self.accel_group.as_ref().unwrap());
-            // Remove the removed [`gtk::Menubar`] from our cache
-            self.gtk_menubars.insert(id, (None, vbox));
-            Ok(())
-        } else {
-            self.gtk_menubars.insert(id, menu_bar);
-            Err(crate::Error::NotInitialized)
         }
+
+        // Remove the [`gtk::Menubar`] from the widget tree
+        unsafe { menu_bar.destroy() };
+        // Detach the accelerators from the window
+        window.remove_accel_group(self.accel_group.as_ref().unwrap());
+        Ok(())
     }
 
     pub fn hide_for_gtk_window<W>(&mut self, window: &W) -> crate::Result<()>
     where
         W: IsA<gtk::ApplicationWindow>,
     {
-        if let Some((Some(menu_bar), _)) = self.gtk_menubars.get(&(window.as_ptr() as u32)) {
+        self.gtk_menubars
-            menu_bar.hide();
+            .get(&(window.as_ptr() as u32))
-            Ok(())
+            .ok_or(crate::Error::NotInitialized)?
-        } else {
+            .hide();
-            Err(crate::Error::NotInitialized)
+        Ok(())
-        }
     }
 
     pub fn show_for_gtk_window<W>(&self, window: &W) -> crate::Result<()>
     where
         W: IsA<gtk::ApplicationWindow>,
     {
-        if let Some((Some(menu_bar), _)) = self.gtk_menubars.get(&(window.as_ptr() as u32)) {
+        self.gtk_menubars
-            menu_bar.show_all();
+            .get(&(window.as_ptr() as u32))
-            Ok(())
+            .ok_or(crate::Error::NotInitialized)?
-        } else {
+            .show_all();
-            Err(crate::Error::NotInitialized)
+        Ok(())
-        }
     }
 
     pub fn is_visible_on_gtk_window<W>(&self, window: &W) -> bool
@@ -326,7 +310,7 @@ impl Menu {
     {
         self.gtk_menubars
             .get(&(window.as_ptr() as u32))
-            .map(|m| m.0.as_ref().map(|m| m.get_visible()).unwrap_or(false))
+            .map(|m| m.get_visible())
             .unwrap_or(false)
     }
 
@@ -562,16 +546,20 @@ impl MenuChild {
         for items in self.gtk_menu_items.borrow().values() {
             for i in items {
                 if let Some((mods, key)) = prev_accel {
-                    i.remove_accelerator(self.accel_group.as_ref().unwrap(), *key, *mods);
+                    if let Some(accel_group) = &self.accel_group {
+                        i.remove_accelerator(accel_group, *key, *mods);
+                    }
                 }
                 if let Some((mods, key)) = new_accel {
-                    i.add_accelerator(
+                    if let Some(accel_group) = &self.accel_group {
-                        "activate",
+                        i.add_accelerator(
-                        self.accel_group.as_ref().unwrap(),
+                            "activate",
-                        key,
+                            accel_group,
-                        mods,
+                            key,
-                        gtk::AccelFlags::VISIBLE,
+                            mods,
-                    )
+                            gtk::AccelFlags::VISIBLE,
+                        )
+                    }
                 }
             }
         }
@@ -1210,6 +1198,7 @@ impl dyn crate::IsMenuItem + '_ {
 }
 
 impl PredfinedMenuItemType {
+    #[cfg(feature = "libxdo")]
     fn xdo_keys(&self) -> &str {
         match self {
             PredfinedMenuItemType::Copy => "ctrl+c",