Migrate ControllerEngine to QJSEngine + improve error handling #1795

ferranpujolcamins · 2018-08-30T15:10:20Z

To do

Be-ing

Thanks for getting started on this!

build/depends.py

src/controllers/controller.h

src/controllers/controllerengine.cpp

src/controllers/engine/controllerengine.cpp

Be-ing · 2018-11-22T01:01:14Z

I made a pull request to update scripts now that ControllerEngine::getThisObjectInFunctionCall is not available with QJSEngine.

Be-ing · 2018-11-23T18:10:32Z

I have been thinking about whether it's a good idea to introduce C++ exceptions here, which AFAIK are not used anywhere else in Mixxx. I know many developers advise against ever using exceptions, or at least avoid them in C++. I don't have enough experience with them to have much of an opinion about that (although Rust's Result enum seems much better because always it forces the caller to deal with potential problems), but I don't think C++ exceptions are necessary or helpful in this code.

I understand it intuitively corresponds to JS Error objects, but I think it compounds the problem of this code being overcomplicated with inconsistent behavior. The overcomplicated code was there before you started working on it, but I think it would be wise to clean it up while working on this code. There are many different places in ControllerEngine where JS code is evaluated. Each of these have subtly different behavior when an error occurs. Sometimes an annoying dialog is shown, sometimes it just logs a warning, and it's not clear if the possibility of JS errors is properly handled every time they may happen.

I think it would be better to wrap all calls to QJSEngine::evaluate in a single function that handles the possibility of errors. Throwing an exception from that function requires a bunch of redundant try-catch code every time the function is called, which kinda defeats the point of having a reusable function.

Don't bind "this" in beginTimer and makeConnection callbacks Adapt ControllerEngine::checkException for QJSEngine Rename checkException to handleEvaluationException

Since QJSEngine takes ownership of exposed QObjects it tried to delete controller and controllerengine when QJSEngine was destroyed (i.e script reload and mixxx shutdown). See https://bugreports.qt.io/browse/QTBUG-41171 This also has the benefit that we have a tighter control on what's exposed. Since Signals and slots, properties and children of object are available as properties of the created QJSValue, lots of undesired things were leaked into the JS engine.

This helps inspecting the test results in eclipse test inspector.

Remove bytearrayclass since now QJSEngine has built in support for QByteArray, which gets converted to a JS ArrayBuffer.

Be-ing · 2018-11-24T13:48:58Z

Please add new commits instead of rebasing. I cannot easily tell what you changed in the code since the last time I looked at it and now my PR does not cleanly merge. If we decide to clean up the history later, we can do that at the end before merging.

The old hacks for preserving the "this" object for engine.makeConnection, engine.connectControl, and engine.beginTimer do not work with QJSEngine, but QJSEngine brings support for ES5, which supports Function.prototype.bind. https://bugs.launchpad.net/mixxx/+bug/1733666

src/controllers/engine/controllerengine.cpp

ferranpujolcamins · 2020-03-23T16:52:17Z

fixing

# Conflicts: # appveyor.yml # build/depends.py # res/controllers/midi-components-0.0.js # src/controllers/controller.h # src/controllers/controllerengine.h # src/controllers/engine/colorjsproxy.cpp # src/controllers/engine/controllerengine.cpp # src/controllers/hid/hidcontroller.cpp # src/preferences/dialog/dlgpreferences.cpp # src/preferences/dialog/dlgpreferences.h # src/skin/skincontext.cpp # src/skin/skincontext.h # src/test/controller_preset_validation_test.cpp # src/test/controllerengine_test.cpp

# Conflicts: # src/preferences/dialog/dlgpreferencesdlg.ui

Be-ing · 2020-03-27T19:37:44Z

Hmm there's still a merge conflict with src/preferences/dialog/dlgpreferencesdlg.ui

ferranpujolcamins · 2020-03-27T19:39:28Z

I need to fix tests and do some manual testing

Holzhaus · 2020-04-09T22:22:26Z

I already started resolving merge conflicts that emerged due to my recent controller related changes, but then I noticed that this doesn't build as-is. A fix can be found here: ferranpujolcamins#15

Also, the ControllerPresetValidationTest.MidiPresetsValid test fails. Apparently the ColorMapper class has been removed?

ControllerEngine: "Uncaught exception at line 1004 in file file:///data/jan/Projects/mixxx/res/controllers/Roland_DJ-505-scripts.js: ReferenceError: ColorMapper is not defined"
ControllerEngine shutting down...
/home/jan/Projects/mixxx/src/test/controller_preset_validation_test.cpp:131: Failure
Value of: testLoadPreset(preset)
  Actual: false
Expected: true
Error while validating /data/jan/Projects/mixxx/res/controllers/Roland_DJ-505.midi.xml

controllers/bulk: Fix BulkController constructor

Holzhaus · 2020-04-16T14:12:37Z

Also, the ControllerPresetValidationTest.MidiPresetsValid test fails. Apparently the ColorMapper class has been removed?

ControllerEngine: "Uncaught exception at line 1004 in file file:///data/jan/Projects/mixxx/res/controllers/Roland_DJ-505-scripts.js: ReferenceError: ColorMapper is not defined"
ControllerEngine shutting down...
/home/jan/Projects/mixxx/src/test/controller_preset_validation_test.cpp:131: Failure
Value of: testLoadPreset(preset)
  Actual: false
Expected: true
Error while validating /data/jan/Projects/mixxx/res/controllers/Roland_DJ-505.midi.xml

Ping. This still fails both with cmake and scons.

Holzhaus · 2020-04-18T14:02:06Z

Conflicts resolved here: ferranpujolcamins#16
Since this branch is broken, I was unable to test it properly, so better double check if I missed something.

Be-ing · 2020-04-19T00:40:30Z

ColorMapper and ColorMapperJSProxy are fixed here: ferranpujolcamins#17

ControllerPresetValidationTest.MidiPresetsValid is failing in a strange way. It is saying there is an exception in Components when loading mappings that don't use Components??

LINT: "/home/be/sw/mixxx/res/controllers/Hercules DJControl Compact.midi.xml" has no forum link.
ControllerEngine: "Uncaught exception at line 126 in file /home/be/sw/mixxx/res/controllers/midi-components-0.0.js: TypeError: Result of expression 'this.output.bind' [undefined] is not a function."
src/test/controller_preset_validation_test.cpp:193: Failure
Value of: testLoadPreset(preset)
  Actual: false
Expected: true
...
LINT: "/home/be/sw/mixxx/res/controllers/Reloop Terminal Mix 2-4.midi.xml" has no forum link.
ControllerEngine: "Uncaught exception at line 126 in file /home/be/sw/mixxx/res/controllers/midi-components-0.0.js: TypeError: Result of expression 'this.output.bind' [undefined] is not a function."
src/test/controller_preset_validation_test.cpp:193: Failure
Value of: testLoadPreset(preset)
  Actual: false
Expected: true

Be-ing · 2020-04-19T02:48:10Z

Actually the tests do work. I got confused because CMake does not install the mixxx-test binary; it has to be run from the CMake build directory. I took the next step and implemented an API for scripts to work without XML mappings: #2682

ferranpujolcamins · 2020-04-22T17:05:05Z

@Holzhaus can you solve the merge conflicts you intoduced with #2588?

Be-ing · 2020-04-22T17:05:56Z

@ferranpujolcamins merge conflicts are already solved in #2682. I think we can close this PR without merging now.

This was introduced in PR mixxxdj#1795 when the migration to QJSEngine was planned for a release when the latest Ubuntu version shipped Qt 5.9, which did not support ES7. The purpose was to show users an error when they loaded a script using ES7 if they were using a Qt version before 5.12. Since Mixxx 2.3 took so long, the QJSEngine migration will be released when the latest Ubuntu LTS (20.04) ships Qt 5.12, so we do not need to introduce this "controller engine version" concept anymore. Mixxx will still build and run with Qt < 5.12 at the small cost of keeping only one #if in ControllerEngine::throwJSError, but mappings using ES7, which now includes any mapping using the Components library, will not work.

ferranpujolcamins force-pushed the migrate-to-QJSEngine branch 2 times, most recently from 81e6002 to b51f457 Compare September 1, 2018 22:34

Be-ing mentioned this pull request Oct 7, 2018

Behringer CMD-MM1 Rework #1744

Merged

ferranpujolcamins force-pushed the migrate-to-QJSEngine branch from 56f88d1 to 6a8f52b Compare October 16, 2018 20:19

ferranpujolcamins mentioned this pull request Nov 17, 2018

Variables in svg attributes #302

Merged

Be-ing suggested changes Nov 22, 2018

View reviewed changes

Be-ing mentioned this pull request Nov 22, 2018

Update scripts for QJSEngine ferranpujolcamins/mixxx#5

Merged

ferranpujolcamins added 15 commits November 24, 2018 13:28

Replace QtScript classes with QtQml script classes

e2441bd

Don't bind "this" in beginTimer and makeConnection callbacks Adapt ControllerEngine::checkException for QJSEngine Rename checkException to handleEvaluationException

Move controllerengine files into new engine subfolder

8e21856

Add checks for JS engine evaluation errors

08dd110

Remove unused evaluate slot in ControllerEngine

6ddae3d

Refactor exceptions

9b3b28f

Add nullengine exception handling

50f8059

Fix tests build

2b14555

Add controller script name to assertion message

a3475d8

This helps inspecting the test results in eclipse test inspector.

Fix script error not displaying file name

9009fc9

Remove comment

d7b6811

Adapt hid devices to new JS Engine

6bd1f15

Remove bytearrayclass since now QJSEngine has built in support for QByteArray, which gets converted to a JS ArrayBuffer.

Add QtQml dependency to travis config file

4047bf8

Update comments

5ed5280

Remove QScriptEngineDebugger from SkinContext

b3380ba

ferranpujolcamins force-pushed the migrate-to-QJSEngine branch from 6a8f52b to b3380ba Compare November 24, 2018 12:29

Be-ing added 3 commits November 24, 2018 08:05

Be-ing suggested changes Nov 24, 2018

View reviewed changes

src/controllers/engine/controllerengine.cpp Outdated Show resolved Hide resolved

src/controllers/engine/controllerengine.cpp Outdated Show resolved Hide resolved

dszakallas mentioned this pull request Mar 25, 2020

Update Novation Launchpad controller scripts #2600

Merged

2 tasks

Ferran Pujol Camins added 2 commits March 27, 2020 20:30

Merge branch 'master' into migrate-to-QJSEngine

cfc4142

# Conflicts: # src/preferences/dialog/dlgpreferencesdlg.ui

Be-ing mentioned this pull request Mar 30, 2020

add DDJ-400 contoller #2403

Closed

Be-ing mentioned this pull request Apr 7, 2020

Controllers: Don't needlessly copy presets into user directory #2569

Merged

controllers/bulk: Fix BulkController constructor

e122b79

ferranpujolcamins added 2 commits April 12, 2020 09:31

Merge pull request #15 from Holzhaus/bulk-controller-fix

97d5623

controllers/bulk: Fix BulkController constructor

Fix wrong file path in scons

381214a

Be-ing mentioned this pull request Apr 19, 2020

migrate legacy controller system to QJSEngine #2682

Merged

Swiftb0y mentioned this pull request Apr 19, 2020

midi-components: Improve error reporting #2583

Closed

ferranpujolcamins closed this Apr 22, 2020

Be-ing mentioned this pull request Jul 12, 2020

split up ControllerEngine #2920

Closed

Be-ing mentioned this pull request Sep 7, 2020

Traktor s3, main branch #3074

Closed

ronso0 removed this from the 2.4.0 milestone Nov 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate ControllerEngine to QJSEngine + improve error handling #1795

Migrate ControllerEngine to QJSEngine + improve error handling #1795

ferranpujolcamins commented Aug 30, 2018 •

edited

Loading

Be-ing left a comment

Be-ing commented Nov 22, 2018

Be-ing commented Nov 23, 2018 •

edited

Loading

Be-ing commented Nov 24, 2018 •

edited

Loading

ferranpujolcamins commented Mar 23, 2020

Be-ing commented Mar 27, 2020

ferranpujolcamins commented Mar 27, 2020

Holzhaus commented Apr 9, 2020 •

edited

Loading

Holzhaus commented Apr 16, 2020

Holzhaus commented Apr 18, 2020

Be-ing commented Apr 19, 2020

Be-ing commented Apr 19, 2020

ferranpujolcamins commented Apr 22, 2020 •

edited

Loading

Be-ing commented Apr 22, 2020

Migrate ControllerEngine to QJSEngine + improve error handling #1795

Migrate ControllerEngine to QJSEngine + improve error handling #1795

Conversation

ferranpujolcamins commented Aug 30, 2018 • edited Loading

To do

Be-ing left a comment

Choose a reason for hiding this comment

Be-ing commented Nov 22, 2018

Be-ing commented Nov 23, 2018 • edited Loading

Be-ing commented Nov 24, 2018 • edited Loading

ferranpujolcamins commented Mar 23, 2020

Be-ing commented Mar 27, 2020

ferranpujolcamins commented Mar 27, 2020

Holzhaus commented Apr 9, 2020 • edited Loading

Holzhaus commented Apr 16, 2020

Holzhaus commented Apr 18, 2020

Be-ing commented Apr 19, 2020

Be-ing commented Apr 19, 2020

ferranpujolcamins commented Apr 22, 2020 • edited Loading

Be-ing commented Apr 22, 2020

ferranpujolcamins commented Aug 30, 2018 •

edited

Loading

Be-ing commented Nov 23, 2018 •

edited

Loading

Be-ing commented Nov 24, 2018 •

edited

Loading

Holzhaus commented Apr 9, 2020 •

edited

Loading

ferranpujolcamins commented Apr 22, 2020 •

edited

Loading