From 6c049ea4c4ebc95cb970985f01eeed461b453991 Mon Sep 17 00:00:00 2001
From: bugreport0 <32939607+bugreport0@users.noreply.github.com>
Date: Sun, 3 Oct 2021 12:33:08 +0200
Subject: [PATCH] LibGUI+WindowServer: Flash menubar menu when using a keyboard
 shortcut

Briefly flash the menubar menu containing the keyboard shortcut action
to give the user immediate visual feedback on their interaction with the
system.
---
 Userland/Libraries/LibGUI/Action.cpp          |  8 +++++
 Userland/Libraries/LibGUI/Action.h            |  1 +
 Userland/Libraries/LibGUI/MenuItem.h          |  1 +
 Userland/Libraries/LibGUI/Window.cpp          | 10 ++++++
 Userland/Libraries/LibGUI/Window.h            |  1 +
 .../LibGUI/WindowServerConnection.cpp         |  1 +
 .../WindowServer/ClientConnection.cpp         | 34 +++++++++++++++++++
 .../Services/WindowServer/ClientConnection.h  |  2 ++
 Userland/Services/WindowServer/Menubar.h      | 21 ++++++++++++
 .../Services/WindowServer/WindowFrame.cpp     |  9 ++++-
 .../Services/WindowServer/WindowServer.ipc    |  1 +
 11 files changed, 88 insertions(+), 1 deletion(-)

diff --git a/Userland/Libraries/LibGUI/Action.cpp b/Userland/Libraries/LibGUI/Action.cpp
index 9b881777da..89beb9082c 100644
--- a/Userland/Libraries/LibGUI/Action.cpp
+++ b/Userland/Libraries/LibGUI/Action.cpp
@@ -135,6 +135,14 @@ void Action::activate(Core::Object* activator)
     m_activator = nullptr;
 }
 
+void Action::flash_menubar_menu()
+{
+    if (auto* app = Application::the())
+        if (auto* window = app->active_window())
+            for (auto& menu_item : m_menu_items)
+                window->flash_menubar_menu_for(*menu_item);
+}
+
 void Action::register_button(Badge<Button>, Button& button)
 {
     m_buttons.set(&button);
diff --git a/Userland/Libraries/LibGUI/Action.h b/Userland/Libraries/LibGUI/Action.h
index 5d0aed769a..53b93e30d0 100644
--- a/Userland/Libraries/LibGUI/Action.h
+++ b/Userland/Libraries/LibGUI/Action.h
@@ -92,6 +92,7 @@ public:
     Function<void(Action&)> on_activation;
 
     void activate(Core::Object* activator = nullptr);
+    void flash_menubar_menu();
 
     bool is_enabled() const { return m_enabled; }
     void set_enabled(bool);
diff --git a/Userland/Libraries/LibGUI/MenuItem.h b/Userland/Libraries/LibGUI/MenuItem.h
index b67cab9f5c..7e5fd1c8ff 100644
--- a/Userland/Libraries/LibGUI/MenuItem.h
+++ b/Userland/Libraries/LibGUI/MenuItem.h
@@ -48,6 +48,7 @@ public:
     bool is_default() const { return m_default; }
     void set_default(bool);
 
+    int menu_id() const { return m_menu_id; }
     void set_menu_id(Badge<Menu>, unsigned menu_id);
     void set_identifier(Badge<Menu>, unsigned identifier);
 
diff --git a/Userland/Libraries/LibGUI/Window.cpp b/Userland/Libraries/LibGUI/Window.cpp
index d0e79ae667..d9b74a57e2 100644
--- a/Userland/Libraries/LibGUI/Window.cpp
+++ b/Userland/Libraries/LibGUI/Window.cpp
@@ -16,6 +16,7 @@
 #include <LibGUI/Application.h>
 #include <LibGUI/Desktop.h>
 #include <LibGUI/Event.h>
+#include <LibGUI/MenuItem.h>
 #include <LibGUI/Menubar.h>
 #include <LibGUI/Painter.h>
 #include <LibGUI/Widget.h>
@@ -1204,6 +1205,15 @@ Menu& Window::add_menu(String name)
     return *menu;
 }
 
