Merge #1297 #1298

1297: More cleanup r=AlanGriffiths a=wmww Lots of little things that don't fit anywhere else 1298: Use focused window as default popup parent r=AlanGriffiths a=wmww When `TRANSIENT_FOR` is set to a window without a scene surface, use the currently focused window as the fallback popup parent. This happens, for example, when opening Kate's `Bookmarks` menu. This PR works somewhat on its own, and better in conjunction with #1296 Co-authored-by: William Wold <[email protected]>
canonical · Feb 21, 2020 · eea9c3b · eea9c3b
1 parent 177cd6f
commit eea9c3b
Show file tree

Hide file tree

Showing 6 changed files with 131 additions and 26 deletions.
diff --git a/src/server/frontend_xwayland/xcb_connection.cpp b/src/server/frontend_xwayland/xcb_connection.cpp
@@ -187,9 +187,14 @@ auto mf::XCBConnection::string_from(xcb_get_property_reply_t const* reply) const
             "Supplied reply is of type " + query_name(reply->type) + " and does not hold string data"));
     }
 
-    return std::string{
+    auto const result = std::string{
         static_cast<const char *>(xcb_get_property_value(reply)),
         static_cast<unsigned long>(xcb_get_property_value_length(reply))};
+    auto const end = result.find('\0');
+    if (end == std::string::npos)
+        return result;
+    else
+        return result.substr(0, end);
 }
 
 bool mf::XCBConnection::is_ours(uint32_t id) const

diff --git a/src/server/frontend_xwayland/xcb_connection.h b/src/server/frontend_xwayland/xcb_connection.h
@@ -96,8 +96,7 @@ class XCBConnection
     auto screen() const -> xcb_screen_t*;
     auto root_window() const -> xcb_window_t;
 
-
-    /// Does a round-trip to the X server and does not cache. Should be improved if needed on normal code paths
+    /// Looks up an atom's name, or requests it from the X server if it is not already cached
     auto query_name(xcb_atom_t atom) const -> std::string;
     auto reply_contains_string_data(xcb_get_property_reply_t const* reply) const -> bool;
     auto string_from(xcb_get_property_reply_t const* reply) const -> std::string;

diff --git a/src/server/frontend_xwayland/xwayland_log.h b/src/server/frontend_xwayland/xwayland_log.h
@@ -25,7 +25,8 @@ namespace mir
 {
 inline auto verbose_xwayland_logging_enabled() -> bool
 {
-    return getenv("MIR_X11_VERBOSE_LOG");
+    static bool const log_verbose = getenv("MIR_X11_VERBOSE_LOG");
+    return log_verbose;
 }
 } /* mir */
 

diff --git a/src/server/frontend_xwayland/xwayland_surface.cpp b/src/server/frontend_xwayland/xwayland_surface.cpp
@@ -290,25 +290,32 @@ void mf::XWaylandSurface::close()
 
 void mf::XWaylandSurface::take_focus()
 {
+    bool supports_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
+        supports_take_focus = (
+            cached.supported_wm_protocols.find(connection->wm_take_focus) !=
+            cached.supported_wm_protocols.end());
+    }
 
-    uint32_t const client_message_data[]{
-        connection->wm_take_focus,
-        XCB_TIME_CURRENT_TIME};
+    if (supports_take_focus)
+    {
+        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);
+        connection->send_client_message<XCBType::WM_PROTOCOLS>(
+            window,
+            XCB_EVENT_MASK_SUBSTRUCTURE_REDIRECT,
+            client_message_data);
+    }
 
