ControllerEngine Refactoring#3463
Conversation
* Move src/controllers/engine to src/controllers/scripting * Create src/controllers/scripting/legacy subfolder * Rename ControllerEngine to ControllerScriptHandler * Rename ControllerEngineJSProxy to ControllerScriptInterface
This allows for a clean separation between new and legacy code.
This avoids calling two separate JS functions (one to convert the ArrayBuffer to a Uint8Array then the callback) every time controller input is received.
ArrayBuffer is more useful for HID and perhaps other use cases. For backwards compatibility, continue to convert the ArrayBuffer to a Uint8Array in ControllerScriptEngineLegacy following discussion on https://github.com/mixxxdj/mixxx/pull/2920/files/9363b497e13e39191655441a32b458f0af1727d8..e25bf12ab4bd910745d4359a9b3520b8c7f673c9#diff-69d807d0e5894e9f5bf1bd447ca878ab
Thanks codespell!
This will be reimplemented differently in the future.
m_pJSEngine is never passed to other threads
With the new engine, this should be implement in JavaScript code, e.g. inside the MIDI dispatcher class or something like that.
daschuer
left a comment
daschuer
left a comment
There was a problem hiding this comment.
Thank you for reviving this branch. I have left some comments.
| s_testing = true; | ||
| } | ||
|
|
||
| static ControlFlags shouldAssertForInvalidControlObjects() { |
There was a problem hiding this comment.
The function name is a question that can be answered with true or false but not with individual flags.
I would prefer to make this bool, because the flags are an implementation detail of code out of sight.
Alternative we need a new name.
|
|
||
| static ControlFlags shouldAssertForInvalidControlObjects() { | ||
| if (s_enabled && !s_testing) { | ||
| return ControlFlag::None; |
There was a problem hiding this comment.
None means assert all assumptions. What is the reasoning that we don't want that during tests? Is this correct:
| s_testing = true | s_testing = false | |
|---|---|---|
| s_enabled = true | ignore | assert |
| s_enabled = false | ignore | ignore |
Please add a comment why.
Thank you.
There was a problem hiding this comment.
We may also consider to rename ControlFlag::None to
ControlFlag::AssertAll; Or AssertExeptions:None or such.
There was a problem hiding this comment.
Please add a comment why.
This was done because some tests are expected to fail. I just pushed the new commits that disable controller testing for these tests.
|
Thank you LGTM. |
3 participants
Continuation of #2920 (great work by @Be-ing!) with merge conflicts resolved.
It also adds the comment to make @daschuer happy and moves the C++ style wrapping into the ControllerScriptEngineLegacy class. For the new API, we should just hand out the dataview and do the cast to Uint8Arry in actual JavaScript if necessary, not in an JavaScript-wrapped-in-C++-and-called-by-JavaScript abomination.