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.

12 files changed, 65 insertions, 124 deletions

diff --git a/Userland/Services/WindowServer/CMakeLists.txt b/Userland/Services/WindowServer/CMakeLists.txt
index b0fd2f652e..b536a16ff5 100644
--- a/Userland/Services/WindowServer/CMakeLists.txt
+++ b/Userland/Services/WindowServer/CMakeLists.txt
@@ -18,8 +18,8 @@ set(SOURCES
     Cursor.cpp
     EventLoop.cpp
     main.cpp
-    Menubar.cpp
     Menu.cpp
+    Menubar.cpp
     MenuItem.cpp
     MenuManager.cpp
     MultiScaleBitmaps.cpp
diff --git a/Userland/Services/WindowServer/ClientConnection.cpp b/Userland/Services/WindowServer/ClientConnection.cpp
index 8b263586a9..2214b0f465 100644
--- a/Userland/Services/WindowServer/ClientConnection.cpp
+++ b/Userland/Services/WindowServer/ClientConnection.cpp
@@ -13,7 +13,6 @@
 #include <WindowServer/Compositor.h>
 #include <WindowServer/Menu.h>
 #include <WindowServer/MenuItem.h>
-#include <WindowServer/Menubar.h>
 #include <WindowServer/Screen.h>
 #include <WindowServer/Window.h>
 #include <WindowServer/WindowClientEndpoint.h>
@@ -91,22 +90,6 @@ void ClientConnection::notify_about_new_screen_rects()
     async_screen_rects_changed(Screen::rects(), Screen::main().index(), wm.window_stack_rows(), wm.window_stack_columns());
 }
 
-void ClientConnection::create_menubar(i32 menubar_id)
-{
-    auto menubar = Menubar::create(*this, menubar_id);
-    m_menubars.set(menubar_id, move(menubar));
-}
-
-void ClientConnection::destroy_menubar(i32 menubar_id)
-{
-    auto it = m_menubars.find(menubar_id);
-    if (it == m_menubars.end()) {
-        did_misbehave("DestroyMenubar: Bad menubar ID");
-        return;
-    }
-    m_menubars.remove(it);
-}
-
 void ClientConnection::create_menu(i32 menu_id, String const& menu_title)
 {
     auto menu = Menu::construct(this, menu_id, menu_title);
@@ -126,44 +109,21 @@ void ClientConnection::destroy_menu(i32 menu_id)
     remove_child(menu);
 }
 
