Merge #1304 #1305

1304: Only modify surface if something changed r=AlanGriffiths a=wmww *Some* toolkits (\*cough\* Java \*cough\*) like to spam changes to window properties. No need to propagate that all the way through Mir. 1305: Improve keyboard focus situation r=AlanGriffiths a=wmww Reverts #825 and fixes #1274. This does not fix #1295, but in my opinion it is an improvement on the status quo. With the PR, Mir and Wayland/X keyboard focus state is allowed to fall out of sync, but the issue is detected and fixed before sending key events. The only visible symptom of the problem is in some cases (such as after closing a popup) a window appears to not have keyboard focus, but will gain it as soon as you start typing. None of the new code need be removed when a proper fix is implemented. The fallback of setting keyboard focus before sending keys is reasonable, and will mitigate against potential future bugs. Co-authored-by: William Wold <[email protected]> # Conflicts: # src/server/frontend_wayland/wl_keyboard.h
canonical · Feb 21, 2020 · 13765a9 · 13765a9
1 parent 9ce32b5
commit 13765a9
Show file tree

Hide file tree

Showing 8 changed files with 66 additions and 29 deletions.
diff --git a/src/server/frontend_wayland/wayland_input_dispatcher.cpp b/src/server/frontend_wayland/wayland_input_dispatcher.cpp
@@ -119,9 +119,9 @@ void mf::WaylandInputDispatcher::handle_keyboard_event(std::chrono::milliseconds
     {
         int const scancode = mir_keyboard_event_scan_code(event);
         bool const down = action == mir_keyboard_action_down;
-        seat->for_each_listener(client, [&ms, scancode, down](WlKeyboard* keyboard)
+        seat->for_each_listener(client, [&ms, wl_surface = wl_surface, scancode, down](WlKeyboard* keyboard)
             {
-                keyboard->key(ms, scancode, down);
+                keyboard->key(ms, wl_surface, scancode, down);
             });
     }
 }

diff --git a/src/server/frontend_wayland/wl_keyboard.cpp b/src/server/frontend_wayland/wl_keyboard.cpp
@@ -24,6 +24,7 @@
 #include "mir/executor.h"
 #include "mir/anonymous_shm_file.h"
 #include "mir/input/keymap.h"
+#include "mir/log.h"
 
 #include <xkbcommon/xkbcommon.h>
 #include <boost/throw_exception.hpp>
@@ -68,8 +69,16 @@ mf::WlKeyboard::~WlKeyboard()
     on_destroy(this);
 }
 
