Completely run effEditIdle from a timer

Although it hasn't shown up, this will get rid of the possibility of
off-thread effEditIdle calls causing issues. And since we need some way
to run call this function while the event loop is running anyways, doing
it entirely from a timer similar to how hosts on Windows would do it
seems like the best solution.

CHANGELOG.md

@@ -44,9 +44,7 @@ TODO: Add an updated screenshot with some fancy VST3-only plugins to the readme
   Most hosts already do something similar themselves, so this may not make any
   difference in responsiveness.
 - VST2 editor idle events are now handled slightly differently. This should
-  result in even more responsive GUIs and I have not come across any plugins
+  result in even more responsive GUIs for VST2 plugins.
-  where this change introduced issues, but please let me know if it does break
-  anything for you.
 - Changed part of the build process considering [this Wine
   bug](https://bugs.winehq.org/show_bug.cgi?id=49138). Building with Wine 5.7
   and 5.8 required a change, but that change now breaks builds using Wine 6.0

src/plugin/bridges/vst2.cpp

@@ -398,6 +398,20 @@ intptr_t Vst2PluginBridge::dispatch(AEffect* /*plugin*/,
 
             return return_value;
         }; break;
+        case effEditIdle: {
+            // This is the only place where we'll deviate from yabridge's
+            // 'one-to-one passthrough' philosophy. While in practice we can
+            // just pass through `effEditIdle` and we have been doing so until
+            // yabridge 3.x, in reality it's much more practical to just run
+            // this on a Win32 timer. We would either need to run `effEditIdle`
+            // from a non-GUI thread (which could cause issues), or we would
+            // need a timer anyways to proc the function when the GUI is being
+            // blocked by for instance an open dropdown.
+            logger.log_event(true, opcode, index, value, nullptr, option,
+                             std::nullopt);
+            logger.log_event_response(true, opcode, 0, nullptr, std::nullopt);
+            return 0;
+        }; break;
         case effCanDo: {
             const std::string query(static_cast<const char*>(data));
 

src/wine-host/bridges/vst2.cpp

@@ -40,12 +40,10 @@ std::mutex current_bridge_instance_mutex;
 /**
  * Opcodes that should always be handled on the main thread because they may
  * involve GUI operations.
- *
- * XXX: We removed effEditIdle from here and everything still seems to work
- *      fine. Verify that this didn't break any plugins.
  */
 const std::set<int> unsafe_opcodes{effOpen,     effClose,     effEditGetRect,
-                                   effEditOpen, effEditClose, effEditTop};
+                                   effEditOpen, effEditClose, effEditIdle,
+                                   effEditTop};
 
 intptr_t VST_CALL_CONV
 host_callback_proxy(AEffect*, int, int, intptr_t, void*, float);
@@ -313,6 +311,11 @@ void Vst2Bridge::run() {
                         //       https://github.com/Ardour/ardour/commit/f7cb1b0b481eeda755bdf8eb9fc5f90a81d2aa01.
                         //       We should keep this in until Ardour 6.3 is no
                         //       longer in distro repositories.
+                        //
+                        //       Note that now that we run `effEditIdle`
+                        //       entirely off of a Win32 timer this will never
+                        //       get hit, but we'll keep it in for the sake of
+                        //       preserving correct behaviour.
                         if (opcode == effEditIdle && !editor) {
                             std::cerr << "WARNING: The host is calling "
                                          "`effEditIdle()` while the "
@@ -379,7 +382,10 @@ intptr_t Vst2Bridge::dispatch_wrapper(AEffect* plugin,
             // provided by the host, and let the plugin embed itself into
             // the Wine window
             const auto x11_handle = reinterpret_cast<size_t>(data);
-            Editor& editor_instance = editor.emplace(config, x11_handle);
+            Editor& editor_instance =
+                editor.emplace(config, x11_handle, [plugin = this->plugin]() {
+                    plugin->dispatcher(plugin, effEditIdle, 0, 0, nullptr, 0.0);
+                });
 
             return plugin->dispatcher(plugin, opcode, index, value,
                                       editor_instance.get_win32_handle(),

src/wine-host/editor.cpp

@@ -18,6 +18,15 @@
 
 #include <iostream>
 
+/**
+ * The Win32 timer ID we'll use to periodically call the VST2 `effeditidle`
+ * function with. We have to do this on a timer because the function has to be
+ * called from the GUI thread, and it should also be called while the Win32
+ * event loop is being blocked (for instance when a plugin opens a dropdown
+ * menu).
+ */
+constexpr size_t idle_timer_id = 1337;
+
 /**
  * The most significant bit in an X11 event's response type is used to indicate
  * the event source.
@@ -100,7 +109,9 @@ WindowClass::~WindowClass() {
     UnregisterClass(reinterpret_cast<LPCSTR>(atom), GetModuleHandle(nullptr));
 }
 
-Editor::Editor(const Configuration& config, const size_t parent_window_handle)
+Editor::Editor(const Configuration& config,
+               const size_t parent_window_handle,
+               std::optional<fu2::unique_function<void()>> timer_proc)
     : use_xembed(config.editor_xembed),
       x11_connection(xcb_connect(nullptr, nullptr), xcb_disconnect),
       client_area(get_maximum_screen_dimensions(*x11_connection)),
@@ -128,6 +139,16 @@ Editor::Editor(const Configuration& config, const size_t parent_window_handle)
       // window in `win32_child_handle`. If we do this before calling
       // `ShowWindow()` on `win32_handle` we'll run into X11 errors.
       win32_child_handle(std::nullopt),
+      idle_timer(
+          timer_proc
+              ? Win32Timer(
+                    win32_handle.get(),
+                    idle_timer_id,
+                    std::chrono::duration_cast<std::chrono::milliseconds>(
+                        event_loop_interval)
+                        .count())
+              : Win32Timer()),
+      idle_timer_proc(std::move(timer_proc)),
       parent_window(parent_window_handle),
       wine_window(get_x11_handle(win32_handle.get())),
       topmost_window(find_topmost_window(*x11_connection, parent_window)) {
@@ -423,6 +444,12 @@ void Editor::set_input_focus(bool grab) const {
     xcb_flush(x11_connection.get());
 }
 
+void Editor::maybe_run_timer_proc() {
+    if (idle_timer_proc) {
+        (*idle_timer_proc)();
+    }
+}
+
 void Editor::destroy_window_async(HWND window_handle) {
     PostMessage(window_handle, WM_CLOSE, 0, 0);
 }
@@ -566,6 +593,20 @@ LRESULT CALLBACK window_proc(HWND handle,
             WINDOWPOS* info = reinterpret_cast<WINDOWPOS*>(lParam);
             info->flags |= SWP_NOCOPYBITS | SWP_DEFERERASE;
         } break;
+        case WM_TIMER: {
+            auto editor = reinterpret_cast<Editor*>(
+                GetWindowLongPtr(handle, GWLP_USERDATA));
+            if (!editor || wParam != idle_timer_id) {
+                break;
+            }
+
+            // We'll send idle messages on a timer for VST2 plugins. This way
+            // the plugin will get keep periodically updating its editor either
+            // when the host sends `effEditIdle` themself, or periodically when
+            // the GUI is being blocked by a dropdown or a message box.
+            editor->maybe_run_timer_proc();
+            return 0;
+        } break;
         // In case the WM does not support the EWMH active window property,
         // we'll fall back to grabbing focus when the user clicks on the window
         // by listening to the generated `WM_PARENTNOTIFY` messages. Otherwise

src/wine-host/editor.h

@@ -25,6 +25,7 @@
 #define WINE_NOWINSOCK
 #endif
 #include <windows.h>
+#include <function2/function2.hpp>
 
 // Use the native version of xcb
 #pragma push_macro("_WIN32")
@@ -99,10 +100,16 @@ class Editor {
      *   editor behaviours.
      * @param parent_window_handle The X11 window handle passed by the VST host
      *   for the editor to embed itself into.
+     * @param timer_proc A function to run on a timer. This is used for VST2
+     *   plugins to periodically call `effEditIdle` from the message loop
+     *   thread, even when the GUI is blocked.
      *
      * @see win32_handle
      */
-    Editor(const Configuration& config, const size_t parent_window_handle);
+    Editor(
+        const Configuration& config,
+        const size_t parent_window_handle,
+        std::optional<fu2::unique_function<void()>> timer_proc = std::nullopt);
 
     ~Editor();
 
@@ -147,6 +154,14 @@ class Editor {
      */
     void set_input_focus(bool grab) const;
 
+    /**
+     * Run the timer proc function passed to the constructor, if one was passed.
+     *
+     * @see idle_timer
+     * @see idle_timer_proc
+     */
+    void maybe_run_timer_proc();
+
     /**
      * Whether to use XEmbed instead of yabridge's normal window embedded. Wine
      * with XEmbed tends to cause rendering issues, so it's disabled by default.
@@ -236,6 +251,23 @@ class Editor {
 
 #pragma GCC diagnostic pop
 
+    /**
+     * A timer we'll use to periodically run `idle_timer_proc`, if set. Thisi is
+     * only needed for VST2 plugins, as they expected the host to periodically
+     * send an idle event. We used to just pass through the calls from the host
+     * before yabridge 3.x, but doing it ourselves here makes things m much more
+     * manageable and we'd still need a timer anyways for when the GUI is
+     * blocked.
+     */
+    Win32Timer idle_timer;
+
+    /**
+     * A function to call when the Win32 timer procs. This is used to
+     * periodically call `effEditIdle()` for VST2 plugins even if the GUI is
+     * being blocked.
+     */
+    std::optional<fu2::unique_function<void()>> idle_timer_proc;
+
     /**
      * The window handle of the editor window created by the DAW.
      */

src/wine-host/utils.cpp

@@ -50,6 +50,8 @@ Win32Thread::~Win32Thread() {
 
 Win32Thread::Win32Thread() : handle(nullptr, nullptr) {}
 
+Win32Timer::Win32Timer() {}
+
 Win32Timer::Win32Timer(HWND window_handle,
                        size_t timer_id,
                        unsigned int interval_ms)
@@ -63,13 +65,12 @@ Win32Timer::~Win32Timer() {
     }
 }
 
-Win32Timer::Win32Timer(Win32Timer&& o) : timer_id(o.timer_id) {
+Win32Timer::Win32Timer(Win32Timer&& o)
-    o.timer_id = std::nullopt;
+    : window_handle(o.window_handle), timer_id(std::move(o.timer_id)) {}
-}
 
 Win32Timer& Win32Timer::operator=(Win32Timer&& o) {
-    timer_id = o.timer_id;
+    window_handle = o.window_handle;
-    o.timer_id = std::nullopt;
+    timer_id = std::move(o.timer_id);
 
     return *this;
 }

src/wine-host/utils.h

@@ -226,7 +226,9 @@ class Win32Thread {
  */
 class Win32Timer {
    public:
+    Win32Timer();
     Win32Timer(HWND window_handle, size_t timer_id, unsigned int interval_ms);
+
     ~Win32Timer();
 
     Win32Timer(const Win32Timer&) = delete;