WindowServer: Rework and simplify Menu event handling

The menu manager will now send events directly to the current menu.
Previously if a menu was opened it would always be set as the current
menu. Now when opening a menu you can optionally say that you do not
want to have it as the current menu.

One scenerio when this happens is when a menu is popped up as part of a
preview, for example, when hovering over a menu item that is a submenu.

Sending the event to the current menu simplifies things and solves a few
inconsistencies in bevhaviour (such as hovering over a submenu, but key
events not being sent to the submenu).

Services/WindowServer/Menu.cpp

@@ -282,12 +282,12 @@ MenuItem* Menu::hovered_item() const
 void Menu::update_for_new_hovered_item()
 {
     if (hovered_item() && hovered_item()->is_submenu()) {
+        ASSERT(menu_window());
         MenuManager::the().close_everyone_not_in_lineage(*hovered_item()->submenu());
-        hovered_item()->submenu()->popup(hovered_item()->rect().top_right().translated(menu_window()->rect().location()), true);
+        hovered_item()->submenu()->popup(hovered_item()->rect().top_right().translated(menu_window()->rect().location()));
     } else {
         MenuManager::the().close_everyone_not_in_lineage(*this);
-        MenuManager::the().set_current_menu(this);
+        ensure_menu_window().set_visible(true);
-        menu_window()->set_visible(true);
     }
     redraw();
 }
@@ -306,17 +306,17 @@ void Menu::open_hovered_item()
 void Menu::descend_into_submenu_at_hovered_item()
 {
     ASSERT(hovered_item());
-    ASSERT(hovered_item()->is_submenu());
     auto submenu = hovered_item()->submenu();
-    submenu->m_hovered_item_index = 0;
+    ASSERT(submenu);
+    MenuManager::the().open_menu(*submenu);
+    submenu->set_hovered_item(0);
     ASSERT(submenu->hovered_item()->type() != MenuItem::Separator);
-    submenu->update_for_new_hovered_item();
-    m_in_submenu = true;
 }
 
 void Menu::handle_mouse_move_event(const MouseEvent& mouse_event)
 {
     ASSERT(menu_window());
+    MenuManager::the().set_current_menu(this);
     if (hovered_item() && hovered_item()->is_submenu()) {
 
         auto item = *hovered_item();
@@ -336,8 +336,6 @@ void Menu::handle_mouse_move_event(const MouseEvent& mouse_event)
         return;
     m_hovered_item_index = index;
 
-    // FIXME: Tell parent menu (if it exists) that it is currently in a submenu
-    m_in_submenu = false;
     update_for_new_hovered_item();
     return;
 }
@@ -385,23 +383,6 @@ void Menu::event(Core::Event& event)
             return;
         }
 
-        // Pass the event for the submenu that we are currently in to handle
-        if (m_in_submenu && key != Key_Left) {
-            ASSERT(hovered_item()->is_submenu());
-            hovered_item()->submenu()->dispatch_event(event);
-            return;
-        }
-
-        if (key == Key_Return) {
-            if (!hovered_item()->is_enabled())
-                return;
-            if (hovered_item()->is_submenu())
-                descend_into_submenu_at_hovered_item();
-            else
-                open_hovered_item();
-            return;
-        }
-
         if (key == Key_Up) {
             ASSERT(m_items.at(0).type() != MenuItem::Separator);
 
@@ -451,22 +432,6 @@ void Menu::event(Core::Event& event)
             update_for_new_hovered_item();
             return;
         }
-
-        if (key == Key_Left) {
-            if (!m_in_submenu)
-                return;
-
-            ASSERT(hovered_item()->is_submenu());
-            hovered_item()->submenu()->clear_hovered_item();
-            m_in_submenu = false;
-            return;
-        }
-
-        if (key == Key_Right) {
-            if (hovered_item()->is_enabled() && hovered_item()->is_submenu())
-                descend_into_submenu_at_hovered_item();
-            return;
-        }
     }
     Core::Object::event(event);
 }
@@ -476,7 +441,6 @@ void Menu::clear_hovered_item()
     if (!hovered_item())
         return;
     m_hovered_item_index = -1;
-    m_in_submenu = false;
     redraw();
 }
 
@@ -525,7 +489,7 @@ void Menu::redraw_if_theme_changed()
         redraw();
 }
 
