Fix/gh12369 beginTimer callback syntax

[Swiftb0y]

Based on #12400. Alternative to #12396. Fixes #12384. Fixes #12369 apart from #12369 (comment).

@ywwg can you take a quick look at the changes to the Reloop-Beatpad script? I did some refactorings that seemed sensible, but you probably know the code better than me.

I'd highly appreciate if anyone that still has access to any of these controllers can give these changes a quick test and see if they broke anything in their respective mappings.

[Swiftb0y]

yikes, still suffering from eslint issues, though this time they're valid and only related to the actually changed lines. I think we should ignore those for now too since some of them just require too many changes (eg renaming an entire controller because its named DeFOrmEd_SnakE_CAsE)...

[ronso0]

Great. Do you mind sharing the regex(es) you used?

[Swiftb0y]

I wouldn't mind but I don't have any. I went through it manually. My initial regex attempts failed, so the manual route was not only faster but also gave me more confidence that I'm not accidentally breaking anything.

[ywwg]

@ywwg can you take a quick look at the changes to the Reloop-Beatpad script? I did some refactorings that seemed sensible, but you probably know the code better than me.

hmm I don't actually know that code. I used to maintain the Hercules DJControls so I know those better and those look fine.

[ywwg]

at first glance I don't see anything risky / weird. I will test with my S3 and dig out my VCI400. My old hercules controllers no longer work (they required kernel modules that no longer build)

[Swiftb0y]

hmm I don't actually know that code.

Whoops, sorry. Yeah didn't inspect the blame close enough. The commit that pointed me to you was just d99d55a, should've double checked more closely.

[JoergAtGithub]

The code change itself is as far as I can see correct ES7 code, that should work.
But unfortunatly, we are not only in the JavaScript domain here. We evaluate the code from the C++ domain and I doubt, that an arrow function can resolves 'this' correct.
I think we should not merge this code unless we have a working unit test for each of these pattern:
-beginTimer with arrow-function
-beginTimer with external function name
-beginTimer with function() {}.bind(this)
-beginTimer with function body in exclamation marks

The last might fail, but I think it is fixable, because we pass it as string in

mixxx/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp

Lines 472 to 473 in dabca58

	timerCallback = m_pScriptEngineLegacy->jsEngine()->evaluate(
	QStringLiteral("()=>%1").arg(timerCallback.toString()));

anyway.

[JoergAtGithub]

BTW: I wonder if this this bug is related: https://doc.qt.io/qt-6/qtqml-cppintegration-exposecppattributes.html#c-methods-and-the-this-object

[Swiftb0y]

But unfortunatly, we are not only in the JavaScript domain here. We evaluate the code from the C++ domain and I doubt, that an arrow function can resolves 'this' correct.

Can you elaborate? I don't understand your concern.

I think we should not merge this code unless we have a working unit test for each of these pattern:

I'll see how feasible it is to implement those. I don't know how testable our JS Api is.

BTW: I wonder if this this bug is related:

Yes, but its QML- and Qt6-only and requires an even bigger API change, so I ruled it out as a potential solution.

[JoergAtGithub]

But unfortunatly, we are not only in the JavaScript domain here. We evaluate the code from the C++ domain and I doubt, that an arrow function can resolves 'this' correct.

Can you elaborate? I don't understand your concern.

I have some code that looks like this:

class MyClass {
    constructor() {
         this.myVar = 1;
    }
    buttonHandler(field) {
         this.shiftPressedTimer = engine.beginTimer(200, ()=> {
              this.myVar = 0;
        }, true);
    }
}

and the timer code never sets the member var of the class.

But what works is:

class MyClass {
    constructor() {
         this.myVar = 1;
    }
    buttonHandler(field) {
         this.shiftPressedTimer = engine.beginTimer(200, function() {
              this.myVar = 0;
        }.bind(this), true);
    }
}

This lead me think, that the arrow function doesn't get the relation to it's parent JS class.

But honestly I'm not sure about this.

[Swiftb0y]

Is this a hypothetical concern or is this an issue you experience? Because if so, its quite serious and needs investigation. For the four cases you mentioned, I'm confident (assuming there is no blatant bug in QJSEngine) that all but the last case (where we evaluate some code given as a string (I assume you meant quotation marks?)) should work flawlessly. Only the last case breaks in case the string contains this literally, but I think that's unlikely for most out-of-tree code and there is also nothing we could do to fix this from our side.

[JoergAtGithub]

Is this a hypothetical concern or is this an issue you experience? Because if so, its quite serious and needs investigation.

The example I posted above is taken from a real mapping, that doesn't work with arrow function.

For the four cases you mentioned, I'm confident (assuming there is no blatant bug in QJSEngine) that all but the last case (where we evaluate some code given as a string) should work flawlessly. Only the last case breaks in case the string contains this literally, but I think that's unlikely for most out-of-tree code and there is also nothing we could do to fix this from our side.

