LibGUI, WindowServer: Greatly simplify menubar logic

Currently, any number of menubars can be plugged in and out of a window.
This is unnecessary complexity, since we only need one menubar on a
window. This commit removes most of the logic for dynamically attaching
and detaching menubars and makes one menubar always available. The
menubar is only considered existent if it has at least a single menu in
it (in other words, an empty menubar will not be shown).

This commit additionally fixes a bug wherein menus added after a menubar
has been attached would not have their rects properly setup, and would
therefore appear glitched out on the top left corner of the menubar.

Userland/Libraries/LibGUI/CMakeLists.txt

@@ -59,7 +59,6 @@ set(SOURCES
     LinkLabel.cpp
     ListView.cpp
     Menu.cpp
-    Menubar.cpp
     MenuItem.cpp
     MessageBox.cpp
     Model.cpp

Userland/Libraries/LibGUI/Menu.cpp

@@ -51,24 +51,35 @@ void Menu::set_icon(const Gfx::Bitmap* icon)
 
 void Menu::add_action(NonnullRefPtr<Action> action)
 {
-    m_items.append(make<MenuItem>(m_menu_id, move(action)));
+    auto item = make<MenuItem>(m_menu_id, move(action));
+    if (m_menu_id != -1)
+        realize_menu_item(*item, m_items.size());
+    m_items.append(move(item));
 }
 
 Menu& Menu::add_submenu(const String& name)
 {
     auto submenu = Menu::construct(name);
-    m_items.append(make<MenuItem>(m_menu_id, submenu));
+
+    auto item = make<MenuItem>(m_menu_id, submenu);
+    if (m_menu_id != -1)
+        realize_menu_item(*item, m_items.size());
+    m_items.append(move(item));
+
     return submenu;
 }
 
 void Menu::add_separator()
 {
-    m_items.append(make<MenuItem>(m_menu_id, MenuItem::Type::Separator));
+    auto item = make<MenuItem>(m_menu_id, MenuItem::Type::Separator);
+    if (m_menu_id != -1)
+        realize_menu_item(*item, m_items.size());
+    m_items.append(move(item));
 }
 
 void Menu::realize_if_needed(const RefPtr<Action>& default_action)
 {
-    if (m_menu_id == -1 || m_last_default_action.ptr() != default_action)
+    if (m_menu_id == -1 || m_current_default_action.ptr() != default_action)
         realize_menu(default_action);
 }
 
@@ -94,32 +105,13 @@ int Menu::realize_menu(RefPtr<Action> default_action)
 
     dbgln_if(MENU_DEBUG, "GUI::Menu::realize_menu(): New menu ID: {}", m_menu_id);
     VERIFY(m_menu_id > 0);
+    m_current_default_action = default_action;
+
     for (size_t i = 0; i < m_items.size(); ++i) {
-        auto& item = m_items[i];
-        item.set_menu_id({}, m_menu_id);
-        item.set_identifier({}, i);
-        if (item.type() == MenuItem::Type::Separator) {
-            WindowServerConnection::the().async_add_menu_separator(m_menu_id);
-            continue;
-        }
-        if (item.type() == MenuItem::Type::Submenu) {
-            auto& submenu = *item.submenu();
-            submenu.realize_if_needed(default_action);
-            auto icon = submenu.icon() ? submenu.icon()->to_shareable_bitmap() : Gfx::ShareableBitmap();
-            WindowServerConnection::the().async_add_menu_item(m_menu_id, i, submenu.menu_id(), submenu.name(), true, false, false, false, "", icon, false);
-            continue;
-        }
-        if (item.type() == MenuItem::Type::Action) {
-            auto& action = *item.action();
-            auto shortcut_text = action.shortcut().is_valid() ? action.shortcut().to_string() : String();
-            bool exclusive = action.group() && action.group()->is_exclusive() && action.is_checkable();
-            bool is_default = (default_action.ptr() == &action);
-            auto icon = action.icon() ? action.icon()->to_shareable_bitmap() : Gfx::ShareableBitmap();
-            WindowServerConnection::the().async_add_menu_item(m_menu_id, i, -1, action.text(), action.is_enabled(), action.is_checkable(), action.is_checkable() ? action.is_checked() : false, is_default, shortcut_text, icon, exclusive);
-        }
+        realize_menu_item(m_items[i], i);
     }
+
     all_menus().set(m_menu_id, this);
-    m_last_default_action = default_action;
     return m_menu_id;
 }
 
@@ -154,4 +146,34 @@ void Menu::visibility_did_change(Badge<WindowServerConnection>, bool visible)
         on_visibility_change(visible);
 }
 