-void ClientConnection::set_window_menubar(i32 window_id, i32 menubar_id)
-{
-    RefPtr<Window> window;
-    {
-        auto it = m_windows.find(window_id);
-        if (it == m_windows.end()) {
-            did_misbehave("SetWindowMenubar: Bad window ID");
-            return;
-        }
-        window = it->value;
-    }
-    RefPtr<Menubar> menubar;
-    if (menubar_id != -1) {
-        auto it = m_menubars.find(menubar_id);
-        if (it == m_menubars.end()) {
-            did_misbehave("SetWindowMenubar: Bad menubar ID");
-            return;
-        }
-        menubar = *(*it).value;
-    }
-    window->set_menubar(menubar);
-}
-
-void ClientConnection::add_menu_to_menubar(i32 menubar_id, i32 menu_id)
+void ClientConnection::add_menu(i32 window_id, i32 menu_id)
 {
-    auto it = m_menubars.find(menubar_id);
+    auto it = m_windows.find(window_id);
     auto jt = m_menus.find(menu_id);
-    if (it == m_menubars.end()) {
-        did_misbehave("AddMenuToMenubar: Bad menubar ID");
+    if (it == m_windows.end()) {
+        did_misbehave("AddMenu: Bad window ID");
         return;
     }
     if (jt == m_menus.end()) {
-        did_misbehave("AddMenuToMenubar: Bad menu ID");
+        did_misbehave("AddMenu: Bad menu ID");
         return;
     }
-    auto& menubar = *(*it).value;
+    auto& window = *(*it).value;
     auto& menu = *(*jt).value;
-    menubar.add_menu(menu);
+    window.add_menu(menu);
 }
 
 void ClientConnection::add_menu_item(i32 menu_id, i32 identifier, i32 submenu_id,
diff --git a/Userland/Services/WindowServer/ClientConnection.h b/Userland/Services/WindowServer/ClientConnection.h
index 74c0f29c32..d315ef1447 100644
--- a/Userland/Services/WindowServer/ClientConnection.h
+++ b/Userland/Services/WindowServer/ClientConnection.h
@@ -90,12 +90,9 @@ private:
     void set_unresponsive(bool);
     void destroy_window(Window&, Vector<i32>& destroyed_window_ids);
 
-    virtual void create_menubar(i32) override;
-    virtual void destroy_menubar(i32) override;
     virtual void create_menu(i32, String const&) override;
     virtual void destroy_menu(i32) override;
-    virtual void add_menu_to_menubar(i32, i32) override;
-    virtual void set_window_menubar(i32, i32) override;
+    virtual void add_menu(i32, i32) override;
     virtual void add_menu_item(i32, i32, i32, String const&, bool, bool, bool, bool, String const&, Gfx::ShareableBitmap const&, bool) override;
     virtual void add_menu_separator(i32) override;
     virtual void update_menu_item(i32, i32, i32, String const&, bool, bool, bool, bool, String const&) override;
@@ -173,7 +170,6 @@ private:
     Window* window_from_id(i32 window_id);
 
     HashMap<int, NonnullRefPtr<Window>> m_windows;
-    HashMap<int, NonnullRefPtr<Menubar>> m_menubars;
     HashMap<int, NonnullRefPtr<Menu>> m_menus;
 
     RefPtr<Core::Timer> m_ping_timer;
diff --git a/Userland/Services/WindowServer/Menu.h b/Userland/Services/WindowServer/Menu.h
index 49b46a5e5c..c3e4afa038 100644
--- a/Userland/Services/WindowServer/Menu.h
+++ b/Userland/Services/WindowServer/Menu.h
@@ -15,13 +15,14 @@
 #include <LibGfx/Forward.h>
 #include <LibGfx/Rect.h>
 #include <WindowServer/Cursor.h>
+#include <WindowServer/Event.h>
 #include <WindowServer/MenuItem.h>
-#include <WindowServer/Window.h>
 
 namespace WindowServer {
 
 class ClientConnection;
 class Menubar;
+class Window;
 
 class Menu final : public Core::Object {
     C_OBJECT(Menu);
diff --git a/Userland/Services/WindowServer/MenuManager.cpp b/Userland/Services/WindowServer/MenuManager.cpp
index c222a244d5..c1e9b88e8b 100644
--- a/Userland/Services/WindowServer/MenuManager.cpp
+++ b/Userland/Services/WindowServer/MenuManager.cpp
@@ -362,7 +362,7 @@ Menu* MenuManager::previous_menu(Menu* current)
         return nullptr;
     Menu* found = nullptr;
     Menu* previous = nullptr;
-    wm.window_with_active_menu()->menubar()->for_each_menu([&](Menu& menu) {
+    wm.window_with_active_menu()->menubar().for_each_menu([&](Menu& menu) {
         if (current == &menu) {
             found = previous;
             return IterationDecision::Break;
@@ -380,7 +380,7 @@ Menu* MenuManager::next_menu(Menu* current)
     auto& wm = WindowManager::the();
     if (!wm.window_with_active_menu())
         return nullptr;
-    wm.window_with_active_menu()->menubar()->for_each_menu([&](Menu& menu) {
+    wm.window_with_active_menu()->menubar().for_each_menu([&](Menu& menu) {
         if (is_next) {
             found = &menu;
             return IterationDecision::Break;
diff --git a/Userland/Services/WindowServer/Menubar.cpp b/Userland/Services/WindowServer/Menubar.cpp
index 3bd9ecea88..f0cd7c50e3 100644
--- a/Userland/Services/WindowServer/Menubar.cpp
+++ b/Userland/Services/WindowServer/Menubar.cpp
@@ -1,27 +1,26 @@
 /*
  * Copyright (c) 2018-2021, Andreas Kling <kling@serenityos.org>
+ * Copyright (c) 2021, sin-ack <sin-ack@protonmail.com>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
 
 #include "Menubar.h"
-#include "Menu.h"
+#include "WindowManager.h"
 
 namespace WindowServer {
 
-Menubar::Menubar(ClientConnection& client, int menubar_id)
-    : m_client(client)
-    , m_menubar_id(menubar_id)
+void Menubar::layout_menu(Menu& menu, Gfx::IntRect window_rect)
 {
-}
+    // FIXME: Maybe move this to the theming system?
+    static constexpr auto menubar_menu_margin = 14;
 
-Menubar::~Menubar()
-{
-}
+    auto& wm = WindowManager::the();
+    auto menubar_rect = Gfx::WindowTheme::current().menubar_rect(Gfx::WindowTheme::WindowType::Normal, window_rect, wm.palette(), 1);
 
-void Menubar::add_menu(Menu& menu)
-{
-    m_menus.append(menu);
+    int text_width = wm.font().width(Gfx::parse_ampersand_string(menu.name()));
+    menu.set_rect_in_window_menubar({ m_next_menu_location.x(), 0, text_width + menubar_menu_margin, menubar_rect.height() });
+    m_next_menu_location.translate_by(menu.rect_in_window_menubar().width(), 0);
 }
 
 }
diff --git a/Userland/Services/WindowServer/Menubar.h b/Userland/Services/WindowServer/Menubar.h
index 838718252b..3463f8e5f5 100644
--- a/Userland/Services/WindowServer/Menubar.h
+++ b/Userland/Services/WindowServer/Menubar.h
@@ -1,5 +1,6 @@
 /*
  * Copyright (c) 2018-2020, Andreas Kling <kling@serenityos.org>
+ * Copyright (c) 2021, sin-ack <sin-ack@protonmail.com>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -7,26 +8,27 @@
 #pragma once
 
 #include "Menu.h"
+#include <AK/Function.h>
+#include <AK/IterationDecision.h>
 #include <AK/Vector.h>
-#include <AK/WeakPtr.h>
-#include <AK/Weakable.h>
 
 namespace WindowServer {
 
-class Menubar
-    : public RefCounted<Menubar>
-    , public Weakable<Menubar> {
+class Menubar {
 public:
-    static NonnullRefPtr<Menubar> create(ClientConnection& client, int menubar_id) { return adopt_ref(*new Menubar(client, menubar_id)); }
-    ~Menubar();
+    void add_menu(Menu& menu, Gfx::IntRect window_rect)
+    {
+        // FIXME: Check against duplicate menu additions.
+        m_menus.append(menu);
+        layout_menu(menu, window_rect);
+    }
 
-    ClientConnection& client() { return m_client; }
-    const ClientConnection& client() const { return m_client; }
-    int menubar_id() const { return m_menubar_id; }
-    void add_menu(Menu&);
+    bool has_menus()
+    {
+        return !m_menus.is_empty();
+    }
 
-    template<typename Callback>
-    void for_each_menu(Callback callback)
+    void for_each_menu(Function<IterationDecision(Menu&)> callback)
     {
         for (auto& menu : m_menus) {
             if (callback(menu) == IterationDecision::Break)
@@ -35,11 +37,13 @@ public:
     }
 
 private:
-    Menubar(ClientConnection&, int menubar_id);
+    void layout_menu(Menu&, Gfx::IntRect window_rect);
 
-    ClientConnection& m_client;
-    int m_menubar_id { 0 };
     Vector<Menu&> m_menus;
+
+    // FIXME: This doesn't support removing menus from a menubar or inserting a
+    //        menu in the middle.
+    Gfx::IntPoint m_next_menu_location { 0, 0 };
 };
 
 }
diff --git a/Userland/Services/WindowServer/Window.cpp b/Userland/Services/WindowServer/Window.cpp
index bec2c4b5be..2b56c56eb7 100644
--- a/Userland/Services/WindowServer/Window.cpp
+++ b/Userland/Services/WindowServer/Window.cpp
@@ -73,6 +73,7 @@ static Gfx::Bitmap& pin_icon()
         s_icon = Gfx::Bitmap::try_load_from_file("/res/icons/16x16/window-pin.png").leak_ref();
     return *s_icon;
 }
+
 Window::Window(Core::Object& parent, WindowType type)
     : Core::Object(&parent)
     , m_type(type)
@@ -576,15 +577,16 @@ void Window::handle_keydown_event(const KeyEvent& event)
         popup_window_menu(position, WindowMenuDefaultAction::Close);
         return;
     }
-    if (event.modifiers() == Mod_Alt && event.code_point() && menubar()) {
+    if (event.modifiers() == Mod_Alt && event.code_point() && m_menubar.has_menus()) {
         Menu* menu_to_open = nullptr;
-        menubar()->for_each_menu([&](Menu& menu) {
+        m_menubar.for_each_menu([&](Menu& menu) {
             if (to_ascii_lowercase(menu.alt_shortcut_character()) == to_ascii_lowercase(event.code_point())) {
                 menu_to_open = &menu;
                 return IterationDecision::Break;
             }
             return IterationDecision::Continue;
         });
+
         if (menu_to_open) {
             frame().open_menubar_menu(*menu_to_open);
             if (!menu_to_open->is_empty())
@@ -874,8 +876,8 @@ void Window::popup_window_menu(const Gfx::IntPoint& position, WindowMenuDefaultA
     m_window_menu_maximize_item->set_default(default_action == WindowMenuDefaultAction::Maximize || default_action == WindowMenuDefaultAction::Restore);
     m_window_menu_maximize_item->set_icon(m_maximized ? &restore_icon() : &maximize_icon());
     m_window_menu_close_item->set_default(default_action == WindowMenuDefaultAction::Close);
-    m_window_menu_menubar_visibility_item->set_enabled(menubar());
-    m_window_menu_menubar_visibility_item->set_checked(menubar() && m_should_show_menubar);
+    m_window_menu_menubar_visibility_item->set_enabled(m_menubar.has_menus());
+    m_window_menu_menubar_visibility_item->set_checked(m_menubar.has_menus() && m_should_show_menubar);
 
     m_window_menu->popup(position);
 }
@@ -1236,32 +1238,16 @@ Optional<HitTestResult> Window::hit_test(Gfx::IntPoint const& position, bool inc
     };
 }
 
-void Window::set_menubar(Menubar* menubar)
+void Window::add_menu(Menu& menu)
 {
-    if (m_menubar == menubar)
-        return;
-    m_menubar = menubar;
-    if (m_menubar) {
-        // FIXME: Maybe move this to the theming system?
-        static constexpr auto menubar_menu_margin = 14;
-
-        auto& wm = WindowManager::the();
-        Gfx::IntPoint next_menu_location { 0, 0 };
-        auto menubar_rect = Gfx::WindowTheme::current().menubar_rect(Gfx::WindowTheme::WindowType::Normal, rect(), wm.palette(), 1);
-        m_menubar->for_each_menu([&](Menu& menu) {
-            int text_width = wm.font().width(Gfx::parse_ampersand_string(menu.name()));
-            menu.set_rect_in_window_menubar({ next_menu_location.x(), 0, text_width + menubar_menu_margin, menubar_rect.height() });
-            next_menu_location.translate_by(menu.rect_in_window_menubar().width(), 0);
-            return IterationDecision::Continue;
-        });
-    }
+    m_menubar.add_menu(menu, rect());
     Compositor::the().invalidate_occlusions();
     frame().invalidate();
 }
 
 void Window::invalidate_menubar()
 {
-    if (!m_should_show_menubar || !menubar())
+    if (!m_should_show_menubar || !m_menubar.has_menus())
         return;
     frame().invalidate_menubar();
 }
diff --git a/Userland/Services/WindowServer/Window.h b/Userland/Services/WindowServer/Window.h
index 17f3baf5e8..4d5c7771be 100644
--- a/Userland/Services/WindowServer/Window.h
+++ b/Userland/Services/WindowServer/Window.h
@@ -14,6 +14,7 @@
 #include <LibGfx/DisjointRectSet.h>
 #include <LibGfx/Rect.h>
 #include <WindowServer/Cursor.h>
+#include <WindowServer/Menubar.h>
 #include <WindowServer/Screen.h>
 #include <WindowServer/WindowFrame.h>
 #include <WindowServer/WindowType.h>
@@ -25,7 +26,6 @@ class ClientConnection;
 class Cursor;
 class KeyEvent;
 class Menu;
-class Menubar;
 class MenuItem;
 class MouseEvent;
 class WindowStack;
@@ -338,9 +338,10 @@ public:
     // area needs to be rendered
     auto& affected_transparency_rects() { return m_affected_transparency_rects; }
 
-    Menubar* menubar() { return m_menubar; }
-    const Menubar* menubar() const { return m_menubar; }
-    void set_menubar(Menubar*);
+    Menubar& menubar() { return m_menubar; }
+    Menubar const& menubar() const { return m_menubar; }
+
+    void add_menu(Menu& menu);
 
     WindowStack& window_stack()
     {
@@ -395,7 +396,7 @@ private:
     Vector<WeakPtr<Window>> m_child_windows;
     Vector<WeakPtr<Window>> m_accessory_windows;
 
-    RefPtr<Menubar> m_menubar;
+    Menubar m_menubar;
 
     String m_title;
     Gfx::IntRect m_rect;
diff --git a/Userland/Services/WindowServer/WindowFrame.cpp b/Userland/Services/WindowServer/WindowFrame.cpp
index 4465e124f4..c2e38e33b3 100644
--- a/Userland/Services/WindowServer/WindowFrame.cpp
+++ b/Userland/Services/WindowServer/WindowFrame.cpp
@@ -56,7 +56,7 @@ static Gfx::IntRect frame_rect_for_window(Window& window, const Gfx::IntRect& re
 {
     if (window.is_frameless())
         return rect;
-    int menu_row_count = (window.menubar() && window.should_show_menubar()) ? 1 : 0;
+    int menu_row_count = (window.menubar().has_menus() && window.should_show_menubar()) ? 1 : 0;
     return Gfx::WindowTheme::current().frame_rect_for_window(to_theme_window_type(window.type()), rect, WindowManager::the().palette(), menu_row_count);
 }
 
@@ -199,7 +199,7 @@ void WindowFrame::did_set_maximized(Badge<Window>, bool maximized)
 
 Gfx::IntRect WindowFrame::menubar_rect() const
 {
-    if (!m_window.menubar() || !m_window.should_show_menubar())
+    if (!m_window.menubar().has_menus() || !m_window.should_show_menubar())
         return {};
     return Gfx::WindowTheme::current().menubar_rect(to_theme_window_type(m_window.type()), m_window.rect(), WindowManager::the().palette(), menu_row_count());
 }
@@ -261,7 +261,7 @@ void WindowFrame::paint_menubar(Gfx::Painter& painter)
     painter.add_clip_rect(menubar_rect);
     painter.translate(menubar_rect.location());
 
-    m_window.menubar()->for_each_menu([&](Menu& menu) {
+    m_window.menubar().for_each_menu([&](Menu& menu) {
         auto text_rect = menu.rect_in_window_menubar();
         Color text_color = palette.window_text();
         auto is_open = menu.is_open();
@@ -283,7 +283,7 @@ void WindowFrame::paint_normal_frame(Gfx::Painter& painter)
     auto leftmost_button_rect = m_buttons.is_empty() ? Gfx::IntRect() : m_buttons.last().relative_rect();
     Gfx::WindowTheme::current().paint_normal_frame(painter, window_state_for_theme(), m_window.rect(), m_window.computed_title(), m_window.icon(), palette, leftmost_button_rect, menu_row_count(), m_window.is_modified());
 
-    if (m_window.menubar() && m_window.should_show_menubar())
+    if (m_window.menubar().has_menus() && m_window.should_show_menubar())
         paint_menubar(painter);
 }
 
@@ -812,7 +812,7 @@ void WindowFrame::handle_menubar_mouse_event(const MouseEvent& event)
     Menu* hovered_menu = nullptr;
     auto menubar_rect = this->menubar_rect();
     auto adjusted_position = event.position().translated(-menubar_rect.location());
-    m_window.menubar()->for_each_menu([&](Menu& menu) {
+    m_window.menubar().for_each_menu([&](Menu& menu) {
         if (menu.rect_in_window_menubar().contains(adjusted_position)) {
             hovered_menu = &menu;
             handle_menu_mouse_event(menu, event);
@@ -968,7 +968,7 @@ int WindowFrame::menu_row_count() const
 {
     if (!m_window.should_show_menubar())
         return 0;
-    return m_window.menubar() ? 1 : 0;
+    return m_window.menubar().has_menus() ? 1 : 0;
 }
 
 }
diff --git a/Userland/Services/WindowServer/WindowManager.h b/Userland/Services/WindowServer/WindowManager.h
index 5cd81a2f43..1c2d077000 100644
--- a/Userland/Services/WindowServer/WindowManager.h
+++ b/Userland/Services/WindowServer/WindowManager.h
@@ -20,7 +20,6 @@
 #include <WindowServer/Cursor.h>
 #include <WindowServer/Event.h>
 #include <WindowServer/MenuManager.h>
-#include <WindowServer/Menubar.h>
 #include <WindowServer/ScreenLayout.h>
 #include <WindowServer/WMClientConnection.h>
 #include <WindowServer/WindowSwitcher.h>
diff --git a/Userland/Services/WindowServer/WindowServer.ipc b/Userland/Services/WindowServer/WindowServer.ipc
index 38bf959770..14e7e0bd99 100644
--- a/Userland/Services/WindowServer/WindowServer.ipc
+++ b/Userland/Services/WindowServer/WindowServer.ipc
@@ -3,13 +3,10 @@
 
 endpoint WindowServer
 {
-    create_menubar(i32 menubar_id) =|
-    destroy_menubar(i32 menubar_id) =|
-
     create_menu(i32 menu_id, [UTF8] String menu_title) =|
     destroy_menu(i32 menu_id) =|
 
-    add_menu_to_menubar(i32 menubar_id, i32 menu_id) =|
+    add_menu(i32 window_id, i32 menu_id) =|
 
     add_menu_item(
         i32 menu_id,
@@ -53,8 +50,6 @@ endpoint WindowServer
 
     destroy_window(i32 window_id) => (Vector<i32> destroyed_window_ids)
 
-    set_window_menubar(i32 window_id, i32 menubar_id) =|
-
     set_window_title(i32 window_id, [UTF8] String title) =|
     get_window_title(i32 window_id) => ([UTF8] String title)