Merge #1264

1264: Focus state r=AlanGriffiths a=wmww Fixes #1206 and other issues related to window focus. Also organizes properties from the X server into a `cached` struct. Co-authored-by: William Wold <[email protected]>
canonical · Feb 17, 2020 · ec8f06e · ec8f06e
1 parent e383085
commit ec8f06e
Show file tree

Hide file tree

Showing 6 changed files with 157 additions and 41 deletions.
diff --git a/src/server/frontend_xwayland/xwayland_surface.cpp b/src/server/frontend_xwayland/xwayland_surface.cpp
@@ -193,7 +193,7 @@ mf::XWaylandSurface::XWaylandSurface(
                   {
                       std::lock_guard<std::mutex> lock{this->mutex};
                       this->pending_spec(lock).parent = parent_scene_surface;
-                      set_position(parent_scene_surface, this->latest_position, this->pending_spec(lock));
+                      set_position(parent_scene_surface, this->cached.top_left, this->pending_spec(lock));
                   }
               },
               [this]()
@@ -208,16 +208,18 @@ mf::XWaylandSurface::XWaylandSurface(
               [this](std::vector<xcb_atom_t> const& value)
               {
                   std::lock_guard<std::mutex> lock{mutex};
-                  this->supported_wm_protocols = std::set<xcb_atom_t>{value.begin(), value.end()};
+                  this->cached.supported_wm_protocols = std::set<xcb_atom_t>{value.begin(), value.end()};
               },
               [this]()
               {
                   std::lock_guard<std::mutex> lock{mutex};
-                  this->supported_wm_protocols.clear();
-              })},
-      latest_size{event->width, event->height},
-      latest_position{event->x, event->y}
+                  this->cached.supported_wm_protocols.clear();
+              })}
 {
+    cached.override_redirect = event->override_redirect;
+    cached.size = {event->width, event->height};
+    cached.top_left = {event->x, event->y};
+
     uint32_t const value = XCB_EVENT_MASK_PROPERTY_CHANGE | XCB_EVENT_MASK_FOCUS_CHANGE;
     xcb_change_window_attributes(*connection, window, XCB_CW_EVENT_MASK, &value);
 }
@@ -232,7 +234,7 @@ void mf::XWaylandSurface::map()
     WindowState state;
     {
         std::lock_guard<std::mutex> lock{mutex};
-        state = window_state;
+        state = cached.state;
     }
 
     uint32_t const workspace = 1;
@@ -257,7 +259,7 @@ void mf::XWaylandSurface::close()
     {
         std::lock_guard<std::mutex> lock{mutex};
 
-        state = window_state;
+        state = cached.state;
 
         scene_surface = weak_scene_surface.lock();
         weak_scene_surface.reset();
@@ -307,6 +309,36 @@ void mf::XWaylandSurface::close()
     }
 }
 
