From f60bd255f543a553ad2a4bf55fb06c57be7ca3e4 Mon Sep 17 00:00:00 2001 From: shannonbooth <35911232+shannonbooth@users.noreply.github.com> Date: Sat, 18 Jan 2020 20:56:51 +1300 Subject: [PATCH] WindowServer: Stop tracking hovered menu independently from the index (#1093) Tracking the hovered menu item independently from the index of the currently hovered item is error prone and bad code. Simplify WSMenu by only tracking the index of the currently hovered item. Fixes #1092 --- Servers/WindowServer/WSMenu.cpp | 95 ++++++++++++++++----------------- Servers/WindowServer/WSMenu.h | 7 ++- 2 files changed, 49 insertions(+), 53 deletions(-) diff --git a/Servers/WindowServer/WSMenu.cpp b/Servers/WindowServer/WSMenu.cpp index a7ea75178f..239ddda812 100644 --- a/Servers/WindowServer/WSMenu.cpp +++ b/Servers/WindowServer/WSMenu.cpp @@ -155,7 +155,7 @@ void WSMenu::draw() for (auto& item : m_items) { if (item.type() == WSMenuItem::Text) { Color text_color = palette.menu_base_text(); - if (&item == m_hovered_item && item.is_enabled()) { + if (&item == hovered_item() && item.is_enabled()) { painter.fill_rect(item.rect(), palette.menu_selection()); painter.draw_rect(item.rect(), palette.menu_selection().darkened()); text_color = palette.menu_selection_text(); @@ -207,12 +207,18 @@ void WSMenu::draw() } } +WSMenuItem* WSMenu::hovered_item() const +{ + if (m_hovered_item_index == -1) + return nullptr; + return const_cast(&item(m_hovered_item_index)); +} + void WSMenu::update_for_new_hovered_item() { - if (m_hovered_item && m_hovered_item->is_submenu()) { - ASSERT(m_hovered_item == &m_items.at(m_current_index)); - WSMenuManager::the().close_everyone_not_in_lineage(*m_hovered_item->submenu()); - m_hovered_item->submenu()->popup(m_hovered_item->rect().top_right().translated(menu_window()->rect().location()), true); + if (hovered_item() && hovered_item()->is_submenu()) { + WSMenuManager::the().close_everyone_not_in_lineage(*hovered_item()->submenu()); + hovered_item()->submenu()->popup(hovered_item()->rect().top_right().translated(menu_window()->rect().location()), true); } else { WSMenuManager::the().close_everyone_not_in_lineage(*this); WSMenuManager::the().set_current_menu(this); @@ -225,22 +231,20 @@ void WSMenu::open_hovered_item() { ASSERT(menu_window()); ASSERT(menu_window()->is_visible()); - if (!m_hovered_item) + if (!hovered_item()) return; - ASSERT(m_hovered_item == &m_items.at(m_current_index)); - if (m_hovered_item->is_enabled()) - did_activate(*m_hovered_item); + if (hovered_item()->is_enabled()) + did_activate(*hovered_item()); clear_hovered_item(); } void WSMenu::decend_into_submenu_at_hovered_item() { - ASSERT(m_hovered_item); - ASSERT(m_hovered_item->is_submenu()); - auto submenu = m_hovered_item->submenu(); - ASSERT(submenu->m_current_index == 0); - submenu->m_hovered_item = &submenu->m_items.at(0); - ASSERT(submenu->m_hovered_item->type() != WSMenuItem::Separator); + ASSERT(hovered_item()); + ASSERT(hovered_item()->is_submenu()); + auto submenu = hovered_item()->submenu(); + submenu->m_hovered_item_index = 0; + ASSERT(submenu->hovered_item()->type() != WSMenuItem::Separator); submenu->update_for_new_hovered_item(); m_in_submenu = true; } @@ -249,17 +253,12 @@ void WSMenu::event(CEvent& event) { if (event.type() == WSEvent::MouseMove) { ASSERT(menu_window()); - auto* item = item_at(static_cast(event).position()); - if (m_hovered_item == item) + int index = item_index_at(static_cast(event).position()); + if (m_hovered_item_index == index) return; - m_hovered_item = item; - - if (m_hovered_item) { - // FIXME: Tell parent menu (if it exists) that it is currently in a submenu - m_current_index = m_items.find([&](auto& item) { return item.ptr() == m_hovered_item; }).index(); - ASSERT(m_current_index != m_items.size()); - } + 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; @@ -280,21 +279,21 @@ void WSMenu::event(CEvent& event) ASSERT(menu_window()->is_visible()); // Default to the first item on key press if one has not been selected yet - if (!m_hovered_item) { - m_hovered_item = &m_items.at(0); + if (!hovered_item()) { + m_hovered_item_index = 0; update_for_new_hovered_item(); return; } // Pass the event for the submenu that we are currently in to handle if (m_in_submenu && key != Key_Left) { - ASSERT(m_hovered_item->is_submenu()); - m_hovered_item->submenu()->dispatch_event(event); + ASSERT(hovered_item()->is_submenu()); + hovered_item()->submenu()->dispatch_event(event); return; } if (key == Key_Return) { - if (m_hovered_item->is_submenu()) + if (hovered_item()->is_submenu()) decend_into_submenu_at_hovered_item(); else open_hovered_item(); @@ -305,11 +304,10 @@ void WSMenu::event(CEvent& event) ASSERT(m_items.at(0).type() != WSMenuItem::Separator); do { - m_current_index--; - if (m_current_index < 0) - m_current_index = m_items.size() - 1; - m_hovered_item = &m_items.at(m_current_index); - } while (m_hovered_item->type() == WSMenuItem::Separator); + m_hovered_item_index--; + if (m_hovered_item_index < 0) + m_hovered_item_index = m_items.size() - 1; + } while (hovered_item()->type() == WSMenuItem::Separator); update_for_new_hovered_item(); return; @@ -319,11 +317,10 @@ void WSMenu::event(CEvent& event) ASSERT(m_items.at(0).type() != WSMenuItem::Separator); do { - m_current_index++; - if (m_current_index >= m_items.size()) - m_current_index = 0; - m_hovered_item = &m_items.at(m_current_index); - } while (m_hovered_item->type() == WSMenuItem::Separator); + m_hovered_item_index++; + if (m_hovered_item_index >= m_items.size()) + m_hovered_item_index = 0; + } while (hovered_item()->type() == WSMenuItem::Separator); update_for_new_hovered_item(); return; @@ -333,15 +330,14 @@ void WSMenu::event(CEvent& event) if (!m_in_submenu) return; - m_hovered_item = &m_items.at(m_current_index); - ASSERT(m_hovered_item->is_submenu()); - m_hovered_item->submenu()->clear_hovered_item(); + ASSERT(hovered_item()->is_submenu()); + hovered_item()->submenu()->clear_hovered_item(); m_in_submenu = false; return; } if (key == Key_Right) { - if (m_hovered_item->is_submenu()) + if (hovered_item()->is_submenu()) decend_into_submenu_at_hovered_item(); return; } @@ -351,10 +347,9 @@ void WSMenu::event(CEvent& event) void WSMenu::clear_hovered_item() { - if (!m_hovered_item) + if (!hovered_item()) return; - m_current_index = 0; - m_hovered_item = nullptr; + m_hovered_item_index = -1; m_in_submenu = false; redraw(); } @@ -382,13 +377,15 @@ WSMenuItem* WSMenu::item_with_identifier(unsigned identifer) return nullptr; } -WSMenuItem* WSMenu::item_at(const Point& position) +int WSMenu::item_index_at(const Point& position) { + int i = 0; for (auto& item : m_items) { if (item.rect().contains(position)) - return &item; + return i; + ++i; } - return nullptr; + return -1; } void WSMenu::close() diff --git a/Servers/WindowServer/WSMenu.h b/Servers/WindowServer/WSMenu.h index 2bb34bae90..6e5e673b2b 100644 --- a/Servers/WindowServer/WSMenu.h +++ b/Servers/WindowServer/WSMenu.h @@ -71,10 +71,9 @@ public: const Font& font() const; WSMenuItem* item_with_identifier(unsigned); - WSMenuItem* item_at(const Point&); void redraw(); - const WSMenuItem* hovered_item() const { return m_hovered_item; } + WSMenuItem* hovered_item() const; void clear_hovered_item(); Function on_item_activation; @@ -90,6 +89,7 @@ public: private: virtual void event(CEvent&) override; + int item_index_at(const Point&); int padding_between_text_and_shortcut() const { return 50; } void did_activate(WSMenuItem&); void open_hovered_item(); @@ -102,7 +102,6 @@ private: Rect m_rect_in_menubar; Rect m_text_rect_in_menubar; WSMenuBar* m_menubar { nullptr }; - WSMenuItem* m_hovered_item { nullptr }; NonnullOwnPtrVector m_items; RefPtr m_menu_window; @@ -110,6 +109,6 @@ private: bool m_is_window_menu_open = { false }; int m_theme_index_at_last_paint { -1 }; - int m_current_index { 0 }; + int m_hovered_item_index { -1 }; bool m_in_submenu { false }; };