Clean up the editor implementation

README.md

@@ -7,6 +7,8 @@ Yet Another way to use Windows VST2 plugins in Linux VST hosts.
 There are a few things that should be done before releasing this, including:
 
 - Fix implementation bugs:
+  - Fix hiding and showing of editor windows, closing a window with alt+f4
+    freezes the editor,
   - Polish GUIs even further. There are some todos left in
     `src/wine-host/editor.{h,cpp}`.
   - There are likely some minor issues left.

src/wine-host/editor.cpp

@@ -30,59 +30,42 @@ xcb_window_t get_x11_handle(HWND win32_handle);
 
 ATOM register_window_class(std::string window_class_name);
 
-Editor::Editor(std::string window_class_name)
+Editor::Editor(const std::string& window_class_name,
-    : window_class(register_window_class(window_class_name)),
+               AEffect* effect,
-      x11_connection(xcb_connect(nullptr, nullptr), &xcb_disconnect) {}
+               const size_t parent_window_handle)
-
+    :  // Needed to send update messages on a timer
-HWND Editor::open(AEffect* effect) {
+      plugin(effect),
-    // Create a window without any decoratiosn for easy embedding. The
+      window_class(register_window_class(window_class_name)),
-    // combination of `WS_EX_TOOLWINDOW` and `WS_POPUP` causes the window to be
+      // Create a window without any decoratiosn for easy embedding. The
-    // drawn without any decorations (making resizes behave as you'd expect) and
+      // combination of `WS_EX_TOOLWINDOW` and `WS_POPUP` causes the window to
-    // also causes mouse coordinates to be relative to the window itself.
+      // be drawn without any decorations (making resizes behave as you'd
-    win32_handle =
+      // expect) and also causes mouse coordinates to be relative to the window
-        std::unique_ptr<std::remove_pointer_t<HWND>, decltype(&DestroyWindow)>(
+      // itself.
-            CreateWindowEx(WS_EX_TOOLWINDOW | WS_EX_ACCEPTFILES,
+      win32_handle(CreateWindowEx(WS_EX_TOOLWINDOW | WS_EX_ACCEPTFILES,
-                           reinterpret_cast<LPCSTR>(window_class),
+                                  reinterpret_cast<LPCSTR>(window_class),
-                           "yabridge plugin", WS_POPUP, CW_USEDEFAULT,
+                                  "yabridge plugin",
-                           CW_USEDEFAULT, client_area_width, client_area_height,
+                                  WS_POPUP,
-                           nullptr, nullptr, GetModuleHandle(nullptr), this),
+                                  CW_USEDEFAULT,
-            &DestroyWindow);
+                                  CW_USEDEFAULT,
-
+                                  client_area_width,
-    // Needed to send update messages on a timer
+                                  client_area_height,
-    plugin = effect;
+                                  nullptr,
-
+                                  nullptr,
+                                  GetModuleHandle(nullptr),
+                                  this),
+                   &DestroyWindow),
+      parent_window(parent_window_handle),
+      child_window(get_x11_handle(win32_handle.get())),
+      x11_connection(xcb_connect(nullptr, nullptr), &xcb_disconnect) {
     // 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
     // update in the background. Because of this we send `effEditIdle` to the
     // plugin on a timer. The refresh rate is purposely fairly low since we
     // we'll also trigger this manually in `Editor::handle_events()` whenever
     // the plugin is not busy.
-    SetTimer(win32_handle->get(), idle_timer_id, 100, nullptr);
+    SetTimer(win32_handle.get(), idle_timer_id, 100, nullptr);
 
-    return win32_handle->get();
+    // see the x11 events part of `editor::handle_events`
-}
-
-void Editor::close() {
-    // RAII will destroy the window and tiemrs for us
-    win32_handle = std::nullopt;
-
-    // TODO: We might want to manually unmap the X11 window instead of having
-    //       the host do it. Right now the window editor window stays open for a
-    //       second when removing a plugin.
-}
-
-bool Editor::embed_into(const size_t parent_window_handle) {
-    if (!win32_handle.has_value()) {
-        return false;
-    }
-
-    child_window = get_x11_handle(win32_handle->get());
-    parent_window = parent_window_handle;
-
-    // See the X11 events part of `Editor::handle_events`.
-    // const uint32_t child_event_mask = XCB_EVENT_MASK_STRUCTURE_NOTIFY;
-    // xcb_change_window_attributes(x11_connection.get(), child_window,
-    //                              XCB_CW_EVENT_MASK, &child_event_mask);
     const uint32_t parent_event_mask = XCB_EVENT_MASK_STRUCTURE_NOTIFY;
     xcb_change_window_attributes(x11_connection.get(), parent_window,
                                  XCB_CW_EVENT_MASK, &parent_event_mask);
@@ -97,91 +80,85 @@ bool Editor::embed_into(const size_t parent_window_handle) {
     xcb_map_window(x11_connection.get(), child_window);
     xcb_flush(x11_connection.get());
 
-    ShowWindow(win32_handle->get(), SW_SHOWNORMAL);
+    ShowWindow(win32_handle.get(), SW_SHOWNORMAL);
-
-    return true;
 }
 
 void Editor::handle_events() {
     // Process any remaining events, otherwise we won't be able to interact with
     // the window
-    if (win32_handle.has_value()) {
+    bool gui_was_updated = false;
-        bool gui_was_updated = false;
+    MSG msg;
-        MSG msg;
 
-        // The second argument has to be null since we not only want to handle
+    // The second argument has to be null since we not only want to handle
-        // events for this window but also for all child windows (i.e.
+    // events for this window but also for all child windows (i.e. dropdowns). I
-        // dropdowns). I spent way longer debugging this than I want to admit.
+    // spent way longer debugging this than I want to admit.
-        while (PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE)) {
+    while (PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE)) {
-            TranslateMessage(&msg);
+        TranslateMessage(&msg);
-            DispatchMessage(&msg);
+        DispatchMessage(&msg);
 
-            if (msg.message == WM_TIMER && msg.wParam == idle_timer_id) {
+        if (msg.message == WM_TIMER && msg.wParam == idle_timer_id) {
-                gui_was_updated = true;
+            gui_was_updated = true;
-            }
         }
+    }
 
-        // Make sure that the GUI always gets updated at least once for every
+    // Make sure that the GUI always gets updated at least once for every
-        // `effEditIdle` call the host has sent to improve responsiveness when
+    // `effEditIdle` call the host has sent to improve responsiveness when the
-        // the GUI isn't being blocked.
+    // GUI isn't being blocked.
-        if (!gui_was_updated) {
+    if (!gui_was_updated) {
-            SendMessage(win32_handle->get(), WM_TIMER, idle_timer_id, 0);
+        SendMessage(win32_handle.get(), WM_TIMER, idle_timer_id, 0);
-        }
+    }
-
+
-        // Handle X11 events
+    // Handle X11 events
-        // TODO: Check if we should forward other events mostly to prevent
+    // TODO: Check if we should forward other events mostly to prevent
-        //       unnecessary GUI processing in the background. Since
+    //       unnecessary GUI processing in the background. Since `effEditIdle`
-        //       `effEditIdle` should only be called when the plugin's editor is
+    //       should only be called when the plugin's editor is open this should
-        //       open this should not cause any different in CPU though.
+    //       not cause any different in CPU though.
-        // TODO: Check whether drag and drop works out of the box
+    // TODO: Check whether drag and drop works out of the box
-        xcb_generic_event_t* generic_event;
+    xcb_generic_event_t* generic_event;
-        while ((generic_event = xcb_poll_for_event(x11_connection.get())) !=
+    while ((generic_event = xcb_poll_for_event(x11_connection.get())) !=
-               nullptr) {
+           nullptr) {
-            switch (generic_event->response_type & event_type_mask) {
+        switch (generic_event->response_type & event_type_mask) {
-                case XCB_CONFIGURE_NOTIFY: {
+            case XCB_CONFIGURE_NOTIFY: {
-                    xcb_configure_notify_event_t event =
+                xcb_configure_notify_event_t event =
-                        *reinterpret_cast<xcb_configure_notify_event_t*>(
+                    *reinterpret_cast<xcb_configure_notify_event_t*>(
-                            generic_event);
+                        generic_event);
-                    if (event.window != parent_window) {
+                if (event.window != parent_window) {
-                        break;
+                    break;
-                    }
+                }
-
+
-                    // We're purposely not using XEmbed. This has the
+                // We're purposely not using XEmbed. This has the consequence
-                    // consequence that wine still thinks that any X and Y
+                // that wine still thinks that any X and Y coordinates are
-                    // coordinates are relative to the x11 window root instead
+                // relative to the x11 window root instead of the parent window
-                    // of the parent window provided by the DAW, causing all
+                // provided by the DAW, causing all sorts of GUI interactions to
-                    // sorts of GUI interactions to break. To alleviate this
+                // break. To alleviate this we'll just lie to Wine and tell it
-                    // we'll just lie to Wine and tell it that it's located at
+                // that it's located at the parent window's location. We'll only
-                    // the parent window's location. We'll only send the event
+                // send the event instead of actually configuring the window.
-                    // instead of actually configuring the window.
+                // NOTE: We're not actually using `SetWindowPos()` to resize the
-                    // NOTE: We're not actually using `SetWindowPos()` to resize
+                //       window. The editor's client area will likely always be
-                    //       the window. The editor's client area will likely
+                //       big enough Since we specified the Window to be
-                    //       always be big enough Since we specified the Window
+                //       2560x1440 pixels large at the time of its creation.
-                    //       to be 2560x1440 pixels large at the time of its
+                //       This works because the embedding hierarchy is DAW
-                    //       creation. This works because the embedding
+                //       window -> Win32 window (created in this class) -> VST
-                    //       hierarchy is DAW window -> Win32 window (created in
+                //       plugin window created by the plugin itself. This makes
-                    //       this class) -> VST plugin window created by the
+                //       the drag-to-resize functionality many plugin editors
-                    //       plugin itself. This makes the drag-to-resize
+                //       have feel smooth and native.
-                    //       functionality many plugin editors have feel smooth
+                xcb_configure_notify_event_t translated_event{};
-                    //       and native.
+                translated_event.response_type = XCB_CONFIGURE_NOTIFY;
-                    xcb_configure_notify_event_t translated_event{};
+                translated_event.event = child_window;
-                    translated_event.response_type = XCB_CONFIGURE_NOTIFY;
+                translated_event.window = child_window;
-                    translated_event.event = child_window;
+                translated_event.width = client_area_width;
-                    translated_event.window = child_window;
+                translated_event.height = client_area_height;
-                    translated_event.width = client_area_width;
+                translated_event.x = event.x;
-                    translated_event.height = client_area_height;
+                translated_event.y = event.y;
-                    translated_event.x = event.x;
+
-                    translated_event.y = event.y;
+                xcb_send_event(x11_connection.get(), false, child_window,
-
+                               XCB_EVENT_MASK_STRUCTURE_NOTIFY |
-                    xcb_send_event(x11_connection.get(), false, child_window,
+                                   XCB_EVENT_MASK_SUBSTRUCTURE_NOTIFY,
-                                   XCB_EVENT_MASK_STRUCTURE_NOTIFY |
+                               reinterpret_cast<char*>(&translated_event));
-                                       XCB_EVENT_MASK_SUBSTRUCTURE_NOTIFY,
+                xcb_flush(x11_connection.get());
-                                   reinterpret_cast<char*>(&translated_event));
+            } break;
-                    xcb_flush(x11_connection.get());
-                } break;
-            }
-            free(generic_event);
         }
+        free(generic_event);
     }
 }
 

src/wine-host/editor.h

@@ -18,11 +18,11 @@
 
 /**
  * A wrapper around the win32 windowing API to create and destroy editor
- * windows. A VST plugin can embed itself in that window, and we can then later
+ * windows. We can embed this window into the window provided by the host, and a
- * embed the window in a VST host provided X11 window.
+ * VST plugin can then later embed itself in the window create here.
  *
- * This was originally implemented using XEmbed. While that sounded like the
+ * This was originally implemented using XEmbed. Even though that sounded like
- * right thing to do, there were a few small issues with Wine's XEmbed
+ * the right thing to do, there were a few small issues with Wine's XEmbed
  * implementation. The most important of which is that resizing GUIs sometimes
  * works fine, but often fails to expand the embedded window's client area
  * leaving part of the window inaccessible. There are also some a small number
@@ -32,41 +32,25 @@
  * issues, please let me know and I'll switch to using XEmbed again.
  *
  * This workaround was inspired by LinVST.
- *
- * TODO: Refactor this so we don't need to stage initialization and just add an
- *       `std::optional<Editor>` to `PluginBridge` instead
  */
 class Editor {
    public:
     /**
-     * @param window_class_name The name for the window class for editor
+     * Open a window, embed it into the DAW's parent window and create a handle
-     *   windows.
-     */
-    Editor(std::string window_class_name);
-
-    /**
-     * Open a window, embed it into the DAW's parent window and return a handle
      * to the new Win32 window that can be used by the hosted VST plugin.
      *
+     * @param window_class_name The name for the window class for editor
+     *   windows.
      * @param effect The plugin this window is being created for. Used to send
      *   `effEditIdle` messages on a timer.
-     *
-     * @return The Win32 window handle of the newly created window.
-     */
-    HWND open(AEffect* effect);
-
-    void close();
-
-    /**
-     * Embed the (open) window into a parent window.
-     *
      * @param parent_window_handle The X11 window handle passed by the VST host
      *   for the editor to embed itself into.
      *
-     * @return Whether the embedding was succesful. Will return false if the
+     * @see win32_handle
-     *   window is not open.
      */
-    bool embed_into(const size_t parent_window_handle);
+    Editor(const std::string& window_class_name,
+           AEffect* effect,
+           const size_t parent_window_handle);
 
     /**
      * Pump messages from the editor GUI's event loop until all events are
@@ -74,10 +58,23 @@ class Editor {
      * of Win32 limitations. I guess that's what `effEditIdle` is for.
      */
     void handle_events();
-
     // Needed to handle idle updates through a timer
     AEffect* plugin;
 
+   private:
+    /**
+     * The Win32 window class registered for the windows window.
+     */
+    ATOM window_class;
+
+   public:
+    /**
+     * The handle for the window created through Wine that the plugin uses to
+     * embed itself in.
+     */
+    const std::unique_ptr<std::remove_pointer_t<HWND>, decltype(&DestroyWindow)>
+        win32_handle;
+
    private:
     /**
      * The window handle of the editor window created by the DAW.
@@ -88,18 +85,10 @@ class Editor {
      */
     xcb_window_t child_window;
 
-    /**
-     * The Win32 window class registered for the windows window.
-     */
-    ATOM window_class;
-
     /**
      * A pointer to the currently active window. Will be a null pointer if no
      * window is active.
      */
-    std::optional<
-        std::unique_ptr<std::remove_pointer_t<HWND>, decltype(&DestroyWindow)>>
-        win32_handle;
 
     std::unique_ptr<xcb_connection_t, decltype(&xcb_disconnect)> x11_connection;
 };

src/wine-host/plugin-bridge.cpp

@@ -66,8 +66,7 @@ PluginBridge::PluginBridge(std::string plugin_dll_path,
       vst_host_callback(io_context),
       host_vst_parameters(io_context),
       host_vst_process_replacing(io_context),
-      vst_host_aeffect(io_context),
+      vst_host_aeffect(io_context) {
-      editor("yabridge plugin") {
     // Got to love these C APIs
     if (plugin_handle == nullptr) {
         throw std::runtime_error("Could not load a shared library at '" +
@@ -225,39 +224,29 @@ intptr_t PluginBridge::dispatch_wrapper(AEffect* plugin,
             // To allow the GUI to update even when this thread gets blocked
             // (e.g. when a dropdown is open), the actual `effEditIdle` event
             // gets sent to the plugin on a timer.
-            editor.handle_events();
+            editor->handle_events();
 
             return 1;
             break;
-        case effClose: {
-            // Closing the editor will also shut down the thread that's
-            // currently handling events
-            editor.close();
-
-            return plugin->dispatcher(plugin, opcode, index, value, data,
-                                      option);
-        } break;
         case effEditOpen: {
-            const auto win32_handle = editor.open(plugin);
+            // Create a Win32 window through Wine, embed it into the window
-
+            // provided by the host, and let the plugin embed itself into the
-            const auto return_value = plugin->dispatcher(
+            // Wine window
-                plugin, opcode, index, value, win32_handle, option);
-            if (return_value == 0) {
-                return 0;
-            }
-
-            // If opening the editor was succesful, reparent it to the window
-            // provided by the DAW
             const auto x11_handle = reinterpret_cast<size_t>(data);
-            editor.embed_into(x11_handle);
+            editor.emplace("yabridge plugin", plugin, x11_handle);
 
-            return return_value;
+            return plugin->dispatcher(plugin, opcode, index, value,
+                                      editor->win32_handle.get(), option);
         } break;
         case effEditClose: {
             const intptr_t return_value =
                 plugin->dispatcher(plugin, opcode, index, value, data, option);
 
-            editor.close();
+            // Cleanup is handled through RAII
+            // TODO: We might want to manually unmap the X11 window instead of
+            //       having the host do it. Right now the window editor window
+            //       stays open for a second when removing a plugin.
+            editor = std::nullopt;
 
             return return_value;
         } break;

src/wine-host/plugin-bridge.h

@@ -156,5 +156,10 @@ class PluginBridge {
      */
     std::vector<uint8_t> process_buffer;
 
-    Editor editor;
+    /**
+     * The plugin editor window. Allows embedding the plugin's editor into a
+     * Wine window, and embedding that Wine window into a window provided by the
+     * host. Should be empty when the editor is not open.
+     */
+    std::optional<Editor> editor;
 };