+void Menu::realize_menu_item(MenuItem& item, int item_id)
+{
+    item.set_menu_id({}, m_menu_id);
+    item.set_identifier({}, item_id);
+    switch (item.type()) {
+    case MenuItem::Type::Separator:
+        WindowServerConnection::the().async_add_menu_separator(m_menu_id);
+        break;
+    case MenuItem::Type::Action: {
+        auto& action = *item.action();
+        auto shortcut_text = action.shortcut().is_valid() ? action.shortcut().to_string() : String();
+        bool exclusive = action.group() && action.group()->is_exclusive() && action.is_checkable();
+        bool is_default = (m_current_default_action.ptr() == &action);
+        auto icon = action.icon() ? action.icon()->to_shareable_bitmap() : Gfx::ShareableBitmap();
+        WindowServerConnection::the().async_add_menu_item(m_menu_id, item_id, -1, action.text(), action.is_enabled(), action.is_checkable(), action.is_checkable() ? action.is_checked() : false, is_default, shortcut_text, icon, exclusive);
+        break;
+    }
+    case MenuItem::Type::Submenu: {
+        auto& submenu = *item.submenu();
+        submenu.realize_if_needed(m_current_default_action.strong_ref());
+        auto icon = submenu.icon() ? submenu.icon()->to_shareable_bitmap() : Gfx::ShareableBitmap();
+        WindowServerConnection::the().async_add_menu_item(m_menu_id, item_id, submenu.menu_id(), submenu.name(), true, false, false, false, "", icon, false);
+        break;
+    }
+    case MenuItem::Type::Invalid:
+    default:
+        VERIFY_NOT_REACHED();
+    }
+}
+
 }

Userland/Libraries/LibGUI/Menu.h

@@ -10,6 +10,7 @@
 #include <AK/WeakPtr.h>
 #include <LibCore/Object.h>
 #include <LibGUI/Action.h>
+#include <LibGUI/Event.h>
 #include <LibGUI/Forward.h>
 #include <LibGfx/Forward.h>
 
@@ -53,11 +54,13 @@ private:
     void unrealize_menu();
     void realize_if_needed(const RefPtr<Action>& default_action);
 
+    void realize_menu_item(MenuItem&, int item_id);
+
     int m_menu_id { -1 };
     String m_name;
     RefPtr<Gfx::Bitmap> m_icon;
     NonnullOwnPtrVector<MenuItem> m_items;
-    WeakPtr<Action> m_last_default_action;
+    WeakPtr<Action> m_current_default_action;
     bool m_visible { false };
 };
 

Userland/Libraries/LibGUI/Menubar.cpp

@@ -1,66 +0,0 @@
-/*
- * Copyright (c) 2018-2020, Andreas Kling <kling@serenityos.org>
- *
- * SPDX-License-Identifier: BSD-2-Clause
- */
-
-#include <AK/Badge.h>
-#include <AK/IDAllocator.h>
-#include <LibGUI/Menu.h>
-#include <LibGUI/MenuItem.h>
-#include <LibGUI/Menubar.h>
-#include <LibGUI/WindowServerConnection.h>
-
-namespace GUI {
-
-static IDAllocator s_menubar_id_allocator;
-
-Menubar::Menubar()
-{
-}
-
-Menubar::~Menubar()
-{
-    unrealize_menubar();
-}
-
-Menu& Menubar::add_menu(String name)
-{
-    auto& menu = add<Menu>(move(name));
-    m_menus.append(menu);
-    return menu;
-}
-
-int Menubar::realize_menubar()
-{
-    auto menubar_id = s_menubar_id_allocator.allocate();
-    WindowServerConnection::the().async_create_menubar(menubar_id);
-    return menubar_id;
-}
-
-void Menubar::unrealize_menubar()
-{
-    if (m_menubar_id == -1)
-        return;
-    WindowServerConnection::the().async_destroy_menubar(m_menubar_id);
-    m_menubar_id = -1;
-}
-
-void Menubar::notify_added_to_window(Badge<Window>)
-{
-    VERIFY(m_menubar_id == -1);
-    m_menubar_id = realize_menubar();
-    VERIFY(m_menubar_id != -1);
-    for (auto& menu : m_menus) {
-        int menu_id = menu.realize_menu();
-        VERIFY(menu_id != -1);
-        WindowServerConnection::the().async_add_menu_to_menubar(m_menubar_id, menu_id);
-    }
-}
-
-void Menubar::notify_removed_from_window(Badge<Window>)
-{
-    unrealize_menubar();
-}
-
-}

Userland/Libraries/LibGUI/Menubar.h

