Merge #2223

2223: Differentiate between active surfaces and surfaces with keyboard input r=AlanGriffiths a=wmww

 Fixes #1626, fixes #2189. Adds a new `mir_window_focus_state_active` focus state, which is used for surfaces that are in the active stack but do not have keyboard focus. We did not previously have a way to represent this state properly, which caused us a number of problems.

Co-authored-by: William Wold <wm@wmww.sh>

include/core/mir_toolkit/common.h

@@ -88,8 +88,9 @@ typedef enum MirWindowState
 
 typedef enum MirWindowFocusState
 {
-    mir_window_focus_state_unfocused = 0,
-    mir_window_focus_state_focused
+    mir_window_focus_state_unfocused = 0,   /**< Inactive and does not have focus           */
+    mir_window_focus_state_focused,         /**< Active and has keybaord focus              */
+    mir_window_focus_state_active           /**< Active but does not have keyboard focus    */
 } MirWindowFocusState;
 
 typedef enum MirWindowVisibility

src/include/server/mir/shell/abstract_shell.h

@@ -169,13 +169,13 @@ class AbstractShell : public virtual Shell, public virtual FocusController
     std::mutex mutable focus_mutex;
     std::weak_ptr<scene::Surface> focus_surface;
     std::weak_ptr<scene::Session> focus_session;
-    std::weak_ptr<scene::Surface> notified_focus_surface;
+    std::vector<std::weak_ptr<scene::Surface>> notified_active_surfaces;
+    std::weak_ptr<scene::Surface> notified_keyboard_focus_surface;
+    std::weak_ptr<scene::Surface> notified_topmost_active_surface;
     std::shared_ptr<scene::SurfaceObserver> focus_surface_observer;