-void mf::WlKeyboard::key(std::chrono::milliseconds const& ms, int scancode, bool down)
+void mf::WlKeyboard::key(std::chrono::milliseconds const& ms, WlSurface* surface, int scancode, bool down)
 {
+    if (*focused_surface_destroyed || focused_surface != surface)
+    {
+        log_warning(
+            "Sending key to wl_surface@%u even though it was not explicitly given keyboard focus",
+            wl_resource_get_id(surface->resource));
+        focussed(surface, true);
+    }
+
     auto const serial = wl_display_next_serial(wl_client_get_display(client));
     /*
      * HACK! Maintain our own XKB state, so we can serialise it for
@@ -83,10 +92,20 @@ void mf::WlKeyboard::key(std::chrono::milliseconds const& ms, int scancode, bool
     update_modifier_state();
 }
 
-void mf::WlKeyboard::focussed(WlSurface* surface, bool focussed)
+void mf::WlKeyboard::focussed(WlSurface* surface, bool should_be_focused)
 {
-    auto const serial = wl_display_next_serial(wl_client_get_display(client));
-    if (focussed)
+    bool const is_currently_focused = (*focused_surface_destroyed == false && focused_surface == surface);
+
+    if (should_be_focused == is_currently_focused)
+        return;
+
+    if (*focused_surface_destroyed == false)
+    {
+        auto const serial = wl_display_next_serial(wl_client_get_display(client));
+        send_leave_event(serial, focused_surface->raw_resource());
+    }
+
+    if (should_be_focused)
     {
         // TODO: Send the surface's keymap here
 
@@ -114,12 +133,17 @@ void mf::WlKeyboard::focussed(WlSurface* surface, bool focussed)
                 keyboard_state.size() * sizeof(decltype(keyboard_state)::value_type));
         }
 
+        auto const serial = wl_display_next_serial(wl_client_get_display(client));
         send_enter_event(serial, surface->raw_resource(), &key_state);
         wl_array_release(&key_state);
+
+        focused_surface = surface;
+        focused_surface_destroyed = surface->destroyed_flag();
     }
     else
     {
-        send_leave_event(serial, surface->raw_resource());
+        focused_surface = nullptr;
+        focused_surface_destroyed = std::make_shared<bool>(true);
     }
 }
 

diff --git a/src/server/frontend_wayland/wl_keyboard.h b/src/server/frontend_wayland/wl_keyboard.h
@@ -55,8 +55,8 @@ class WlKeyboard : public wayland::Keyboard
 
     ~WlKeyboard();
 
-    void key(std::chrono::milliseconds const& ms, int scancode, bool down);
-    void focussed(WlSurface* surface, bool focussed);
+    void key(std::chrono::milliseconds const& ms, WlSurface* surface, int scancode, bool down);
+    void focussed(WlSurface* surface, bool should_be_focused);
     void set_keymap(char const* const buffer, size_t length);
     void set_keymap(mir::input::Keymap const& new_keymap);
     void resync_keyboard();
@@ -72,6 +72,9 @@ class WlKeyboard : public wayland::Keyboard
     std::function<void(WlKeyboard*)> on_destroy;
     std::function<std::vector<uint32_t>()> const acquire_current_keyboard_state;
 
+    std::shared_ptr<bool> focused_surface_destroyed{std::make_shared<bool>(true)};
+    WlSurface* focused_surface{nullptr}; ///< Should only be used if *focused_surface_destroyed == false
+
     uint32_t mods_depressed{0};
     uint32_t mods_latched{0};
     uint32_t mods_locked{0};

diff --git a/src/server/frontend_wayland/wl_seat.cpp b/src/server/frontend_wayland/wl_seat.cpp
@@ -204,10 +204,14 @@ void mf::WlSeat::for_each_listener(wl_client* client, std::function<void(WlTouch
     touch_listeners->for_each(client, func);
 }
 
-void mf::WlSeat::notify_focus(wl_client *focus) const
+void mf::WlSeat::notify_focus(wl_client *focus)
 {
-    for (auto const listener : focus_listeners)
-        listener->focus_on(focus);
+    if (focus != focused_client)
+    {
+        focused_client = focus;
+        for (auto const listener : focus_listeners)
+            listener->focus_on(focus);
+    }
 }
 
 void mf::WlSeat::spawn(std::function<void()>&& work)

diff --git a/src/server/frontend_wayland/wl_seat.h b/src/server/frontend_wayland/wl_seat.h
@@ -79,11 +79,12 @@ class WlSeat : public wayland::Seat::Global
 
     void add_focus_listener(ListenerTracker* listener);
     void remove_focus_listener(ListenerTracker* listener);
-    void notify_focus(wl_client* focus) const;
+    void notify_focus(wl_client* focus);
 
     void server_restart();
 
 private:
+    wl_client* focused_client{nullptr}; ///< Can be null
     std::vector<ListenerTracker*> focus_listeners;
 
     struct FocusClient : ListenerTracker

diff --git a/src/server/frontend_xwayland/xwayland_surface.cpp b/src/server/frontend_xwayland/xwayland_surface.cpp
@@ -492,7 +492,20 @@ void mf::XWaylandSurface::property_notify(xcb_atom_t property)
 
         if (spec && scene_surface)
         {
-            shell->modify_surface(scene_surface->session().lock(), scene_surface, *spec.value());
+            if (spec.value()->application_id.is_set() &&
+                spec.value()->application_id.value() == scene_surface->application_id())
+                spec.value()->application_id.consume();
+
+            if (spec.value()->name.is_set() &&
+                spec.value()->name.value() == scene_surface->name())
+                spec.value()->name.consume();
+
+            if (spec.value()->parent.is_set() &&
+                spec.value()->parent.value().lock() == scene_surface->parent())
+                spec.value()->parent.consume();
+
+            if (!spec.value()->is_empty())
+                shell->modify_surface(scene_surface->session().lock(), scene_surface, *spec.value());
         }
     }
 }

diff --git a/src/server/frontend_xwayland/xwayland_wm.cpp b/src/server/frontend_xwayland/xwayland_wm.cpp
@@ -328,14 +328,11 @@ void mf::XWaylandWM::set_focus(xcb_window_t xcb_window, bool should_be_focused)
             connection->net_active_window,
             static_cast<xcb_window_t>(XCB_WINDOW_NONE));
 
-        // TODO: enable clearing of input focus once github.com/MirServer/mir/issues/1295 is fixed
-        // at time of writing, focus is momentarily cleared when unmapping a popup
-        // clearing input focus results in the client closing any parent popups, so don't do that
-        // xcb_set_input_focus_checked(
-        //     *connection,
-        //     XCB_INPUT_FOCUS_POINTER_ROOT,
-        //     XCB_NONE,
-        //     XCB_CURRENT_TIME);
+        xcb_set_input_focus_checked(
+            *connection,
+            XCB_INPUT_FOCUS_POINTER_ROOT,
+            XCB_NONE,
+            XCB_CURRENT_TIME);
     }
 
     connection->flush();

diff --git a/src/server/shell/abstract_shell.cpp b/src/server/shell/abstract_shell.cpp
@@ -416,11 +416,7 @@ void msh::AbstractShell::notify_focus_locked(
 
         for (auto const& item : current_focus_tree)
         {
-            // It looks odd to unfocus `surface` here and focus it later, but this is a workaround
-            // for bug 823: There seems to be no easy alternative to prod the surface event source.
-            // TODO: find a better way.
-            if (find(begin(new_focus_tree), end(new_focus_tree), item) == end(new_focus_tree) ||
-                item == surface)
+            if (find(begin(new_focus_tree), end(new_focus_tree), item) == end(new_focus_tree))
             {
                 item->set_focus_state(mir_window_focus_state_unfocused);
             }
@@ -445,8 +441,7 @@ void msh::AbstractShell::notify_focus_locked(
 
             for (auto const& item : new_focus_tree)
             {
-                if (find(begin(current_focus_tree), end(current_focus_tree), item) == end(current_focus_tree) ||
-                    item == surface)
+                if (find(begin(current_focus_tree), end(current_focus_tree), item) == end(current_focus_tree))
                 {
                     item->set_focus_state(mir_window_focus_state_focused);
                 }