MIDI4lights: Give beginTimer callbacks the anonymous function expression form

[geraldog]

Merged PR #11953 "fix(controllers): engine.beginTimer(ms, string) would evaluate the code immediately instead of after ms" requires anonymous functions expressions instead of string literals.

This should solve #12042

[Swiftb0y]

#11953 should've fixed this exact issue already though. If #11953 works correctly, this PR would just be noop. Do you think #11953 is broken?

[geraldog]

Hi @Swiftb0y and sorry for bringing you out of your time off.

I tested again and again with real fixture to make sure we understand better the problem and save some face. Still, it seems I need to test even more but let me explain.

I use a modified midi_for_light.vuMeter to interact with AudioPathType::Stem I have created in my fourstems_padj_2023 branch.

Without the proposed fix, here's the warning I get when I engage my Programmable AutoDJ with Stems capabilities:

controller.midithroughport0:warning [Controller] "passed a string to 'engine.beginTimer', please pass a function instead!"

And it doesn't work properly. I specifically lose the on-beat information!

[geraldog]

@Swiftb0y as not to waste your precious time I reproduced #12042 with Vanilla Mixxx (no weird customizations). I had a hunch the author of #12042 was using AutoDJ too like me, and lo and behold if you don't touch the crossfader but instead rely on AutoDJ to do crossfading to the next song for you, the bug happens and I get the same warning as with the strange customizations.

EDIT: I was able to reproduce the bug multiple times now, and sometimes I happen to lose Deck 1 instead of the opposite. I'm still not sure what exactly triggers the bug, I would bet on some timing issue, and I'm putting no pressure to merge this as is. It definitely deserves more investigation.

[Swiftb0y]

Without the proposed fix, here's the warning I get when I engage my Programmable AutoDJ with Stems capabilities:

Right, thats a warning that the mapping needs some update love. This PR fixes that warning, but the behavior should be identical due to #11953

I have no doubt that you are experiencing the issue you're describing (I have never looked into the midi4lights script myself so I'm not the right person for this anyways). The thing I don't understand is how this PR is supposed to fix #12042. Since as I said, the behavior should I identical (assuming #11953 works as intended).

So, are you confident that this PR actually fixes #12042? I'm unfortunately not able to confirm for myself.

[Swiftb0y]

I appreciate it highly btw that you are so understanding of my personal resource constraints. Thank you.

[geraldog]

I have no doubt that you are experiencing the issue you're describing (I have never looked into the midi4lights script myself so I'm not the right person for this anyways). The thing I don't understand is how this PR is supposed to fix #12042. Since as I said, the behavior should I identical (assuming #11953 works as intended).

So, are you confident that this PR actually fixes #12042? I'm unfortunately not able to confirm for myself.

No, I'm not confident at all. We need the author of #12042 to test the proposed fix, see if it solves his bug too.

[Holzhaus]

Thanks. Please check if we can pass the function directly instead of wrapping it in an anonymous function.

[geraldog]

Yes Jan it works normally by passing the function directly, except for the functions which need arguments of course, which I kept as anonymous function expressions.

[Holzhaus]

Yes Jan it works normally by passing the function directly, except for the functions which need arguments of course, which I kept as anonymous function expressions.

Good 👍

I think you didn't push your changes yet though.

[Holzhaus]

Code changes look good to make and make sense, even if they don't fix #12042 (the changes should make no difference, but I didn't test).

@geraldog it would be great if you could apply the proposed fixes from eslint to fix the pre-commit CI failure, too.

[geraldog]

Hi Jan, thanks for the review by the way, and thank you @ronso0 for updating with the correct title, my bad.

Code changes look good to make and make sense, even if they don't fix #12042 (the changes should make no difference, but I didn't test).

The changes do make a difference Jan, they don't go through that code path that Niko fixed inside src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp in #11953. Namely:

int ControllerScriptInterfaceLegacy::beginTimer(
        int intervalMillis, QJSValue timerCallback, bool oneShot) {
    if (timerCallback.isString()) {
        m_pScriptEngineLegacy->logOrThrowError(
                QStringLiteral("passed a string to 'engine.beginTimer', please "
                               "pass a function instead!"));
        // wrap the code in a function to make the evaluation lazy.
        // otherwise the code would be evaluated immediately instead of after
        // the timer which is obviously undesired and could also cause
        // issues when used recursively.
        timerCallback = m_pScriptEngineLegacy->jsEngine()->evaluate(
                QStringLiteral("()=>%1").arg(timerCallback.toString()));
    } else if (!timerCallback.isCallable()) {

@geraldog it would be great if you could apply the proposed fixes from eslint to fix the pre-commit CI failure, too.

I will look into it Jan, and I don't want to be rude that's why I held back on this comment I'm about to make.

Niko said the script needs more love, but I consider this an understatement. Quite frankly that script looks like one of my hacks, amazing but kind-of constructed in a rush, if you know what I mean.

[Swiftb0y]

welp, lgtm, eslint failure is expected. Still very likely won't solve #12042

[geraldog]

Thanks @Swiftb0y