+void mf::XWaylandSurface::take_focus()
+{
+    {
+        std::lock_guard<std::mutex> lock{mutex};
+
+        if (cached.override_redirect)
+            return;
+    }
+
+    // We may want to respect requested input focus model here
+    // see https://tronche.com/gui/x/icccm/sec-4.html#s-4.1.7
+
+    uint32_t const client_message_data[]{
+        connection->wm_take_focus,
+        XCB_TIME_CURRENT_TIME};
+
+    connection->send_client_message<XCBType::WM_PROTOCOLS>(
+        window,
+        XCB_EVENT_MASK_SUBSTRUCTURE_REDIRECT,
+        client_message_data);
+
+    xcb_set_input_focus(
+        *connection,
+        XCB_INPUT_FOCUS_POINTER_ROOT,
+        window,
+        XCB_CURRENT_TIME);
+
+    connection->flush();
+}
+
 void mf::XWaylandSurface::configure_request(xcb_configure_request_event_t* event)
 {
     std::shared_ptr<scene::Surface> scene_surface;
@@ -363,8 +395,9 @@ void mf::XWaylandSurface::configure_request(xcb_configure_request_event_t* event
 void mf::XWaylandSurface::configure_notify(xcb_configure_notify_event_t* event)
 {
     std::lock_guard<std::mutex> lock{mutex};
-    latest_position = geom::Point{event->x, event->y},
-    latest_size = geom::Size{event->width, event->height};
+    cached.override_redirect = event->override_redirect;
+    cached.top_left = geom::Point{event->x, event->y},
+    cached.size = geom::Size{event->width, event->height};
 }
 
 void mf::XWaylandSurface::net_wm_state_client_message(uint32_t const (&data)[5])
@@ -391,7 +424,7 @@ void mf::XWaylandSurface::net_wm_state_client_message(uint32_t const (&data)[5])
     {
         std::lock_guard<std::mutex> lock{mutex};
 
-        new_window_state = window_state;
+        new_window_state = cached.state;
 
         for (xcb_atom_t const property : properties)
         {
@@ -431,7 +464,7 @@ void mf::XWaylandSurface::wm_change_state_client_message(uint32_t const (&data)[
     {
         std::lock_guard<std::mutex> lock{mutex};
 
-        new_window_state = window_state;
+        new_window_state = cached.state;
 
         switch (requested_state)
         {
@@ -606,12 +639,17 @@ auto mf::XWaylandSurface::WindowState::updated_from(MirWindowState state) const
     return updated;
 }
 
+void mf::XWaylandSurface::scene_surface_focus_set(bool has_focus)
+{
+    xwm->set_focus(window, has_focus);
+}
+
 void mf::XWaylandSurface::scene_surface_state_set(MirWindowState new_state)
 {
     WindowState state;
     {
         std::lock_guard<std::mutex> lock{mutex};
-        state = window_state.updated_from(new_state);
+        state = cached.state.updated_from(new_state);
     }
     inform_client_of_window_state(state);
 }
@@ -639,7 +677,9 @@ void mf::XWaylandSurface::scene_surface_close_requested()
     bool delete_window;
     {
         std::lock_guard<std::mutex> lock{mutex};
-        delete_window = (supported_wm_protocols.find(connection->wm_delete_window) != supported_wm_protocols.end());
+        delete_window = (
+            cached.supported_wm_protocols.find(connection->wm_delete_window) !=
+            cached.supported_wm_protocols.end());
     }
 
     if (delete_window)
@@ -727,7 +767,7 @@ void mf::XWaylandSurface::create_scene_surface_if_needed()
             log_debug("creating scene surface for %s", connection->window_debug_string(window).c_str());
         }
 
-        state = window_state;
+        state = cached.state;
         state.withdrawn = false;
 
         observer = surface_observer.value();
@@ -736,25 +776,15 @@ void mf::XWaylandSurface::create_scene_surface_if_needed()
         params.input_shape = std::move(initial_wl_surface_data.value()->input_shape);
         initial_wl_surface_data = std::experimental::nullopt;
 
-        params.size = latest_size;
-        params.top_left = latest_position;
+        params.size = cached.size;
+        params.top_left = cached.top_left;
         params.type = mir_window_type_freestyle;
         params.state = state.mir_window_state();
+        params.server_side_decorated = !cached.override_redirect;
     }
 
     std::vector<std::function<void()>> reply_functions;
 
-    auto const window_attrib_cookie = xcb_get_window_attributes(*connection, window);
-    reply_functions.push_back([this, &params, window_attrib_cookie]
-        {
-            auto const reply = xcb_get_window_attributes_reply(*connection, window_attrib_cookie, nullptr);
-            if (reply)
-            {
-                params.server_side_decorated = !reply->override_redirect;
-                free(reply);
-            }
-        });
-
     // Read all properties
     for (auto const& handler : property_handlers)
     {
@@ -803,10 +833,10 @@ void mf::XWaylandSurface::inform_client_of_window_state(WindowState const& new_w
     {
         std::lock_guard<std::mutex> lock{mutex};
 
-        if (new_window_state == window_state)
+        if (new_window_state == cached.state)
             return;
 
-        window_state = new_window_state;
+        cached.state = new_window_state;
     }
 
     WmState wm_state;

diff --git a/src/server/frontend_xwayland/xwayland_surface.h b/src/server/frontend_xwayland/xwayland_surface.h
@@ -58,6 +58,7 @@ class XWaylandSurface
 
     void map();
     void close(); ///< Idempotent
+    void take_focus();
     void configure_request(xcb_configure_request_event_t* event);
     void configure_notify(xcb_configure_notify_event_t* event);
     void net_wm_state_client_message(uint32_t const (&data)[5]);
@@ -85,6 +86,7 @@ class XWaylandSurface
 
     /// Overrides from XWaylandSurfaceObserverSurface
     /// @{
+    void scene_surface_focus_set(bool has_focus) override;
     void scene_surface_state_set(MirWindowState new_state) override;
     void scene_surface_resized(geometry::Size const& new_size) override;
     void scene_surface_moved_to(geometry::Point const& new_top_left) override;
@@ -132,15 +134,21 @@ class XWaylandSurface
 
     std::mutex mutable mutex;
 
-    /// Reflects the _NET_WM_STATE and WM_STATE we have currently set on the window
-    /// Should only be modified by set_wm_state()
-    WindowState window_state;
+    /// Cached version of properties on the X server
+    struct
+    {
+        /// Reflects the _NET_WM_STATE and WM_STATE we have currently set on the window
+        /// Should only be modified by set_wm_state()
+        WindowState state;
+
+        bool override_redirect;
 
-    geometry::Size latest_size;
-    geometry::Point latest_position; ///< Always in global coordinates
+        geometry::Size size;
+        geometry::Point top_left; ///< Always in global coordinates
 
-    /// The contents of the _NET_SUPPORTED property set by the client
-    std::set<xcb_atom_t> supported_wm_protocols;
+        /// The contents of the _NET_SUPPORTED property set by the client
+        std::set<xcb_atom_t> supported_wm_protocols;
+    } cached;
 
     /// Set in set_wl_surface and cleared when a scene surface is created from it
     std::experimental::optional<std::unique_ptr<InitialWlSurfaceData>> initial_wl_surface_data;

diff --git a/src/server/frontend_xwayland/xwayland_surface_observer.cpp b/src/server/frontend_xwayland/xwayland_surface_observer.cpp
@@ -58,7 +58,7 @@ void mf::XWaylandSurfaceObserver::attrib_changed(ms::Surface const*, MirWindowAt
     case mir_window_attrib_focus:
     {
         auto has_focus = static_cast<bool>(value);
-        /// TODO: update the active state on the wm_window
+        wm_surface->scene_surface_focus_set(has_focus);
         aquire_input_dispatcher(
             [has_focus](auto input_dispatcher)
             {

diff --git a/src/server/frontend_xwayland/xwayland_surface_observer_surface.h b/src/server/frontend_xwayland/xwayland_surface_observer_surface.h
@@ -32,6 +32,7 @@ namespace frontend
 class XWaylandSurfaceObserverSurface
 {
 public:
+    virtual void scene_surface_focus_set(bool has_focus) = 0;
     virtual void scene_surface_state_set(MirWindowState new_state) = 0;
     virtual void scene_surface_resized(geometry::Size const& new_size) = 0;
     virtual void scene_surface_moved_to(geometry::Point const& new_top_left) = 0;

diff --git a/src/server/frontend_xwayland/xwayland_wm.cpp b/src/server/frontend_xwayland/xwayland_wm.cpp
@@ -279,6 +279,56 @@ auto mf::XWaylandWM::get_wm_surface(
         return surface->second;
 }
 
+void mf::XWaylandWM::set_focus(xcb_window_t xcb_window, bool should_be_focused)
+{
+    bool was_focused;
+    {
+        std::lock_guard<std::mutex> lock{mutex};
+        was_focused = (focused_window && focused_window.value() == xcb_window);
+        focused_window = should_be_focused;
+    }
+
+    if (verbose_xwayland_logging_enabled())
+    {
+        log_debug(
+            "%s %s %s...",
+            should_be_focused ? "Focusing" : "Unfocusing",
+            was_focused ? "focused" : "unfocused",
+            connection->window_debug_string(xcb_window).c_str());
+    }
+
+    if (should_be_focused == was_focused)
+        return;
+
+    if (should_be_focused)
+    {
+        connection->set_property<XCBType::WINDOW>(
+            connection->root_window(),
+            connection->net_active_window,
+            static_cast<xcb_window_t>(xcb_window));
+
+        if (auto const surface = get_wm_surface(xcb_window))
+        {
+            surface.value()->take_focus();
+        }
+    }
+    else
+    {
+        connection->set_property<XCBType::WINDOW>(
+            connection->root_window(),
+            connection->net_active_window,
+            static_cast<xcb_window_t>(XCB_WINDOW_NONE));
+
+        xcb_set_input_focus_checked(
+            *connection,
+            XCB_INPUT_FOCUS_POINTER_ROOT,
+            XCB_NONE,
+            XCB_CURRENT_TIME);
+    }
+
+    connection->flush();
+}
+
 void mf::XWaylandWM::run_on_wayland_thread(std::function<void()>&& work)
 {
     wayland_connector->run_on_wayland_display([work = move(work)](auto){ work(); });
@@ -377,9 +427,7 @@ void mf::XWaylandWM::handle_event(xcb_generic_event_t* event)
         handle_client_message(reinterpret_cast<xcb_client_message_event_t *>(event));
         break;
     case XCB_FOCUS_IN:
-        if (verbose_xwayland_logging_enabled())
-            log_debug("XCB_FOCUS_IN");
-        //(reinterpret_cast<xcb_focus_in_event_t *>(event));
+        handle_focus_in(reinterpret_cast<xcb_focus_in_event_t*>(event));
     default:
         break;
     }
@@ -641,6 +689,32 @@ void mf::XWaylandWM::handle_configure_notify(xcb_configure_notify_event_t *event
     }
 }
 
+void mf::XWaylandWM::handle_focus_in(xcb_focus_in_event_t* event)
+{
+    if (verbose_xwayland_logging_enabled())
+    {
+        std::string mode_str;
+        switch (event->mode)
+        {
+        case XCB_NOTIFY_MODE_NORMAL:        mode_str = "normal focus"; break;
+        case XCB_NOTIFY_MODE_GRAB:          mode_str = "focus grabbed"; break;
+        case XCB_NOTIFY_MODE_UNGRAB:        mode_str = "focus ungrabbed"; break;
+        case XCB_NOTIFY_MODE_WHILE_GRABBED: mode_str = "focus while grabbed"; break;
+        default: mode_str = "unknown focus mode " + std::to_string(event->mode); break;
+        }
+        log_debug("XCB_FOCUS_IN %s on %s", mode_str.c_str(), connection->window_debug_string(event->event).c_str());
+    }
+
+    // Ignore grabs
+    if (event->mode != XCB_NOTIFY_MODE_GRAB && event->mode != XCB_NOTIFY_MODE_UNGRAB)
+    {
+        std::lock_guard<std::mutex> lock{mutex};
+        focused_window = event->event;
+        // We might want to keep X11 focus and Mir focus in sync
+        // (either by requesting a focus change in Mir, reverting this X11 focus change or both)
+    }
+}
+
 // Cursor
 xcb_cursor_t mf::XWaylandWM::xcb_cursor_image_load_cursor(const XcursorImage *img)
 {

diff --git a/src/server/frontend_xwayland/xwayland_wm.h b/src/server/frontend_xwayland/xwayland_wm.h
@@ -65,6 +65,7 @@ class XWaylandWM
     ~XWaylandWM();
 
     auto get_wm_surface(xcb_window_t xcb_window) -> std::experimental::optional<std::shared_ptr<XWaylandSurface>>;
+    void set_focus(xcb_window_t xcb_window, bool should_be_focused);
     void run_on_wayland_thread(std::function<void()>&& work);
 
 private:
@@ -107,6 +108,7 @@ class XWaylandWM
     void handle_configure_notify(xcb_configure_notify_event_t *event);
     void handle_unmap_notify(xcb_unmap_notify_event_t *event);
     void handle_destroy_notify(xcb_destroy_notify_event_t *event);
+    void handle_focus_in(xcb_focus_in_event_t* event);
 
     std::mutex mutex;
 
@@ -122,6 +124,7 @@ class XWaylandWM
 
     xcb_window_t xcb_window;
     std::map<xcb_window_t, std::shared_ptr<XWaylandSurface>> surfaces;
+    std::experimental::optional<xcb_window_t> focused_window;
     std::shared_ptr<dispatch::ReadableFd> wm_dispatcher;
     int xcb_cursor;
     std::vector<xcb_cursor_t> xcb_cursors;