-    std::vector<std::weak_ptr<scene::Surface>> popups_of_focused_surface;
 
     void notify_focus_locked(
         std::unique_lock<std::mutex> const& lock,
-        std::shared_ptr<scene::Session> const& focus_session,
         std::shared_ptr<scene::Surface> const& focus_surface);
 
     void update_focus_locked(

src/server/frontend_wayland/pointer_constraints_unstable_v1.cpp

@@ -140,7 +140,7 @@ void LockedPointerV1::MyWaylandSurfaceObserver::attrib_changed(
         {
         case mir_pointer_locked_persistent:
         case mir_pointer_locked_oneshot:
-            if (value)
+            if (value != mir_window_focus_state_unfocused)
             {
                 wayland_executor.spawn([self=self]()
                     {
@@ -191,7 +191,7 @@ void ConfinedPointerV1::SurfaceObserver::attrib_changed(
         {
         case mir_pointer_confined_persistent:
         case mir_pointer_confined_oneshot:
-            if (value)
+            if (value != mir_window_focus_state_unfocused)
             {
                 wayland_executor.spawn([self=self]()
                     {

src/server/frontend_wayland/wayland_surface_observer.cpp

@@ -56,9 +56,9 @@ void mf::WaylandSurfaceObserver::attrib_changed(ms::Surface const*, MirWindowAtt
         run_on_wayland_thread_unless_window_destroyed(
             [value](Impl* impl, WindowWlSurfaceRole* window)
             {
-                auto has_focus = static_cast<bool>(value);
-                impl->input_dispatcher->set_focus(has_focus);
-                window->handle_active_change(has_focus);
+                auto state = static_cast<MirWindowFocusState>(value);
+                impl->input_dispatcher->set_focus(state == mir_window_focus_state_focused);
+                window->handle_active_change(state != mir_window_focus_state_unfocused);
             });
         break;
 

src/server/frontend_wayland/window_wl_surface_role.cpp

@@ -321,9 +321,14 @@ auto mf::WindowWlSurfaceRole::window_state() const -> MirWindowState
 auto mf::WindowWlSurfaceRole::is_active() const -> bool
 {
     if (auto const scene_surface = weak_scene_surface.lock())
-        return scene_surface->focus_state() == mir_window_focus_state_focused;
+    {
+        auto const state = scene_surface->focus_state();
+        return state != mir_window_focus_state_unfocused;
+    }
     else
+    {
         return false;
+    }
 }
 
 auto mf::WindowWlSurfaceRole::latest_timestamp() const -> std::chrono::nanoseconds
@@ -451,9 +456,10 @@ void mf::WindowWlSurfaceRole::create_scene_surface()
     {
         observer->content_resized_to(scene_surface.get(), content_size);
     }
-    if (is_active())
+    auto const focus_state = scene_surface->focus_state();
+    if (focus_state != mir_window_focus_state_unfocused)
     {
-        observer->attrib_changed(scene_surface.get(), mir_window_attrib_focus, 1);
+        observer->attrib_changed(scene_surface.get(), mir_window_attrib_focus, focus_state);
     }
 
     // Send wl_surface.enter events for every output

src/server/frontend_xwayland/xwayland_surface_observer.cpp

@@ -61,12 +61,12 @@ void mf::XWaylandSurfaceObserver::attrib_changed(ms::Surface const*, MirWindowAt
     {
     case mir_window_attrib_focus:
     {
-        auto has_focus = static_cast<bool>(value);
-        wm_surface->scene_surface_focus_set(has_focus);
+        auto state = static_cast<MirWindowFocusState>(value);
+        wm_surface->scene_surface_focus_set(state != mir_window_focus_state_unfocused);
         aquire_input_dispatcher(
-            [has_focus](auto input_dispatcher)
+            [state](auto input_dispatcher)
             {
-                input_dispatcher->set_focus(has_focus);
+                input_dispatcher->set_focus(state == mir_window_focus_state_focused);
             });
     }   break;
 

src/server/scene/basic_surface.cpp

@@ -988,6 +988,7 @@ auto mir::scene::BasicSurface::focus_state() const -> MirWindowFocusState
 void mir::scene::BasicSurface::set_focus_state(MirWindowFocusState new_state)
 {
     if (new_state != mir_window_focus_state_focused &&
+        new_state != mir_window_focus_state_active &&
         new_state != mir_window_focus_state_unfocused)
     {
         BOOST_THROW_EXCEPTION(std::logic_error("Invalid focus state."));

src/server/shell/abstract_shell.cpp

@@ -404,79 +404,66 @@ void msh::AbstractShell::set_focus_to(
     std::shared_ptr<ms::Session> const& focus_session,
     std::shared_ptr<ms::Surface> const& focus_surface)
 {
-    std::vector<std::shared_ptr<ms::Surface>> new_popups;
+    std::unique_lock<std::mutex> lock(focus_mutex);
+
+    notify_focus_locked(lock, focus_surface);
+    update_focus_locked(lock, focus_session, focus_surface);
+}
+
+void msh::AbstractShell::notify_focus_locked(
+    std::unique_lock<std::mutex> const& /*lock*/,
+    std::shared_ptr<ms::Surface> const& new_active_surface)
+{
+    auto const current_focus = notified_keyboard_focus_surface.lock();
 
     // Don't give keyboard focus to popups
-    auto surface = focus_surface;
-    while (surface)
+    auto new_keyboard_focus = new_active_surface;
+    while (new_keyboard_focus)
     {
-        auto const type = surface->type();
+        auto const type = new_keyboard_focus->type();
         if (type != mir_window_type_gloss &&
             type != mir_window_type_tip &&
             type != mir_window_type_menu)
         {
             break;
         }
-        new_popups.push_back(surface);
-        surface = surface->parent();
+        new_keyboard_focus = new_keyboard_focus->parent();
     }
 
-    std::unique_lock<std::mutex> lock(focus_mutex);
-
-    for (auto const& old_popup_weak : popups_of_focused_surface)
+    if (current_focus != new_keyboard_focus || notified_topmost_active_surface.lock() != new_active_surface)
     {
-        if (auto const old_popup = old_popup_weak.lock())
+        std::vector<std::shared_ptr<ms::Surface>> new_active_surfaces;
+        for (auto item = new_active_surface; item; item = item->parent())
         {
-            if (find(begin(new_popups), end(new_popups), old_popup) == end(new_popups))
-            {
-                // If the popup isn't in the set of new popups, hide it
-                old_popup->request_client_surface_close();
-                old_popup->hide();
-            }
+            new_active_surfaces.insert(begin(new_active_surfaces), item);
         }
-    }
-
-    popups_of_focused_surface.clear();
-    for (auto const& new_popup : new_popups)
-    {
-        popups_of_focused_surface.push_back(new_popup);
-    }
-
-    notify_focus_locked(lock, focus_session, surface);
-    update_focus_locked(lock, focus_session, surface);
-}
-
-void msh::AbstractShell::notify_focus_locked(
-    std::unique_lock<std::mutex> const& /*lock*/,
-    std::shared_ptr<ms::Session> const& /*session*/,
-    std::shared_ptr<ms::Surface> const& surface)
-{
-    auto const current_focus = notified_focus_surface.lock();
-
-    std::vector<std::shared_ptr<ms::Surface>> new_focus_tree;
-
-    for (auto item = surface; item; item = item->parent())
-    {
-        new_focus_tree.insert(begin(new_focus_tree), item);
-    }
 
-    std::vector<std::shared_ptr<ms::Surface>> current_focus_tree;
+        std::vector<std::shared_ptr<ms::Surface>> current_focus_tree;
 
-    for (auto item = current_focus; item; item = item->parent())
-    {
-        current_focus_tree.push_back(item);
-    }
-
-    if (surface != current_focus)
-    {
-        notified_focus_surface = surface;
+        notified_keyboard_focus_surface = new_keyboard_focus;
+        notified_topmost_active_surface = new_active_surface;
         seat->reset_confinement_regions();
 
-        for (auto const& item : current_focus_tree)
+        for (auto const& item : notified_active_surfaces)
         {
-            if (find(begin(new_focus_tree), end(new_focus_tree), item) == end(new_focus_tree))
+            if (auto const active = item.lock())
             {
-                item->set_focus_state(mir_window_focus_state_unfocused);
+                current_focus_tree.push_back(active);
+                if (find(begin(new_active_surfaces), end(new_active_surfaces), active) == end(new_active_surfaces))
+                {
+                    active->set_focus_state(mir_window_focus_state_unfocused);
+
+                    // When a menu loses focus we should close and unmap it
+                    if (active->type() == mir_window_type_menu || active->type() == mir_window_type_gloss)
+                    {
+                        active->request_client_surface_close();
+                        active->hide();
+                    }
+                }
+                else if (active == current_focus)
+                {
+                    active->set_focus_state(mir_window_focus_state_active);
+                }
             }
         }
 
@@ -497,21 +484,32 @@ void msh::AbstractShell::notify_focus_locked(
             }
         }
 
-        if (surface)
+        notified_active_surfaces.clear();
+        if (notified_active_surfaces.capacity() > 100)
         {
-            update_confinement_for(surface);
+            notified_active_surfaces.shrink_to_fit();
+        }
+
+        if (new_active_surface)
+        {
+            update_confinement_for(new_active_surface);
 
             // Ensure the surface has really taken the focus before notifying it that it is focused
-            input_targeter->set_focus(surface);
-            surface->consume(seat->create_device_state().get());
-            surface->add_observer(focus_surface_observer);
+            input_targeter->set_focus(new_active_surface);
+            new_active_surface->consume(seat->create_device_state().get());
+            new_active_surface->add_observer(focus_surface_observer);
 
-            for (auto const& item : new_focus_tree)
+            for (auto const& item : new_active_surfaces)
             {
-                if (find(begin(current_focus_tree), end(current_focus_tree), item) == end(current_focus_tree))
+                notified_active_surfaces.push_back(item);
+                if (item == new_keyboard_focus)
                 {
                     item->set_focus_state(mir_window_focus_state_focused);
                 }
+                else if (find(begin(current_focus_tree), end(current_focus_tree), item) == end(current_focus_tree))
+                {
+                    item->set_focus_state(mir_window_focus_state_active);
+                }
             }
         }
         else

src/server/shell/decoration/renderer.cpp

@@ -502,7 +502,7 @@ void msd::Renderer::update_state(WindowState const& window_state, InputState con
         titlebar_pixels.reset(); // force a reallocation next time it's needed
     }
 
-    Theme const* const new_theme = (window_state.focused_state() == mir_window_focus_state_focused) ?
+    Theme const* const new_theme = (window_state.focused_state() != mir_window_focus_state_unfocused) ?
         &focused_theme :
         &unfocused_theme;
 

tests/unit-tests/scene/test_basic_surface.cpp

@@ -823,8 +823,8 @@ AttributeTestParameters const surface_dpi_test_parameters{
 AttributeTestParameters const surface_focus_test_parameters{
     mir_window_attrib_focus,
     mir_window_focus_state_unfocused,
-    mir_window_focus_state_focused,
-    mir_window_focus_state_focused + 1,
+    mir_window_focus_state_active,
+    mir_window_focus_state_active + 1,
     mir_window_focus_state_unfocused - 1
 };
 
@@ -965,7 +965,7 @@ TEST_F(BasicSurfaceTest, throws_on_invalid_focus_state)
 {
     using namespace testing;
 
-    auto const invalid_state = static_cast<MirWindowFocusState>(mir_window_focus_state_focused + 1);
+    auto const invalid_state = static_cast<MirWindowFocusState>(mir_window_focus_state_active + 1);
 
     EXPECT_THROW({
             surface.configure(mir_window_attrib_focus, invalid_state);