Simplify the opening editor behaviour again

This partially reverts 16fce5577d.

src/wine-host/bridges/vst2.cpp

@@ -146,7 +146,7 @@ Vst2Bridge::Vst2Bridge(std::string plugin_dll_path,
 }
 
 bool Vst2Bridge::should_skip_message_loop() {
-    return editor_is_opening;
+    return std::holds_alternative<EditorOpening>(editor);
 }
 
 void Vst2Bridge::handle_dispatch_single() {
@@ -362,27 +362,18 @@ intptr_t Vst2Bridge::dispatch_wrapper(AEffect* plugin,
         case effEditGetRect: {
             // Some plugins will have a race condition if the message loops gets
             // handled between the call to `effEditGetRect()` and
-            // `effEditOpen()`, since this won't ever happen on Windows and
+            // `effEditOpen()`, although this behavior never appears on Windows
-            // plugins thus assume that this can't happen at all. If
+            // as hosts will always either call these functions in sequence or
-            // `effEditOpen()` has not yet been called, then we'll mark the
+            // in reverse. We need to temporarily stop handling messages when
-            // editor as currently opening to prevent the message loop from
+            // this happens.
-            // running.
+            if (!std::holds_alternative<Editor>(editor)) {
-            editor_is_opening = !editor.has_value();
+                editor = EditorOpening{};
+            }
 
             return plugin->dispatcher(plugin, opcode, index, value, data,
                                       option);
         } break;
         case effEditOpen: {
-            // As explained above, if `effEditGetRect()` was called first then
-            // after this the editor has finally been opened. Otherwise if this
-            // function was first then we'll say that the editor is in the
-            // process of being opened as the host will call `effEditGetRect()`
-            // next.
-            // TODO: Without plugin groups only the `effEditGetRect()` ->
-            //       `effEditOpen()`, is skipping the message loop in between
-            //       `effEditOpen()` and `effEditGetRect()` really needed?
-            editor_is_opening = !editor_is_opening;
-
             // Create a Win32 window through Wine, embed it into the window
             // provided by the host, and let the plugin embed itself into
             // the Wine window
@@ -392,17 +383,19 @@ intptr_t Vst2Bridge::dispatch_wrapper(AEffect* plugin,
             // should get a unique window class
             const std::string window_class =
                 "yabridge plugin " + socket_endpoint.path();
+            Editor& editor_instance =
+                editor.emplace<Editor>(window_class, plugin, x11_handle);
 
-            editor.emplace(window_class, plugin, x11_handle);
             return plugin->dispatcher(plugin, opcode, index, value,
-                                      editor->win32_handle.get(), option);
+                                      editor_instance.win32_handle.get(),
+                                      option);
         } break;
         case effEditClose: {
             const intptr_t return_value =
                 plugin->dispatcher(plugin, opcode, index, value, data, option);
 
             // Cleanup is handled through RAII
-            editor.reset();
+            editor = std::monostate();
 
             return return_value;
         } break;
@@ -414,28 +407,28 @@ intptr_t Vst2Bridge::dispatch_wrapper(AEffect* plugin,
 }
 
 void Vst2Bridge::handle_win32_events() {
-    // Don't run them message loop during the two step process of opening the
+    std::visit(
-    // plugin editor since some plugins don't expect this
+        overload{[](Editor& editor) { editor.handle_win32_events(); },
-    if (should_skip_message_loop()) {
+                 [](std::monostate&) {
-        return;
-    }
-
-    if (editor.has_value()) {
-        editor->handle_win32_events();
-    } else {
                      MSG msg;
 
                      while (PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE)) {
                          TranslateMessage(&msg);
                          DispatchMessage(&msg);
                      }
-    }
+                 },
+                 [](EditorOpening&) {
+                     // Don't handle any events in this
+                     // particular case as explained in
+                     // `Vst2Bridge::editor`
+                 }},
+        editor);
 }
 
 void Vst2Bridge::handle_x11_events() {
-    if (editor.has_value()) {
+    std::visit(overload{[](Editor& editor) { editor.handle_x11_events(); },
-        editor->handle_x11_events();
+                        [](auto&) {}},
-    }
+               editor);
 }
 
 class HostCallbackDataConverter : DefaultDataConverter {
@@ -453,17 +446,18 @@ class HostCallbackDataConverter : DefaultDataConverter {
                 return WantsVstTimeInfo{};
                 break;
             case audioMasterIOChanged:
-                // This is a helpful event that indicates that the VST plugin's
+                // This is a helpful event that indicates that the VST
-                // `AEffect` struct has changed. Writing these results back is
+                // plugin's `AEffect` struct has changed. Writing these
-                // done inside of `passthrough_event`.
+                // results back is done inside of `passthrough_event`.
                 return AEffect(*plugin);
                 break;
             case audioMasterProcessEvents:
                 return DynamicVstEvents(*static_cast<const VstEvents*>(data));
                 break;
-            // We detect whether an opcode should return a string by checking
+            // We detect whether an opcode should return a string by
-            // whether there's a zeroed out buffer behind the void pointer. This
+            // checking whether there's a zeroed out buffer behind the void
-            // works for any host, but not all plugins zero out their buffers.
+            // pointer. This works for any host, but not all plugins zero
+            // out their buffers.
             case audioMasterGetVendorString:
             case audioMasterGetProductString:
                 return WantsString{};
@@ -482,11 +476,11 @@ class HostCallbackDataConverter : DefaultDataConverter {
     void write(const int opcode, void* data, const EventResult& response) {
         switch (opcode) {
             case audioMasterGetTime:
-                // Write the returned `VstTimeInfo` struct into a field and make
+                // Write the returned `VstTimeInfo` struct into a field and
-                // the function return a poitner to it in the function below.
+                // make the function return a poitner to it in the function
-                // Depending on whether the host supported the requested time
+                // below. Depending on whether the host supported the
-                // information this operations returns either a null pointer or
+                // requested time information this operations returns either
-                // a pointer to a `VstTimeInfo` object.
+                // a null pointer or a pointer to a `VstTimeInfo` object.
                 if (std::holds_alternative<std::nullptr_t>(response.payload)) {
                     time_info = std::nullopt;
                 } else {
@@ -502,8 +496,8 @@ class HostCallbackDataConverter : DefaultDataConverter {
     intptr_t return_value(const int opcode, const intptr_t original) {
         switch (opcode) {
             case audioMasterGetTime: {
-                // Return a pointer to the `VstTimeInfo` object written in the
+                // Return a pointer to the `VstTimeInfo` object written in
-                // function above
+                // the function above
                 VstTimeInfo* time_info_pointer = nullptr;
                 if (time_info.has_value()) {
                     time_info_pointer = &time_info.value();

src/wine-host/bridges/vst2.h

@@ -33,6 +33,13 @@
 #include "../editor.h"
 #include "../utils.h"
 
+/**
+ * A marker struct to indicate that the editor is about to be opened.
+ *
+ * @see Vst2Bridge::editor
+ */
+struct EditorOpening {};
+
 /**
  * This hosts a Windows VST2 plugin, forwards messages sent by the Linux VST
  * plugin and provides host callback function for the plugin to talk back.
@@ -75,7 +82,9 @@ class Vst2Bridge {
      * but in our approach there will be an opportunity to handle events in
      * between these two calls. Most plugins will handle this just fine, but
      * some plugins end up blocking indefinitely while waiting for the other
-     * function to be called, hence why this function is needed.
+     * function to be called, hence why this function is needed. For
+     * individually hosted plugins this check is done implicitely in
+     * `Vst2Bridge::handle_win32_events()`.
      */
     bool should_skip_message_loop();
 
@@ -261,17 +270,25 @@ class Vst2Bridge {
      * Wine window, and embedding that Wine window into a window provided by the
      * host. Should be empty when the editor is not open.
      *
-     * There is some special behavior with regards to message handling when the
+     * This field can have three possible states:
-     * editor is in the process of being opened, see
+     *
-     * `should_postpone_message_loop()`.
+     * - `std::nullopt` when the editor is closed.
+     * - An `Editor` object when the editor is open.
+     * - `EditorOpening` when the editor is not yet open, but the host has
+     *   already called `effEditGetRect()` and is about to call `effEditOpen()`.
+     *   This is needed because there is a race condition in some bugs that
+     *   cause them to crash or enter an infinite Win32 message loop when
+     *   `effEditGetRect()` gets dispatched and we then enter the message loop
+     *   loop before `effEditOpen()` gets called. Most plugins will handle this
+     *   just fine, but a select few plugins make the assumption that the editor
+     *   is already open once `effEditGetRect()` has been called, even if
+     *   `effEditOpen` has not yet been dispatched. VST hsots on Windows will
+     *   call these two events in sequence, so the bug would never occur there.
+     *   To work around this we'll use this third state to temporarily stop
+     *   processing Windows events in the one or two ticks between these two
+     *   events.
      *
      * @see should_postpone_message_loop
      */
-    std::optional<Editor> editor;
+    std::variant<std::monostate, Editor, EditorOpening> editor;
-
-    /**
-     * Keeps track of when the editor is being opened during the two-phase
-     * process of the host calling `effEditOpen()` and `effEditGetRect()`.
-     */
-    bool editor_is_opening = false;
 };