Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: forwarding certain key events of 'wine_window' to 'host_window' #263

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Goli4thus
Copy link
Contributor

@Goli4thus Goli4thus commented Jul 7, 2023

TLDR: Found a way to forward key events received on wine_window to host_window.

backstory

This feature request: #259
Also inspired by some things I learned recently from linvst.

the not so intuitive part

All about this part:

const uint32_t event_mask = XCB_EVENT_MASK_NO_EVENT;
event->event = host_window_;
xcb_send_event(
        x11_connection_.get(),
        false, host_window_, event_mask,
        reinterpret_cast<const char*>(event));

The comments in actual code mention it somewhat, but here in more detail:

For whatever reason, using something like XCB_EVENT_MASK_KEY_PRESS for the event mask didn't work (i.e. sent key events didn't trigger anything in REAPER).
But using XCB_EVENT_MASK_NO_EVENT somehow makes it work.

I find this bit suprising:
My guess would have been that the whole event mask mechanism for deciding which events are sent to a client (e.g. REAPER) would be like a bitwise AND operation:

(event_mask__reaper & event_mask__xcb_send_event) != 0

If that evaluates to true, the event would be forwarded to REAPER by Xserver (which xcb, just like xlib builds upon).
But considering XCB_EVENT_MASK_NO_EVENT has the value 0 and considering this commit works kind of invalidates that guess. Almost seems like it would function as a wildcard of some sort and forward the event anyway, regardless of what REAPER might have set.

Then again, I didn't debug the Xserver to check if:
a) the event was sent with both event masks, but REAPER somehow dropped the event by itself
b) if the event really wasn't sent by Xserver, because it figured that REAPER has set an event mask that just doesn't allow it (still leaves the question how XCB_EVENT_MASK_NO_EVENT actually works)

Open points

  1. I couldn't figure out how to detect the current host being REAPER or something else. The goal would be to just enable this forwarding for REAPER (not point in having it for bitwig).
  2. I'm open for criticism on what could be improved (apart from point 1 above). Obviously I'm still quite unfamiliar with the codebase, so I can't be too sure if this isn't breaking anything. But at least on REAPER it seemed rather solid.

For now it's about 'spacebar' and 'function keys' regarding keycodes.
Forwarding can temporarily be disabled via 'shift + hover' mechanism.

This commit has a WIP status, because:
- this forwarding mechanism is atm. applied in general for any DAW, even
  though it's only meant for REAPER only
  (TBD how to detect the present host is REAPER)
- maybe could use some improving for things like usage of logging or
  code style
@Goli4thus
Copy link
Contributor Author

Forgot a couple things to mention:

  1. For the most part one could just forward all keys to REAPER, cause it would ignore everything except spacebar and function keys. But in conjunction with REAPER's FX window, keys like a would trigger some things in FX window. That's why this narrowing down made sense.
  2. Due the the resp. key events being forwarded in addition to wine_window events, it means that when a textbox in editor window has focus and one types spacebar, it would also toggle play/pause in REAPER. That's why that shift + hover mechanism was added. In a sense, it's bit similar to the situation with bitwig and spacebar, just kind of the other way around ("too much instead of too little spacebar").

@robbert-vdh robbert-vdh linked an issue Jul 9, 2023 that may be closed by this pull request
Copy link
Owner

@robbert-vdh robbert-vdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this approach (that I also briefly outlined in #259) is that you have incomplete information if you do this only on the X11 side, and doing this on both the Windows and the X11 side gets messy real quickly. For instance, if I enter a space character in a (non-popup) text input field in a plugin in REAPER (MNotepad, for instance), it will both insert a space character and play/pause transport. They key press should go either to REAPER or to the Wine/the plugin, but not to both.

I tried this patch in REAPER and with the current implementation whether keypresses get passed through to REAPER or not is also a bit flaky. Sometimes it works, other times it doesn't. But that's something that can be fixed of course.

I think a feature like this could be useful, but it will need to be behind an optional yabridge.toml compatibility option since it could result in unexpected behavior. And it would need to take care of the KEYCODE/scan code translation, and it would need to prevent the event from being processed by the plugin (so it would basically behave like in Bitwig). I'll see if I have some time this week to take a look at that.

* is typical for REAPER to receive even when editor (wine) window has
* focus (i.e. native behavior on all OS).
*/
const std::vector<int> keycodes_to_forward {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of the reason why I didn't really want to look into myself (and I'd much rather have REAPER implement similar behavior as Bitwig) is that doing something like this is quite brittle, and doing it the right way is a bit involved. These key codes are specific to QWERTY keyboard layouts (and potentially other system settings). The proper way to do this is to map a list of KEYSYMs to KEYCODEs first. See the keyboard section of the X11 spec here: https://www.x.org/releases/X11R7.7/doc/xproto/x11protocol.html#Keyboards

(this should also be an std::unordered_set for constant time lookups, but I can fix those things up later)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of the reason why I didn't really want to look into myself (and I'd much rather have REAPER implement similar behavior as Bitwig) is that doing something like this is quite brittle, and doing it the right way is a bit involved. These key codes are specific to QWERTY keyboard layouts (and potentially other system settings). The proper way to do this is to map a list of KEYSYMs to KEYCODEs first. See the keyboard section of the X11 spec here: https://www.x.org/releases/X11R7.7/doc/xproto/x11protocol.html#Keyboards