+void Window::flash_menubar_menu_for(const MenuItem& menu_item)
+{
+    auto menu_id = menu_item.menu_id();
+    if (menu_id < 0)
+        return;
+
+    WindowServerConnection::the().async_flash_menubar_menu(m_window_id, menu_id);
+}
+
 bool Window::is_modified() const
 {
     if (!m_window_id)
diff --git a/Userland/Libraries/LibGUI/Window.h b/Userland/Libraries/LibGUI/Window.h
index 8cca40b193..dffb0adaa9 100644
--- a/Userland/Libraries/LibGUI/Window.h
+++ b/Userland/Libraries/LibGUI/Window.h
@@ -211,6 +211,7 @@ public:
 
     Menu& add_menu(String name);
     ErrorOr<NonnullRefPtr<Menu>> try_add_menu(String name);
+    void flash_menubar_menu_for(const MenuItem&);
 
     void flush_pending_paints_immediately();
 
diff --git a/Userland/Libraries/LibGUI/WindowServerConnection.cpp b/Userland/Libraries/LibGUI/WindowServerConnection.cpp
index c3ba84086e..32e93e18d7 100644
--- a/Userland/Libraries/LibGUI/WindowServerConnection.cpp
+++ b/Userland/Libraries/LibGUI/WindowServerConnection.cpp
@@ -172,6 +172,7 @@ void WindowServerConnection::key_down(i32 window_id, u32 code_point, u32 key, u3
 
     if (auto* action = action_for_key_event(*window, *key_event)) {
         if (action->is_enabled()) {
+            action->flash_menubar_menu();
             action->activate();
             return;
         }
diff --git a/Userland/Services/WindowServer/ClientConnection.cpp b/Userland/Services/WindowServer/ClientConnection.cpp
index 25feecad82..33f191238f 100644
--- a/Userland/Services/WindowServer/ClientConnection.cpp
+++ b/Userland/Services/WindowServer/ClientConnection.cpp
@@ -192,6 +192,40 @@ void ClientConnection::update_menu_item(i32 menu_id, i32 identifier, [[maybe_unu
         menu_item->set_checked(checked);
 }
 
+void ClientConnection::flash_menubar_menu(i32 window_id, i32 menu_id)
+{
+    auto itw = m_windows.find(window_id);
+    if (itw == m_windows.end()) {
+        did_misbehave("FlashMenubarMenu: Bad window ID");
+        return;
+    }
+    auto& window = *(*itw).value;
+
+    auto itm = m_menus.find(menu_id);
+    if (itm == m_menus.end()) {
+        did_misbehave("FlashMenubarMenu: Bad menu ID");
+        return;
+    }
+    auto& menu = *(*itm).value;
+
+    if (window.menubar().flash_menu(&menu)) {
+        window.frame().invalidate_menubar();
+
+        if (m_flashed_menu_timer && m_flashed_menu_timer->is_active()) {
+            m_flashed_menu_timer->on_timeout();
+            m_flashed_menu_timer->stop();
+        }
+
+        m_flashed_menu_timer = Core::Timer::create_single_shot(75, [&window] {
+            window.menubar().flash_menu(nullptr);
+            window.frame().invalidate_menubar();
+        });
+        m_flashed_menu_timer->start();
+    } else if (m_flashed_menu_timer) {
+        m_flashed_menu_timer->restart();
+    }
+}
+
 void ClientConnection::add_menu_separator(i32 menu_id)
 {
     auto it = m_menus.find(menu_id);
diff --git a/Userland/Services/WindowServer/ClientConnection.h b/Userland/Services/WindowServer/ClientConnection.h
index 5821bd95c9..9db8ed10b6 100644
--- a/Userland/Services/WindowServer/ClientConnection.h
+++ b/Userland/Services/WindowServer/ClientConnection.h
@@ -97,6 +97,7 @@ private:
     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;
+    virtual void flash_menubar_menu(i32, i32) override;
     virtual void create_window(i32, Gfx::IntRect const&, bool, bool, bool, bool, bool,
         bool, bool, bool, bool, bool, float, float, Gfx::IntSize const&, Gfx::IntSize const&, Gfx::IntSize const&,
         Optional<Gfx::IntSize> const&, i32, String const&, i32, Gfx::IntRect const&) override;
@@ -178,6 +179,7 @@ private:
     HashMap<int, NonnullRefPtr<Window>> m_windows;
     HashMap<int, NonnullRefPtr<Menu>> m_menus;
 
+    RefPtr<Core::Timer> m_flashed_menu_timer;
     RefPtr<Core::Timer> m_ping_timer;
 
     bool m_has_display_link { false };
diff --git a/Userland/Services/WindowServer/Menubar.h b/Userland/Services/WindowServer/Menubar.h
index 3463f8e5f5..a670444e44 100644
--- a/Userland/Services/WindowServer/Menubar.h
+++ b/Userland/Services/WindowServer/Menubar.h
@@ -23,6 +23,26 @@ public:
         layout_menu(menu, window_rect);
     }
 
+    bool flash_menu(Menu* flashed_submenu)
+    {
+        Menu* const old_flashed_menu = m_flashed_menu;
+        m_flashed_menu = nullptr;
+
+        if (flashed_submenu) {
+            for_each_menu([&](Menu& menu) {
+                if ((&menu) == flashed_submenu || menu.is_menu_ancestor_of(*flashed_submenu)) {
+                    m_flashed_menu = &menu;
+                    return IterationDecision::Break;
+                }
+                return IterationDecision::Continue;
+            });
+        }
+
+        return (old_flashed_menu != m_flashed_menu);
+    }
+
+    Menu* flashed_menu() const { return m_flashed_menu; }
+
     bool has_menus()
     {
         return !m_menus.is_empty();
@@ -40,6 +60,7 @@ private:
     void layout_menu(Menu&, Gfx::IntRect window_rect);
 
     Vector<Menu&> m_menus;
+    Menu* m_flashed_menu { nullptr };
 
     // FIXME: This doesn't support removing menus from a menubar or inserting a
     //        menu in the middle.
diff --git a/Userland/Services/WindowServer/WindowFrame.cpp b/Userland/Services/WindowServer/WindowFrame.cpp
index 401b2b61a2..230e7948d1 100644
--- a/Userland/Services/WindowServer/WindowFrame.cpp
+++ b/Userland/Services/WindowServer/WindowFrame.cpp
@@ -264,8 +264,15 @@ void WindowFrame::paint_menubar(Gfx::Painter& painter)
     painter.translate(menubar_rect.location());
 
     m_window.menubar().for_each_menu([&](Menu& menu) {
+        bool paint_as_flashed = ((&menu) == m_window.menubar().flashed_menu());
+        if (paint_as_flashed) {
+            auto flashed_rect = menu.rect_in_window_menubar();
+            flashed_rect.shrink(2, 2);
+            painter.fill_rect(flashed_rect, palette.selection());
+        }
+
         auto text_rect = menu.rect_in_window_menubar();
-        Color text_color = palette.window_text();
+        Color text_color = (paint_as_flashed ? palette.selection_text() : palette.window_text());
         auto is_open = menu.is_open();
         if (is_open)
             text_rect.translate_by(1, 1);
diff --git a/Userland/Services/WindowServer/WindowServer.ipc b/Userland/Services/WindowServer/WindowServer.ipc
index 43ae877718..012e29e7e6 100644
--- a/Userland/Services/WindowServer/WindowServer.ipc
+++ b/Userland/Services/WindowServer/WindowServer.ipc
@@ -24,6 +24,7 @@ endpoint WindowServer
     add_menu_separator(i32 menu_id) =|
 
     update_menu_item(i32 menu_id, i32 identifier, i32 submenu_id, [UTF8] String text, bool enabled, bool checkable, bool checked, bool is_default, [UTF8] String shortcut) =|
+    flash_menubar_menu(i32 window_id, i32 menu_id) =|
 
     create_window(
         i32 window_id,