qt5: Replace deprecated QtScript with QJSEngine

[mixxxbot]

Reported by: Pegasus-RPG
Date: 2017-11-21T18:00:37Z
Status: Fix Committed
Importance: Low
Launchpad Issue: lp1733666
Tags: controllers, qt5
Attachments: sample.js

---

As of Qt5.5, QtScript for Applications has been deprecated: http://wiki.qt.io/New_Features_in_Qt_5.5#Deprecated_Functionality

Its replacement is QJSEngine: http://doc.qt.io/qt-5/qjsengine.html

While there is currently no mention of QtScript being removed, it's not being improved so we should consider migrating.

[mixxxbot]

Commented by: Be-ing
Date: 2017-11-21T19:21:11Z

---

It seems there isn't a way to get the current "this" object from a C++ function that is exposed to the JS environment, which will break engine.makeConnection and engine.beginTimer. However, QJSEngine supports ECMAScript 5, which introduced Function.prototype.bind. This will require some careful planning...

[mixxxbot]

Commented by: Be-ing
Date: 2017-12-28T03:54:20Z

---

I am assigning this to the 2.3 milestone for now, but I'm not sure we'll get to it by then. Let's reconsider when the 2.2 beta branch is cut.

[mixxxbot]

Commented by: Be-ing
Date: 2017-12-28T03:55:08Z

---

It may make sense to implement this concurrently with Ctlra support.

[mixxxbot]

Commented by: Pegasus-RPG
Date: 2018-01-30T20:52:01Z

---

In the worst case, can we do something in C++ that works around the "this" object problems?