I've just read through that section you linked (couple times actually). I guess I can see for why this would be the "proper way" of making this robust for any KEYSYM.

Maybe I just wrongly assumed that at least keys like spacebar> and function keys would be quite universally always the same KEYCODE on X11. At least that's the impression I got after finding things like this one:
https://gist.github.com/rickyzhang82/8581a762c9f9fc6ddb8390872552c250

I did find this library mentioned during my research: https://xkbcommon.org/
Using the KEYCODE directly just seemed the only feasible thing for me at the time without adding another dependency like this one. But I might have been mistaken of what can be done "natively" with xcb.

(this should also be an std::unordered_set for constant time lookups, but I can fix those things up later)

Just had to read up on it. Yeah, I guess that would be the correct choice for a lookup scenario in general. I didn't even think of this, because I figured for such a small list that won't grow any further (assuming this keeps being just about spacebar and function keys), any kind of performance gains from a more sophisticated data structure than a simple array seemed negligible. Or if it's about avoiding any kind of issues due to "variable execution time" down the line, well, I'd assume even a simple for-loop for such a small list would be hard to notice.
But of course, I didn't make the measurement. Really just tried to learn something from this.

@Goli4thus
Copy link
Contributor Author

The problem with this approach (that I also briefly outlined in #259) is that you have incomplete information if you do this only on the X11 side, and doing this on both the Windows and the X11 side gets messy real quickly. For instance, if I enter a space character in a (non-popup) text input field in a plugin in REAPER (MNotepad, for instance), it will both insert a space character and play/pause transport. They key press should go either to REAPER or to the Wine/the plugin, but not to both.

Totally agree! That shift + hover mechanism I added in here was just kind of a hack to temporarily disable that spacebar from reaching REAPER in case one wants to type some text in the editor window while not also have it toggle play/stop while doing so.

If there's a way to block spacebar from reaching the editor window by default, just so it's in line with bitwig behavior, then that certainly would be better. I simply couldn't figure out how to intercepts key events before they reach the focused editor window.

Even better of course would be to detect key events that are "emitted" by the editor window and only forward those to REAPER. I don't know how else to say that.

https://www.kvraudio.com/forum/viewtopic.php?t=458972

Discussions like these certainly make me think that's how it usually works (at least for REAPER):

  • key events are captured by plugin first
  • any key events the plugin doesn't "consume" (e.g. spacebar, because not input field is focused) will be forwarded to DAW

But even that one discussion makes it clear that this behavior is not always guaranteed among all plugins.

But yeah, maybe there is a way to listen for such "emitted" events somehow. Then this whole forcing of spacebar to either host or editor window would kind of become obsolete.

I tried this patch in REAPER and with the current implementation whether keypresses get passed through to REAPER or not is also a bit flaky. Sometimes it works, other times it doesn't. But that's something that can be fixed of course.

I admit I didn't test this with all that many plugins. And I assume that shift + hover mechanism didn't get in the way for you.

I think a feature like this could be useful, but it will need to be behind an optional yabridge.toml compatibility option since it could result in unexpected behavior. And it would need to take care of the KEYCODE/scan code translation, and it would need to prevent the event from being processed by the plugin (so it would basically behave like in Bitwig). I'll see if I have some time this week to take a look at that.

Yes, this certainly shouldn't be forced on users by default if there's a chance of it acting up with certain plugins.

This certainly isn't meant as a finished patch, it's really just a draft. So feel free to change or even discard it if you think there's a better approach! If all this patch ends up being is a spark for further ideas on how to solve this topic robustly, then it was still worth it IMO.

And take your time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

optional keyboard passthrough
2 participants