-void Menu::popup(const Gfx::Point& position, bool is_submenu)
+void Menu::popup(const Gfx::Point& position)
 {
     if (is_empty()) {
         dbg() << "Menu: Empty menu popup";
@@ -550,8 +514,7 @@ void Menu::popup(const Gfx::Point& position, bool is_submenu)
 
     window.move_to(adjusted_pos);
     window.set_visible(true);
-    MenuManager::the().set_current_menu(this, is_submenu);
+    MenuManager::the().open_menu(*this, false);
-
     WindowManager::the().did_popup_a_menu({});
 }
 

Services/WindowServer/Menu.h

@@ -104,13 +104,20 @@ public:
     void redraw();
 
     MenuItem* hovered_item() const;
+
+    void set_hovered_item(int index)
+    {
+        m_hovered_item_index = index;
+        update_for_new_hovered_item();
+    }
+
     void clear_hovered_item();
 
     Function<void(MenuItem&)> on_item_activation;
 
     void close();
 
-    void popup(const Gfx::Point&, bool is_submenu = false);
+    void popup(const Gfx::Point&);
 
     bool is_menu_ancestor_of(const Menu&) const;
 
@@ -119,6 +126,9 @@ public:
     bool is_scrollable() const { return m_scrollable; }
     int scroll_offset() const { return m_scroll_offset; }
 
+    void descend_into_submenu_at_hovered_item();
+    void open_hovered_item();
+
 private:
     virtual void event(Core::Event&) override;
 
@@ -130,9 +140,7 @@ private:
     int item_index_at(const Gfx::Point&);
     int padding_between_text_and_shortcut() const { return 50; }
     void did_activate(MenuItem&);
-    void open_hovered_item();
     void update_for_new_hovered_item();
-    void descend_into_submenu_at_hovered_item();
 
     ClientConnection* m_client { nullptr };
     int m_menu_id { 0 };
@@ -148,7 +156,6 @@ private:
     Gfx::Point m_last_position_in_hover;
     int m_theme_index_at_last_paint { -1 };
     int m_hovered_item_index { -1 };
-    bool m_in_submenu { false };
 
     bool m_scrollable { false };
     int m_scroll_offset { 0 };

Services/WindowServer/MenuManager.cpp

@@ -134,11 +134,37 @@ void MenuManager::event(Core::Event& event)
         }
 
         if (event.type() == Event::KeyDown) {
-            for_each_active_menubar_menu([&](Menu& menu) {
+
-                if (is_open(menu))
+            if (key_event.key() == Key_Left) {
-                    menu.dispatch_event(event);
+                auto it = m_open_menu_stack.find([&](auto& other) { return m_current_menu == other.ptr(); });
-                return IterationDecision::Continue;
+                ASSERT(!it.is_end());
-            });
+
+                // Going "back" a menu should be the previous menu in the stack
+                if (it.index() > 0)
+                    set_current_menu(m_open_menu_stack.at(it.index() - 1));
+                close_everyone_not_in_lineage(*m_current_menu);
+                return;
+            }
+
+            if (key_event.key() == Key_Right) {
+                auto hovered_item = m_current_menu->hovered_item();
+                if (hovered_item && hovered_item->is_submenu())
+                    m_current_menu->descend_into_submenu_at_hovered_item();
+                return;
+            }
+
+            if (key_event.key() == Key_Return) {
+                auto hovered_item = m_current_menu->hovered_item();
+
+                if (!hovered_item->is_enabled())
+                    return;
+                if (hovered_item->is_submenu())
+                    m_current_menu->descend_into_submenu_at_hovered_item();
+                else
+                    m_current_menu->open_hovered_item();
+                return;
+            }
+            m_current_menu->dispatch_event(event);
         }
     }
 
@@ -227,6 +253,7 @@ void MenuManager::handle_menu_mouse_event(Menu& menu, const MouseEvent& event)
         m_bar_open = !m_bar_open;
 
     if (should_open_menu && m_bar_open) {
+        close_everyone();
         open_menu(menu);
         return;
     }
@@ -252,7 +279,8 @@ void MenuManager::close_all_menus_from_client(Badge<ClientConnection>, ClientCon
 void MenuManager::close_everyone()
 {
     for (auto& menu : m_open_menu_stack) {
-        if (menu && menu->menu_window())
+        ASSERT(menu);
+        if (menu->menu_window())
             menu->menu_window()->set_visible(false);
         menu->clear_hovered_item();
     }
@@ -316,40 +344,45 @@ void MenuManager::toggle_menu(Menu& menu)
     open_menu(menu);
 }
 
-void MenuManager::open_menu(Menu& menu)
+void MenuManager::open_menu(Menu& menu, bool as_current_menu)
 {
-    if (is_open(menu))
+    if (is_open(menu)) {
+        if (as_current_menu)
+            set_current_menu(&menu);
         return;
+    }
+
     if (!menu.is_empty()) {
         menu.redraw_if_theme_changed();
-        auto& menu_window = menu.ensure_menu_window();
+        if (!menu.menu_window()) {
-        menu_window.move_to({ menu.rect_in_menubar().x(), menu.rect_in_menubar().bottom() + 2 });
+            auto& menu_window = menu.ensure_menu_window();
-        menu_window.set_visible(true);
+            menu_window.move_to({ menu.rect_in_menubar().x(), menu.rect_in_menubar().bottom() + 2 });
+        }
+        menu.menu_window()->set_visible(true);
     }
-    set_current_menu(&menu);
+
+    if (m_open_menu_stack.find([&menu](auto& other) { return &menu == other.ptr(); }).is_end())
+        m_open_menu_stack.append(menu.make_weak_ptr());
+
+    if (as_current_menu)
+        set_current_menu(&menu);
+
     refresh();
 }
 
-void MenuManager::set_current_menu(Menu* menu, bool is_submenu)
+void MenuManager::set_current_menu(Menu* menu)
 {
-    if (menu == m_current_menu)
-        return;
-
-    if (!is_submenu) {
-        if (menu)
-            close_everyone_not_in_lineage(*menu);
-        else
-            close_everyone();
-    }
-
     if (!menu) {
         m_current_menu = nullptr;
         return;
     }
 
+    ASSERT(is_open(*menu));
+    if (menu == m_current_menu) {
+        return;
+    }
+
     m_current_menu = menu->make_weak_ptr();
-    if (m_open_menu_stack.find([menu](auto& other) { return menu == other.ptr(); }).is_end())
-        m_open_menu_stack.append(menu->make_weak_ptr());
 }
 
 void MenuManager::close_bar()

Services/WindowServer/MenuManager.h

@@ -53,8 +53,8 @@ public:
     void set_needs_window_resize();
 
     Menu* current_menu() { return m_current_menu.ptr(); }
-    void set_current_menu(Menu*, bool is_submenu = false);
+    void set_current_menu(Menu*);
-    void open_menu(Menu&);
+    void open_menu(Menu&, bool as_current_menu = true);
     void toggle_menu(Menu&);
 
     MenuBar* current_menubar() { return m_current_menubar.ptr(); }