Rewrite how input focus grabbing works #38

CHANGELOG.md

@@ -10,17 +10,21 @@ Versioning](https://semver.org/spec/v2.0.0.html).
 
 ### Changed
 
+- The way keyboard input works has been completely rewritten to be more reliable
+  in certain hosts and to provide a more integrated experience. Hovering over
+  the plugin's editor while the window provided by the host is active will now
+  immediately grab keyboard focus, and yabridge will now return input focus to
+  the host's window when moving the mouse outside of the plugin's editor when
+  the window is still active. This should fix some instances where keyboard
+  input was not working in hosts with more complex editor windows like _REAPER_
+  and _Ardour_, and it will also allow things like the comment field in REAPER's
+  FX window to still function. Please let me know if this causes any issues
+  elsewhere!
 - Added a note to the message saying that libSwell GUI support has been disabled
   that his is perfectly normal when using REAPER. The message now also contains
   the suggestion to enable the `hack_reaper_update_display` workaround for
   REAPER when it is not already enabled.
 
-### Fixed
-
-- Changed the way keyboard input focus works. This fixes keyboard input
-  sometimes not working in _REAPER_ and _Ardour_. Please let me know if this
-  causes any issues elsewhere!
-
 ## [1.6.1] - 2020-09-28
 
 ### Fixed

src/wine-host/editor.cpp

@@ -25,6 +25,12 @@ constexpr size_t idle_timer_id = 1337;
  */
 constexpr uint16_t event_type_mask = ((1 << 7) - 1);
 
+/**
+ * The name of the X11 property on the root window used to denote the active
+ * window in EWMH compliant window managers.
+ */
+constexpr char active_window_property_name[] = "_NET_ACTIVE_WINDOW";
+
 /**
  * Find the topmost window (i.e. the window before the root window in the window
  * tree) starting from a certain window.
@@ -37,6 +43,19 @@ constexpr uint16_t event_type_mask = ((1 << 7) - 1);
  */
 xcb_window_t find_topmost_window(xcb_connection_t& x11_connection,
                                  xcb_window_t starting_at);
+/**
+ * Check whether `child` is a descendant of `parent` or the same window. Used
+ * during focus checks to only grab focus when needed.
+ *
+ * @param x11_connection The X11 connection to use.
+ * @param child The potential child window.
+ * @param parent The potential parent window.
+ *
+ * @return Whether `child` is a descendant of or the same window as `parent.`
+ */
+bool is_child_window_or_same(xcb_connection_t& x11_connection,
+                             xcb_window_t child,
+                             xcb_window_t parent);
 /**
  * Compute the size a window would have to be to be allowed to fullscreened on
  * any of the connected screens.
@@ -104,6 +123,22 @@ Editor::Editor(const Configuration& config,
       topmost_window(find_topmost_window(*x11_connection, parent_window)),
       // Needed to send update messages on a timer
       plugin(effect) {
+    xcb_generic_error_t* error;
+
+    // Used for input focus grabbing to only grab focus when the window is
+    // active.
+    const xcb_intern_atom_cookie_t atom_cookie = xcb_intern_atom(
+        x11_connection.get(), true, strlen(active_window_property_name),
+        active_window_property_name);
+    xcb_intern_atom_reply_t* atom_reply =
+        xcb_intern_atom_reply(x11_connection.get(), atom_cookie, &error);
+    assert(!error);
+    // TODO: Check for XCB_ATOM_NONE. If the user is using some obscure WM that
+    //       does not support _NET_WM_NAME, then we can print a warning message
+    //       and fall back to grabbing focus on `WM_PARENTNOTIFY`.
+    active_window_property = atom_reply->atom;
+    free(atom_reply);
+
     // Because we're not using XEmbed Wine will interpret any local coordinates
     // as global coordinates. To work around this we'll tell the Wine window
     // it's located at its actual coordinates on screen rather than somewhere
@@ -115,7 +150,9 @@ Editor::Editor(const Configuration& config,
     xcb_change_window_attributes(x11_connection.get(), topmost_window,
                                  XCB_CW_EVENT_MASK, &topmost_event_mask);
     xcb_flush(x11_connection.get());
-    const uint32_t parent_event_mask = XCB_EVENT_MASK_ENTER_WINDOW;
+    const uint32_t parent_event_mask = XCB_EVENT_MASK_FOCUS_CHANGE |
+                                       XCB_EVENT_MASK_ENTER_WINDOW |
+                                       XCB_EVENT_MASK_LEAVE_WINDOW;
     xcb_change_window_attributes(x11_connection.get(), parent_window,
                                  XCB_CW_EVENT_MASK, &parent_event_mask);
     xcb_flush(x11_connection.get());
@@ -211,9 +248,37 @@ void Editor::handle_x11_events() const {
             // check is sometimes necessary for using multiple editor windows
             // within a single plugin group.
             case XCB_CONFIGURE_NOTIFY:
-            case XCB_ENTER_NOTIFY:
                 fix_local_coordinates();
                 break;
+            // We want to grab keyboard input focus when the user hovers over
+            // our embedded Wine window AND that window is a child of the
+            // currently active window. This ensures that the behavior is
+            // similar to what you'd expect of a native application, without
+            // grabbing input focus when accidentally hovering over a yabridge
+            // window in the background.
+            // The `FocusIn` is needed for when returning to the main plugin
+            // window after closing a dialog, since that often won't trigger an
+            // `EnterNotify'.
+            case XCB_ENTER_NOTIFY:
+            case XCB_FOCUS_IN:
+                fix_local_coordinates();
+
+                if (is_wine_window_active()) {
+                    set_input_focus(true);
+                }
+                break;
+            // When the user moves their mouse away from the Wine window _while
+            // the window provided by the host it is contained in is still
+            // active_, we will give back keyboard focus to that window. This
+            // for instance allows you to still use the search bar in REAPER's
+            // FX window. This distinction is important, because we do not want
+            // to mess with keyboard focus when hovering over the window while
+            // for instance a dialog is open.
+            case XCB_LEAVE_NOTIFY:
+                if (is_wine_window_active()) {
+                    set_input_focus(false);
+                }
+                break;
         }
 
         free(generic_event);
@@ -239,7 +304,6 @@ void Editor::fix_local_coordinates() const {
     xcb_query_tree_reply_t* query_reply =
         xcb_query_tree_reply(x11_connection.get(), query_cookie, &error);
     assert(!error);
-
     xcb_window_t root = query_reply->root;
     free(query_reply);
 
@@ -274,29 +338,50 @@ void Editor::fix_local_coordinates() const {
     xcb_flush(x11_connection.get());
 }
 
-void Editor::grab_input_focus() const {
+void Editor::set_input_focus(bool grab) const {
-    // Explicitely request input focus when the user clicks on the window.
+    // Explicitly request input focus when the user interacts with the window.
-    // Without this the parent window will capture all keyboard events in most
+    // Without this `topmost_window` will capture all keyboard events in most
     // hosts. Ideally we would just do this whenever the child window calls
     // `SetFocus()` (or no handling should be necessary), but as far as I'm
-    // aware there is no way to do this. Right now we're doing grabbing input
+    // aware there is no way to do this. Right now we will grab input focus when
-    // focus in one of two situations:
+    // the user hovers over the Wine window while the window it is contained in
-    //
+    // (the one provided by the host) is active. Keyboard focus will be given
-    // - On `WM_PARENTNOTIFY`, which include every time the user clicks on the
+    // back to that window when the user moves their mouse outside of the Wine
-    //   Wine window. On most hosts we could get away with using the `FocusIn`
+    // window while the host's window is still active (that's an important
-    //   X11 event instead, but this does not work reliably in REAPER and
+    // detail, since plugins may have dialogs).
-    //   Ardour.
-    // - On `WM_NCACTIVATE` and `WM_CANCELMODE`. This is a small HACK to
-    //   immediatly grab keyboard focus again when the user closes a popup
-    //   window. Again, this is only needed for Ardour and REAPER, but without
-    //   this the F1 help dialog system in Melda plugins does not work like
-    //   you'd expect, instead requiring you to click on the window in between
-    //   F1 presses.
     xcb_set_input_focus(x11_connection.get(), XCB_INPUT_FOCUS_PARENT,
-                        wine_window, XCB_CURRENT_TIME);
+                        grab ? parent_window : topmost_window,
+                        XCB_CURRENT_TIME);
     xcb_flush(x11_connection.get());
 }
 
+bool Editor::is_wine_window_active() const {
+    // We will only grab focus when the Wine window is active. To do this we'll
+    // read the `_NET_ACTIVE_WINDOW` property from the root window (which can
+    // change when the window gets moved to another screen, so we won't cache
+    // this).
+    xcb_generic_error_t* error;
+    const xcb_query_tree_cookie_t root_query_cookie =
+        xcb_query_tree(x11_connection.get(), wine_window);
+    xcb_query_tree_reply_t* root_query_reply =
+        xcb_query_tree_reply(x11_connection.get(), root_query_cookie, &error);
+    assert(!error);
+    const xcb_window_t root_window = root_query_reply->root;
+    free(root_query_reply);
+
+    const xcb_get_property_cookie_t property_cookie =
+        xcb_get_property(x11_connection.get(), false, root_window,
+                         active_window_property, XCB_ATOM_WINDOW, 0, 1);
+    xcb_get_property_reply_t* property_reply =
+        xcb_get_property_reply(x11_connection.get(), property_cookie, &error);
+    assert(!error);
+    const xcb_window_t active_window =
+        *static_cast<xcb_window_t*>(xcb_get_property_value(property_reply));
+    free(property_reply);
+
+    return is_child_window_or_same(*x11_connection, wine_window, active_window);
+}
+
 LRESULT CALLBACK window_proc(HWND handle,
                              UINT message,
                              WPARAM wParam,
@@ -332,24 +417,6 @@ LRESULT CALLBACK window_proc(HWND handle,
             editor->send_idle_event();
             return 0;
         } break;
-        // See the comment in `Editor::grab_input_focus()` for more details on
-        // what's happening here
-        case WM_NCACTIVATE:
-        case WM_CANCELMODE:
-        case WM_PARENTNOTIFY: {
-            auto editor = reinterpret_cast<Editor*>(
-                GetWindowLongPtr(handle, GWLP_USERDATA));
-            if (!editor) {
-                break;
-            }
-
-            // Grab input focus when the user interacts with the Wine window.
-            // Ideally this would only be done when the window does not yet have
-            // keyboard focus but I wasn't able to find a reliable way to do
-            // that that also works in REAPER.
-            editor->fix_local_coordinates();
-            editor->grab_input_focus();
-        } break;
     }
 
     return DefWindowProc(handle, message, wParam, lParam);

src/wine-host/editor.h

@@ -144,14 +144,23 @@ class Editor {
     void fix_local_coordinates() const;
 
     /**
-     * Steal keyboard focus. This is done whenever the user clicks on the window
+     * Steal or release keyboard focus. This is done whenever the user clicks on
-     * since we don't have a way to detect whether the client window is calling
+     * the window since we don't have a way to detect whether the client window
-     * `SetFocus()`. See the comment inside of this function for more details on
+     * is calling `SetFocus()`. See the comment inside of this function for more
-     * when this is used.
+     * details on when this is used.
+     *
+     * @param grab Whether to grab input focus (if `true`) or to give back input
+     *   focus to `topmost_window` (if `false`).
      */
-    void grab_input_focus() const;
+    void set_input_focus(bool grab) const;
 
    private:
+    /**
+     * Returns `true` if the currently active window (as per
+     * `_NET_ACTIVE_WINDOW`) contains `wine_window`.
+     */
+    bool is_wine_window_active() const;
+
     /**
      * A pointer to the currently active window. Will be a null pointer if no
      * window is active.
@@ -193,7 +202,6 @@ class Editor {
         std::unique_ptr<std::remove_pointer_t<HWND>, decltype(&DestroyWindow)>>
         win32_child_handle;
 
-   private:
     /**
      * The Win32 API will block the `DispatchMessage` call when opening e.g. a
      * dropdown, but it will still allow timers to be run so the GUI can still
@@ -225,4 +233,9 @@ class Editor {
      *Needed to handle idle updates through a timer
      */
     AEffect* plugin;
+
+    /**
+     * The atom corresponding to `_NET_ACTIVE_WINDOW`.
+     */
+    xcb_atom_t active_window_property;
 };