Fix focus handling when reopening REAPER FX window

REAPER initializes the plugin's editor first before reparenting the
parent window to the FX window, so our `topmost_window` didn't actually
refer to the FX window.

CHANGELOG.md

@@ -70,6 +70,16 @@ Versioning](https://semver.org/spec/v2.0.0.html).
   plugins would at runtime change their query interface to support more VST3
   interfaces, including the mandatory edit controller interface. Yabridge now
   requeries the supported interfaces at a later stage to work around this.
+- Fixed VST2 plugins in **Ardour** not receiving all transport information,
+  breaking host sync and LFOs in certain plugins. This was a regression from
+  yabridge 3.2.0.
+- Fixed input focus handling being broken **REAPER** after reopning a closed FX
+  window. Now moving the mouse cursor outside of the plugin's GUI will always
+  release input focus, even after closing the window.
+- Fixed _Insert Piz Here_'s _midiLooper_ crashing in **REAPER** when the plugin
+  tries to use REAPER's [host function
+  API](https://www.reaper.fm/sdk/vst/vst_ext.php#vst_host). This currently isn't
+  supported by yabridge. We now explicitly ignore these requests.
 - Fixed JUCE VST3 plugins like Tokyo Dawn Records' _SlickEQ M_ causing the host
   to freeze when they send a parameter change from the audio thread using the
   wrong VST3 API while the plugin is also trying to resize the window from the
@@ -77,13 +87,6 @@ Versioning](https://semver.org/spec/v2.0.0.html).
   the Smart Ops panel after having used it once. To fix this, yabridge's
   Wine-side VST3 mutual recursion mechanism now only operates when invoked from
   the GUI thread.
-- Fixed VST2 plugins in **Ardour** not receiving all transport information,
-  breaking host sync and LFOs in certain plugins. This was a regression from
-  yabridge 3.2.0.
-- Fixed _Insert Piz Here_'s _midiLooper_ crashing in **REAPER** when the plugin
-  tries to use REAPER's [host function
-  API](https://www.reaper.fm/sdk/vst/vst_ext.php#vst_host). This currently isn't
-  supported by yabridge. We now explicitly ignore these requests.
 - Fixed yabridge's logging seeking the STDERR stream to position 0 every time it
   writes a log message. This would be noticeable when piping the host's STDERR
   stream to a file and `YABRIDGE_DEBUG_LEVEL` wasn't set.

src/wine-host/bridges/vst3.cpp

@@ -1171,7 +1171,7 @@ void Vst3Bridge::run() {
 void Vst3Bridge::handle_x11_events() noexcept {
     std::lock_guard lock(object_instances_mutex);
 
-    for (const auto& [instance_id, object] : object_instances) {
+    for (auto& [instance_id, object] : object_instances) {
         if (object.editor) {
             object.editor->handle_x11_events();
         }

src/wine-host/editor.cpp

@@ -48,6 +48,21 @@ using namespace std::literals::chrono_literals;
  */
 constexpr size_t idle_timer_id = 1337;
 
+/**
+ * The X11 event mask for the topmost window, which in most DAWs except for
+ * Ardour and REAPER will be the same as `parent_window`.
+ */
+constexpr uint32_t topmost_event_mask =
+    XCB_EVENT_MASK_STRUCTURE_NOTIFY | XCB_EVENT_MASK_VISIBILITY_CHANGE;
+
+/**
+ * The X11 event mask for the parent window. We need this structure notify here
+ * as well to detect reparents.
+ */
+constexpr uint32_t parent_event_mask =
+    topmost_event_mask | XCB_EVENT_MASK_FOCUS_CHANGE |
+    XCB_EVENT_MASK_ENTER_WINDOW | XCB_EVENT_MASK_LEAVE_WINDOW;
+
 /**
  * The name of the X11 property on the root window used to denote the active
  * window in EWMH compliant window managers.
@@ -278,13 +293,8 @@ Editor::Editor(MainContext& main_context,
     // release input focus as necessary.
     // If we do enable XEmbed support, we'll also listen for visibility changes
     // and trigger the embedding when the window becomes visible
-    const uint32_t topmost_event_mask =
-        XCB_EVENT_MASK_STRUCTURE_NOTIFY | XCB_EVENT_MASK_VISIBILITY_CHANGE;
     xcb_change_window_attributes(x11_connection.get(), topmost_window,
                                  XCB_CW_EVENT_MASK, &topmost_event_mask);
-    const uint32_t parent_event_mask =
-        XCB_EVENT_MASK_FOCUS_CHANGE | XCB_EVENT_MASK_ENTER_WINDOW |
-        XCB_EVENT_MASK_LEAVE_WINDOW | XCB_EVENT_MASK_VISIBILITY_CHANGE;
     xcb_change_window_attributes(x11_connection.get(), parent_window,
                                  XCB_CW_EVENT_MASK, &parent_event_mask);
     xcb_flush(x11_connection.get());
@@ -334,7 +344,7 @@ Editor::Editor(MainContext& main_context,
     }
 }
 
-void Editor::handle_x11_events() const noexcept {
+void Editor::handle_x11_events() noexcept {
     // NOTE: Ardour will unmap the window instead of closing the editor. When
     //       the window is unmapped `wine_window` doesn't exist and any X11
     //       function calls involving it will fail. All functions called from
@@ -346,6 +356,17 @@ void Editor::handle_x11_events() const noexcept {
             const uint8_t event_type =
                 generic_event->response_type & xcb_event_type_mask;
             switch (event_type) {
+                // NOTE: When reopening a closed editor window in REAPER, REAPER
+                //       will initialize the editor first, and only then will it
+                //       reparent `parent_window` to a new FX window. This means
+                //       that `topmost_window` will be the same as
+                //       `parent_window` in REAPER if you reopen a plugin GUI,
+                //       which breaks our input focus handling. To work around
+                //       this, we will just check if the topmost window has
+                //       changed whenever the parent window gets reparented.
+                case XCB_REPARENT_NOTIFY: {
+                    redetect_topmost_window();
+                } break;
                 // We're listening for `ConfigureNotify` events on the topmost
                 // window before the root window, i.e. the window that's
                 // actually going to get dragged around the by the user. In most
@@ -354,19 +375,19 @@ void Editor::handle_x11_events() const noexcept {
                 // window, the local coordinates should be updated. The
                 // additional `EnterWindow` check is sometimes necessary for
                 // using multiple editor windows within a single plugin group.
-                case XCB_CONFIGURE_NOTIFY:
+                case XCB_CONFIGURE_NOTIFY: {
                     if (!use_xembed) {
                         fix_local_coordinates();
                     }
-                    break;
+                } break;
                 // Start the XEmbed procedure when the window becomes visible,
                 // since most hosts will only show the window after the plugin
                 // has embedded itself into it.
-                case XCB_VISIBILITY_NOTIFY:
+                case XCB_VISIBILITY_NOTIFY: {
                     if (use_xembed) {
                         do_xembed();
                     }
-                    break;
+                } 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
@@ -376,7 +397,7 @@ void Editor::handle_x11_events() const noexcept {
                 // 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:
+                case XCB_FOCUS_IN: {
                     if (!use_xembed) {
                         fix_local_coordinates();
                     }
@@ -389,7 +410,7 @@ void Editor::handle_x11_events() const noexcept {
                         is_wine_window_active()) {
                         set_input_focus(true);
                     }
-                    break;
+                } 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
@@ -566,6 +587,35 @@ bool Editor::is_wine_window_active() const {
     return is_child_window_or_same(*x11_connection, wine_window, active_window);
 }
 
+void Editor::redetect_topmost_window() noexcept {
+    const xcb_window_t new_topmost_window =
+        find_ancestor_windows(*x11_connection, parent_window).back();
+    if (new_topmost_window == topmost_window) {
+        return;
+    }
+
+    // We need to readjust the event masks for the new topmost window, keeping
+    // the (very probable) possibility in mind that the old topmost window is
+    // the same as the parent window or that the parent window now is the
+    // topmost window.
+    if (topmost_window != parent_window) {
+        constexpr uint32_t no_event_mask = XCB_EVENT_MASK_NO_EVENT;
+        xcb_change_window_attributes(x11_connection.get(), topmost_window,
+                                     XCB_CW_EVENT_MASK, &no_event_mask);
+    }
+
+    if (new_topmost_window == parent_window) {
+        xcb_change_window_attributes(x11_connection.get(), new_topmost_window,
+                                     XCB_CW_EVENT_MASK, &parent_event_mask);
+    } else {
+        xcb_change_window_attributes(x11_connection.get(), new_topmost_window,
+                                     XCB_CW_EVENT_MASK, &topmost_event_mask);
+    }
+
+    topmost_window = new_topmost_window;
+    xcb_flush(x11_connection.get());
+}
+
 bool Editor::supports_ewmh_active_window() const {
     if (supports_ewmh_active_window_cache) {
         return *supports_ewmh_active_window_cache;

src/wine-host/editor.h

@@ -163,7 +163,7 @@ class Editor {
     /**
      * Handle X11 events sent to the window our editor is embedded in.
      */
-    void handle_x11_events() const noexcept;
+    void handle_x11_events() noexcept;
 
     /**
      * Get the Win32 window handle so it can be passed to an `effEditOpen()`
@@ -225,6 +225,12 @@ class Editor {
      */
     bool is_wine_window_active() const;
 
+    /**
+     * After `parent_window` gets reparented, we may need to redetect the
+     * topmost window and adjust the events we're subscribed to accordingly.
+     */
+    void redetect_topmost_window() noexcept;
+
     /**
      * Send an XEmbed message to a window. This does not include a flush. See
      * the spec for more information:
@@ -313,8 +319,14 @@ class Editor {
      * REAPER) embed `parent_window` into another window. We have to listen for
      * configuration changes on this topmost window to know when the window is
      * being dragged around.
+     *
+     * NOTE: When reopening a REAPER FX window that has previously been closed,
+     *       REAPER will initialize the first plugin's editor first before
+     *       opening the window. This means that the topmost FX window doesn't
+     *       actually exist yet at that point, so we need to redetect this
+     *       later.
      */
-    const xcb_window_t topmost_window;
+    xcb_window_t topmost_window;
 
     /**
      * The atom corresponding to `_NET_ACTIVE_WINDOW`.