This also may be a good time to plan how we want to expose some C++ objects (e.g. ControlObjects) directly to scripts. (I'm not even sure if QJSEngine can do that like QtScript can.)

[mixxxbot]

Commented by: ferranpujolcamins
Date: 2018-03-25T23:37:21Z
Attachments: sample.js

---

It seems there isn't a way to get the current "this" object from a C++ function that is exposed to the JS environment, which will break engine.makeConnection and engine.beginTimer.

The current implementation is actually wrong. We should not artificially bind to "this" in makeConnection and beginTimer. It can cause unexpected behaviour (see attached example).

As a javascript programmer, you must know that sometimes "this" won't be bind to what you expect at first glance. The current implementation makes the thing easier in some cases, but introduces unexpected behaviour in other cases. I prefer users to have the need to always bind "this" themselves over them to figure out why something that should not happen is actually happening.

[mixxxbot]

Commented by: ferranpujolcamins
Date: 2018-03-25T23:43:58Z

---

By the way, are you actively working on this / have you made significant progress? I'd like to take it otherwise.

[mixxxbot]

Commented by: Be-ing
Date: 2018-03-26T00:00:35Z

---

We should not artificially bind to "this" in makeConnection and beginTimer.

You are correct, this imposes some unexpected limitations. That actually just caused me trouble working on the Xone K2 mapping and I had to modify the Components library to work around it: 987520e#diff-9bedde251ff7d14d89b40d05168275e4R617

I have not started working on this. I have only very briefly glanced at the QJSEngine documentation. I'd be happy to work together with you on this after the 2.1 release.

[mixxxbot]

Commented by: ferranpujolcamins
Date: 2018-03-26T00:03:05Z

---

Nice, I'm working on it meanwhile.

[mixxxbot]

Commented by: ferranpujolcamins
Date: 2018-03-27T07:54:55Z

---

I think the solution is to add support for ES6 arrow functions. Users that are new to JS will just learn to use arrow functions in makeConnection and beginTimer because that's what we'll document in the wiki. On the other hand, seasoned JS developers will already know what a arrow function does, so they will know what's going on, because Mixxx will not bind "this" unexpectedly anymore.

How can we bring support for arrow functions?

1 - add a second QJSEngine in ControllerEngine, let's call it preprocessorEngine.
2 - bundle a browserified build of babel (https://github.com/babel/babelify) with only the arrow function plugin (https://babeljs.io/docs/plugins/transform-es2015-arrow-functions/)
3 - load babel in preprocessorEngine
4 - when a script is loaded, first pass through babel in preprocessorEngine, then evaluate the transformed script in engine.

I'd like to write tests for each JS extension we add, so I think it's not a good idea to bundle a full-blown babel build to write ES6 code. We can add specific JS extensions you fancy. I'd like to add just specific plugins for specific and justified purposes.

How can we keep compatibility for old scripts?

I think I'll add a ControllerEngine interface and then make a class for the current implementation and a class for the new implementation. Then add a engine version field in the xml file to choose which version to use (if no version is specified, use the new version). Then, specify old engine version for mappings that are not tested with the new engine when we are about to release Mixxx.
This also has the benefit that next person that needs to update the engine will have things a little bit easier.
This needs a little bit of work because there are uses of QScriptValue outside ControllerEngine.

What do you think of these ideas?

[mixxxbot]

Commented by: ferranpujolcamins
Date: 2018-03-27T08:19:55Z

---

I've just discovered this: https://github.com/quickly/quickly
And essentially what they do is use Babel to transform ES6 to ES5, so maybe we can ship a full babel build...

[mixxxbot]

Commented by: ferranpujolcamins
Date: 2018-03-27T08:37:23Z

---

Although, the difference is that quickly runs babel on node, and my plan is to run it on QJSEngine. So better stick with the original plan and only add the arrow function pluguin.

[mixxxbot]

Commented by: Be-ing
Date: 2018-03-27T14:21:13Z

---

Yes, the current way of getting "this" for functions passed to engine.makeConnection is basically the same as arrow functions. However, I don't think it's a great idea to bring in external dependencies to hack around QJSEngine's limitations. I think that effort would be better spent contributing the desired functionality upstream to Qt. I recognize that is likely harder, but I think it would be more sustainable. I also don't think it's a good idea to recommend developers to rely on external JavaScript tooling to write mappings. We can mention that it's a possibility, but I am afraid that setting that up would be such a pain that it would deter people from contributing mappings.

IIRC the ancient QtScript JS interpeter does not support Function.prototype.bind, but I think QJSEngine does. Requiring a call to Function.prototype.bind for every function passed to engine.makeConnection would be annoying, but in general I don't think controller mapping developers should be using engine.makeConnection directly. I think Component.prototype.connect should abstract those details away unless the mapping needs something fancy for a Component with a custom "connect" function implemented.

I am quite confused why Qt hasn't implemented full ES6 support yet considering it uses JavaScript so heavily now. There is an issue on Qt's tracker for this:
https://bugreports.qt.io/browse/QTBUG-47735

[mixxxbot]

Commented by: ferranpujolcamins
Date: 2018-03-28T19:41:59Z

---

Developers would not need to rely on external Javascript functionality.
My proposal is for Mixxx to have native support for arrow functions.
But instead of implementing it in c++, we load a well known and popular JS library.

[mixxxbot]

Commented by: ferranpujolcamins
Date: 2018-03-28T23:10:17Z

---

Other idea:

Let's make engine.makeConnection take an additional parameter that is the object that the callback will be bond to.

Then, on engine init, eval the following code:

Object.prototype.makeConnection = function(group, name, callback) {
    return engine.makeConnection(group, name, callback, this);
};

So if you call makeConnection on an object instead of on engine, "this" is bond to the object.

The same can be done with beginTimer.

I think is a good enough compromise solution until we can get "this" for a function in QJSEngine. Does this better appeal to you?

[mixxxbot]

Commented by: ferranpujolcamins
Date: 2018-03-28T23:14:09Z

---

Another way to do this is to make engine.makeConnection take no additional parameter and make it execute its callback as is, without binding it to any object.

Then bind the function to "this" in Object.prototype.makeConnection.

[mixxxbot]

Commented by: Be-ing
Date: 2018-03-28T23:25:29Z

---

That is quite similar to what I was alluding to about above. I think adding nonstandard functions to the prototype of language builtins like Object will be odd for developers who know JS well. But we can accomplish essentially the same thing with Components by changing the line in Component.prototype.connect:

this.connections[0] = engine.makeConnection(this.group, this.outKey, this.output);

to

this.connections[0] = engine.makeConnection(this.group, this.outKey, this.output.bind(this));

[mixxxbot]

Commented by: ferranpujolcamins
Date: 2018-04-19T12:56:06Z

---

The 2018-06-30 date is too tight for me to finish this. Unless feature freeze for 2.2 is postponed a week, I have to unassign this from me.

[mixxxbot]

Commented by: Be-ing
Date: 2018-04-19T13:11:18Z

---

This was already assigned to 2.3 anyway. I don't think there's any need to rush it for 2.2.

[mixxxbot]

Commented by: ferranpujolcamins
Date: 2018-04-19T18:22:43Z

---

Then I'm retaking it :)

[mixxxbot]

Commented by: Be-ing
Date: 2018-05-27T18:19:17Z

---

This post has some background on why QtScript has been deprecated:
http://article.gmane.org/gmane.comp.lib.qt.devel/4224

According to https://forum.qt.io/topic/52306/qt-5-5-qt-script-deprecated-what-is-replacement/2 QtScript will continue to be a part of Qt until at least Qt 6. In May 2017 it was announced that planning for Qt 6 would begin after Qt 5.11, which is the current stable version. However, I cannot find any information about Qt 6 more recent than 2017. So I think targeting this migration to QJSEngine for Mixxx 2.3 is a good idea to avoid the rug being pulled out from under us.

[mixxxbot]

Commented by: Be-ing
Date: 2018-05-27T19:44:10Z

---

Good news! Work is in progress to upgrade QJSEngine to support ECMAScript 2017: https://bugreports.qt.io/browse/QTBUG-47735?focusedCommentId=398077&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-398077

[mixxxbot]

Commented by: Be-ing
Date: 2018-07-07T18:07:04Z

---

ECMAScript 2017 support including arrow functions are targeted for Qt 5.12, which will be released in November 2018. So I anticipate distributions will be packaging it in spring 2019 (for Fedora, this would be Fedora 30). I propose we start working on migrating to QJSEngine now and try using Function.prototype.bind(this) in place of arrow functions. However, it may be a good idea to hold off releasing a new XML-less mapping system and rewriting Components for that until distributions are shipping Qt 5.12.

https://bugreports.qt.io/browse/QTBUG-47735?focusedCommentId=410092&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-410092

[mixxxbot]

Commented by: Be-ing
Date: 2021-11-13T22:56:32Z

---

#2682

[mixxxbot]

Issue closed with status Fix Committed.