Maybe I'm thinking to simple, but I thought removing exclamation marks around a code snippet to evaluate, should be an easy regex before

mixxx/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp

Lines 472 to 473 in dabca58

	timerCallback = m_pScriptEngineLegacy->jsEngine()->evaluate(
	QStringLiteral("()=>%1").arg(timerCallback.toString()));

But maybe there are side effects.

[Swiftb0y]

The example I posted above is taken from a real mapping, that doesn't work with arrow function.

which one exactly? I'll look into it.

Maybe I'm thinking to simple, but I thought removing exclamation marks around a code snippet to evaluate, should be an easy regex before

Yes, of course the most robust solution would be to not pass as a string. That's why this PR eliminates that for all mappings we have. But we can't do anything about mappings that have not been contributed. The arrow function on the C++ side is there because otherwise the code was evaluated immediately instead of after the timer, which is obviously a bug.

[JoergAtGithub]

The example I posted above is taken from a real mapping, that doesn't work with arrow function.

which one exactly? I'll look into it.

A version of my Z2 mapping under development:

mixxx/res/controllers/Traktor-Kontrol-Z2-hid-scripts.js

Lines 1396 to 1405 in 8796b3c

	this.shiftPressedTimer = engine.beginTimer(200, () => {
	// Reset sync button timer state if active
	if (this.shiftPressedTimer !== 0) {
	this.shiftPressedTimer = 0;
	}
	// Change display values to beatloopsize
	this.displayLoopCount("[Channel1]", false);
	this.displayLoopCount("[Channel2]", true);
	console.log("TraktorZ2: shift unlocked");
	}, true);

[Swiftb0y]

I don't have access to a Z2, so I can't test this in context of your mapping. I created a little QJSEngine playground outside of Qt, but I can't reproduce the issue there. I'll try reproducing it in Mixxx.

[Swiftb0y]

Can't even reproduce within mixxx with a test script. Please double check your mapping then. I don't know what the culprit could be.

[Swiftb0y]

try something like

const that = this;
engine.beginTimer(100, () => {console.log(this === that);}, true);

This prints true on my system.

[ywwg]

this is hard to test on the Traktor S3 because a bunch of other controlobjects are broken, some of which should be fixed by #12208 but others need to be fixed too.

[ywwg]

(#12409

[Swiftb0y]

Testing the mappings is not a requirement IMO. the only thing that worries me slightly is the issue reported by @JoergAtGithub previously. Speaking of which, if you can still reproduce the issue, what Qt version is this on? I'm compiling with 5.15.11, maybe yours is older?

[ywwg]

I'll see how feasible it is to implement those. I don't know how testable our JS Api is.

a long time ago I wrote a test framework for controllers and it worked fairly nicely. You can find those changes in this PR: https://github.com/mixxxdj/mixxx/pull/3074/files

I don't think we can include this in 2.4 but it might be worth adding for the future

[Swiftb0y]

Yup, I remember that PR, but I think it would make more sense to try to integrate Qt Quick test into our current test runner instead (which would allow us to test Qt quick and by extension Qml and Js code on its own).

[JoergAtGithub]

I know that we can't test these mappings in practice. But AFAIK we have several unit tests for the scripting engine API, but not for beginTimer.
Why can't we add some simple unit tests for beginTimer in https://github.com/mixxxdj/mixxx/blob/main/src/test/controllerscriptenginelegacy_test.cpp to ensure that the code works on all platforms/Qt versions?
I've really worry, that this untested mass modification might not work in some environment.
Please don't understand me wrong, I appreciate the work in this PR, I just have a bad feeling to merge this without further prove.

[Swiftb0y]

I agree, your concerns are valid. I'm quite short on time due to university, so if you want to do me a favor and get this merged, I would really appreciate if you could take care of the unit test implementation.

[ywwg]

This is not the first time we have to choose between "PR without test" or "No PR" -- if this were a merge to main then I'd feel better about just getting it in.

Lacking a test, what I'd prefer to do is only fix those mappings that we know have problems, and save the mass search-and-replace for main. That way we have less risk of breaking the imminent release.

[ronso0]

We could also only fix those mappings where the pattern matches the mappings that were reported to be broken (and fixed by this), namely the DDJ-SB3 and Mixtrack Pro 3, #12384 and #12369, and merge the rest to main.
Didn't check what would be left then x)

[JoergAtGithub]

I created testcases for engine.beginTimer myself in #12437 and all tested code pattern work as intended.
o.t.: I couldn't reproduce the issue, that I see with my Z2 mapping with these testcase. So there might still be a latent error in the code of the beginTimer implementation.

[JoergAtGithub]

Thank you!

[Swiftb0y]

Thank you.