+    // TODO: only send if allowed based on wm hints input mode
+    // see https://tronche.com/gui/x/icccm/sec-4.html#s-4.1.7
     xcb_set_input_focus(
         *connection,
         XCB_INPUT_FOCUS_POINTER_ROOT,
@@ -714,6 +721,12 @@ void mf::XWaylandSurface::scene_surface_close_requested()
 
     if (delete_window)
     {
+        if (verbose_xwayland_logging_enabled())
+        {
+            log_debug(
+                "Sending WM_DELETE_WINDOW request to %s",
+                connection->window_debug_string(window).c_str());
+        }
         uint32_t const client_message_data[]{
             connection->wm_delete_window,
             XCB_TIME_CURRENT_TIME,
@@ -722,6 +735,12 @@ void mf::XWaylandSurface::scene_surface_close_requested()
     }
     else
     {
+        if (verbose_xwayland_logging_enabled())
+        {
+            log_debug(
+                "Killing %s because it does not support WM_DELETE_WINDOW",
+                connection->window_debug_string(window).c_str());
+        }
         xcb_kill_client(*connection, window);
     }
     connection->flush();
@@ -770,14 +789,87 @@ void mf::XWaylandSurface::is_transient_for(xcb_window_t transient_for)
 {
     std::shared_ptr<scene::Surface> parent_scene_surface; // May remain nullptr
 
+    // returns nullptr on error
+    auto const get_scene_surface_from = [this](xcb_window_t xcb_window) -> std::shared_ptr<scene::Surface>
+        {
+            if (auto const xwayland_surface = this->xwm->get_wm_surface(xcb_window))
+            {
+                std::lock_guard<std::mutex> lock{xwayland_surface.value()->mutex};
+                auto const scene_surface = xwayland_surface.value()->weak_scene_surface.lock();
+                if (verbose_xwayland_logging_enabled())
+                {
+                    if (scene_surface)
+                    {
+                        log_debug(
+                            "%s set as transient for %s",
+                            connection->window_debug_string(window).c_str(),
+                            connection->window_debug_string(xcb_window).c_str());
+                    }
+                    else
+                    {
+                        log_debug(
+                            "%s can not be transient for %s as the latter does not have a scene surface",
+                            connection->window_debug_string(window).c_str(),
+                            connection->window_debug_string(xcb_window).c_str());
+                    }
+                }
+                return scene_surface;
+            }
+            else
+            {
+                if (verbose_xwayland_logging_enabled())
+                {
+                    log_debug(
+                        "%s can not be transient for %s as the latter does not have an XWayland surface",
+                        connection->window_debug_string(window).c_str(),
+                        connection->window_debug_string(xcb_window).c_str());
+                }
+                return nullptr;
+            }
+        };
+
     if (transient_for != XCB_WINDOW_NONE)
     {
-        if (auto const parent_surface = this->xwm->get_wm_surface(transient_for))
+        parent_scene_surface = get_scene_surface_from(transient_for);
+
+        if (!parent_scene_surface)
+        {
+            auto const focused_window = xwm->get_focused_window();
+            if (focused_window)
+            {
+                if (verbose_xwayland_logging_enabled())
+                {
+                    log_debug(
+                        "Falling back to the currently focused window (%s)",
+                        connection->window_debug_string(focused_window.value()).c_str());
+                }
+                parent_scene_surface = get_scene_surface_from(focused_window.value());
+            }
+            else
+            {
+                if (verbose_xwayland_logging_enabled())
+                {
+                    log_debug(
+                        "There is no focused window",
+                        connection->window_debug_string(window).c_str(),
+                        connection->window_debug_string(transient_for).c_str());
+                }
+            }
+        }
+
+        if (!parent_scene_surface && verbose_xwayland_logging_enabled())
         {
-            std::lock_guard<std::mutex> parent_lock{parent_surface.value()->mutex};
-            parent_scene_surface = parent_surface.value()->weak_scene_surface.lock();
+            log_debug(
+                "Failed to find a window for %s to be transient for",
+                connection->window_debug_string(window).c_str());
         }
     }
+    else if (verbose_xwayland_logging_enabled())
+    {
+        log_debug(
+            "%s is not transient",
+            connection->window_debug_string(window).c_str());
+    }
 
     {
         std::lock_guard<std::mutex> lock{this->mutex};

diff --git a/src/server/frontend_xwayland/xwayland_wm.cpp b/src/server/frontend_xwayland/xwayland_wm.cpp
@@ -241,30 +241,30 @@ void mf::XWaylandWM::create_wm_window()
 {
     std::string const wm_name{"Mir XWM"};
 
-    xcb_window = xcb_generate_id(*connection);
+    wm_window = xcb_generate_id(*connection);
     xcb_create_window(
         *connection,
         XCB_COPY_FROM_PARENT,
-        xcb_window,
+        wm_window,
         connection->root_window(),
         0, 0, 10, 10, 0,
         XCB_WINDOW_CLASS_INPUT_OUTPUT,
         connection->screen()->root_visual,
         0, NULL);
 
-    connection->set_property<XCBType::WINDOW>(xcb_window, connection->net_supporting_wm_check, xcb_window);
-    connection->set_property<XCBType::UTF8_STRING>(xcb_window, connection->net_wm_name, wm_name);
+    connection->set_property<XCBType::WINDOW>(wm_window, connection->net_supporting_wm_check, wm_window);
+    connection->set_property<XCBType::UTF8_STRING>(wm_window, connection->net_wm_name, wm_name);
 
     connection->set_property<XCBType::WINDOW>(
         connection->root_window(),
         connection->net_supporting_wm_check,
-        xcb_window);
+        wm_window);
 
     /* Claim the WM_S0 selection even though we don't support
      * the --replace functionality. */
-    xcb_set_selection_owner(*connection, xcb_window, connection->wm_s0, XCB_TIME_CURRENT_TIME);
+    xcb_set_selection_owner(*connection, wm_window, connection->wm_s0, XCB_TIME_CURRENT_TIME);
 
-    xcb_set_selection_owner(*connection, xcb_window, connection->net_wm_cm_s0, XCB_TIME_CURRENT_TIME);
+    xcb_set_selection_owner(*connection, wm_window, connection->net_wm_cm_s0, XCB_TIME_CURRENT_TIME);
 }
 
 auto mf::XWaylandWM::get_wm_surface(
@@ -279,6 +279,12 @@ auto mf::XWaylandWM::get_wm_surface(
         return surface->second;
 }
 
+auto mf::XWaylandWM::get_focused_window() -> std::experimental::optional<xcb_window_t>
+{
+    std::lock_guard<std::mutex> lock{mutex};
+    return focused_window;
+}
+
 void mf::XWaylandWM::set_focus(xcb_window_t xcb_window, bool should_be_focused)
 {
     {
@@ -308,7 +314,7 @@ void mf::XWaylandWM::set_focus(xcb_window_t xcb_window, bool should_be_focused)
         connection->set_property<XCBType::WINDOW>(
             connection->root_window(),
             connection->net_active_window,
-            static_cast<xcb_window_t>(xcb_window));
+            xcb_window);
 
         if (auto const surface = get_wm_surface(xcb_window))
         {
@@ -654,6 +660,7 @@ void mf::XWaylandWM::handle_client_message(xcb_client_message_event_t *event)
 
     if (auto const surface = get_wm_surface(event->window))
     {
+        // TODO: net_active_window?
         if (event->type == connection->net_wm_moveresize)
             handle_move_resize(surface.value(), event);
         else if (event->type == connection->net_wm_state)

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>>;
+    auto get_focused_window() -> std::experimental::optional<xcb_window_t>;
     void set_focus(xcb_window_t xcb_window, bool should_be_focused);
     void run_on_wayland_thread(std::function<void()>&& work);
 
@@ -122,7 +123,7 @@ class XWaylandWM
     wl_client* const wayland_client;
     std::shared_ptr<XWaylandWMShell> const wm_shell;
 
-    xcb_window_t xcb_window;
+    xcb_window_t wm_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;