@@ -1,15 +1,18 @@
 /*
  * Copyright (c) 2018-2020, Andreas Kling <kling@serenityos.org>
+ * Copyright (c) 2021, sin-ack <sin-ack@protonmail.com>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
 
 #pragma once
 
-#include <AK/Forward.h>
+#include <AK/Badge.h>
+#include <AK/IterationDecision.h>
 #include <AK/NonnullRefPtrVector.h>
 #include <LibCore/Object.h>
 #include <LibGUI/Forward.h>
+#include <LibGUI/Menu.h>
 
 namespace GUI {
 
@@ -17,22 +20,27 @@ class Menubar : public Core::Object {
     C_OBJECT(Menubar);
 
 public:
-    ~Menubar();
+    ~Menubar() { }
 
-    Menu& add_menu(String name);
+    Menu& add_menu(Badge<Window>, String name)
+    {
+        auto& menu = add<Menu>(move(name));
+        m_menus.append(menu);
+        return menu;
+    }
 
-    void notify_added_to_window(Badge<Window>);
-    void notify_removed_from_window(Badge<Window>);
-
-    int menubar_id() const { return m_menubar_id; }
+    void for_each_menu(Function<IterationDecision(Menu&)> callback)
+    {
+        for (auto& menu : m_menus) {
+            if (callback(menu) == IterationDecision::Break) {
+                return;
+            }
+        }
+    }
 
 private:
-    Menubar();
+    Menubar() { }
 
-    int realize_menubar();
-    void unrealize_menubar();
-
-    int m_menubar_id { -1 };
     NonnullRefPtrVector<Menu> m_menus;
 };
 

Userland/Libraries/LibGUI/Window.cpp

@@ -66,6 +66,7 @@ Window* Window::from_window_id(int window_id)
 
 Window::Window(Core::Object* parent)
     : Core::Object(parent)
+    , m_menubar(Menubar::construct())
 {
     all_windows->set(this);
     m_rect_when_windowless = { -5000, -5000, 140, 140 };
@@ -92,8 +93,6 @@ Window::Window(Core::Object* parent)
 
 Window::~Window()
 {
-    if (m_menubar)
-        m_menubar->notify_removed_from_window({});
     all_windows->remove(this);
     hide();
 }
@@ -162,11 +161,11 @@ void Window::show()
 
     apply_icon();
 
-    if (m_menubar) {
-        // This little dance makes us create a server-side menubar.
-        auto menubar = move(m_menubar);
-        set_menubar(menubar);
-    }
+    m_menubar->for_each_menu([&](Menu& menu) {
+        menu.realize_menu_if_needed();
+        WindowServerConnection::the().async_add_menu(m_window_id, menu.menu_id());
+        return IterationDecision::Continue;
+    });
 
     reified_windows->set(m_window_id, this);
     Application::the()->did_create_window({});
@@ -1172,22 +1171,12 @@ Gfx::Bitmap* Window::back_bitmap()
 
 Menu& Window::add_menu(String name)
 {
-    if (!m_menubar)
-        set_menubar(GUI::Menubar::construct());
-    return m_menubar->add_menu(move(name));
-}
-
-void Window::set_menubar(RefPtr<Menubar> menubar)
-{
-    if (m_menubar == menubar)
-        return;
-    if (m_menubar)
-        m_menubar->notify_removed_from_window({});
-    m_menubar = move(menubar);
-    if (m_window_id && m_menubar) {
-        m_menubar->notify_added_to_window({});
-        WindowServerConnection::the().async_set_window_menubar(m_window_id, m_menubar->menubar_id());
+    Menu& menu = m_menubar->add_menu({}, move(name));
+    if (m_window_id) {
+        menu.realize_menu_if_needed();
+        WindowServerConnection::the().async_add_menu(m_window_id, menu.menu_id());
     }
+    return menu;
 }
 
 bool Window::is_modified() const

Userland/Libraries/LibGUI/Window.h

@@ -201,7 +201,6 @@ public:
     void did_disable_focused_widget(Badge<Widget>);
 
     Menu& add_menu(String name);
-    void set_menubar(RefPtr<Menubar>);
 
 protected:
     Window(Core::Object* parent = nullptr);
@@ -242,7 +241,7 @@ private:
     OwnPtr<WindowBackingStore> m_front_store;
     OwnPtr<WindowBackingStore> m_back_store;
 
-    RefPtr<Menubar> m_menubar;
+    NonnullRefPtr<Menubar> m_menubar;
 
     RefPtr<Gfx::Bitmap> m_icon;
     RefPtr<Gfx::Bitmap> m_custom_cursor;