Don't override XML mapping when using `makeInputHandler` #13089

christophehenry · 2024-04-11T10:09:11Z

Follow up of #12781. This PR is designed to prevent overriting an existing XML mapping when when using makeInputHandler.

christophehenry · 2024-04-11T10:10:06Z

src/controllers/midi/midicontroller.cpp

@@ -631,6 +631,12 @@ QJSValue MidiController::makeInputHandler(int status, int midino, const QJSValue
        return QJSValue();
    }

+    if (m_pMapping->getInputMappings().contains(status)) {


This just check if an existing mapping exists. Should it perform a more thourough search to check if existing binding is an XML type?

I'd argue yes. I think its valid that JS would like to bind different handlers to the same message. It's also possible in the XML manually, so the limitation for pure JS seems arbitrary.

Before, all bindings are from XML right? So the question is regarding to allow a second js binding. Is that your point? Do we have a use case for it? I think in such cases you could just put both features onto one callback.

I wonder why midino is not relevant here?

Oh that's a mistake. Fixed.

christophehenry · 2024-04-11T10:11:52Z

src/controllers/midi/midicontroller.cpp

@@ -631,6 +631,12 @@ QJSValue MidiController::makeInputHandler(int status, int midino, const QJSValue
        return QJSValue();
    }

+    if (m_pMapping->getInputMappings().contains(status)) {
+        qDebug(m_logBase).nospace() << "Ignoring anonymous JS function for status "
+                                    << status << " because a previous binding exists";


Here, I just log a message. I was thinking maybe also log the script line. getScriptEngine()->throwJSError() indicates a line in the script. Is there a similar method to just log a warning?

not that I'd know of. I think its easy enough to identify if you also include the midino in the warning. Alternatively consider using ControllerScriptEngineBase::logOrThrowError.

My guess is:
getScriptEngine()->currentStackFrame->lineNumber()

that doesn't exist / is private, or at least it seems to be undocumented. where is this from?

I have skimmed through the Qt source.

I can't find getScriptEngine()->currentStackFrame.

christophehenry · 2024-04-11T10:12:30Z

I didn't write any tests here. Should I?

daschuer · 2024-04-11T16:51:36Z

I didn't write any tests here. Should I?

At least don't spend to much time on that. ;-)

daschuer

Thank you. Here some comments.

daschuer · 2024-04-11T16:30:52Z

src/controllers/midi/midicontroller.cpp

@@ -631,6 +631,12 @@ QJSValue MidiController::makeInputHandler(int status, int midino, const QJSValue
        return QJSValue();
    }

+    if (m_pMapping->getInputMappings().contains(status)) {


Before, all bindings are from XML right? So the question is regarding to allow a second js binding. Is that your point? Do we have a use case for it? I think in such cases you could just put both features onto one callback.

daschuer · 2024-04-11T16:47:58Z

src/controllers/midi/midicontroller.cpp

@@ -631,6 +631,12 @@ QJSValue MidiController::makeInputHandler(int status, int midino, const QJSValue
        return QJSValue();
    }

+    if (m_pMapping->getInputMappings().contains(status)) {
+        qDebug(m_logBase).nospace() << "Ignoring anonymous JS function for status "
+                                    << status << " because a previous binding exists";


My guess is:
getScriptEngine()->currentStackFrame->lineNumber()

daschuer · 2024-04-11T16:57:37Z

src/controllers/midi/midicontroller.cpp

@@ -631,6 +631,12 @@ QJSValue MidiController::makeInputHandler(int status, int midino, const QJSValue
        return QJSValue();
    }

+    if (m_pMapping->getInputMappings().contains(status)) {


I wonder why midino is not relevant here?

Swiftb0y

sorry, forgot to submit my review earlier

Swiftb0y · 2024-04-11T12:28:45Z

src/controllers/midi/midicontroller.cpp

@@ -631,6 +631,12 @@ QJSValue MidiController::makeInputHandler(int status, int midino, const QJSValue
        return QJSValue();
    }

+    if (m_pMapping->getInputMappings().contains(status)) {


I'd argue yes. I think its valid that JS would like to bind different handlers to the same message. It's also possible in the XML manually, so the limitation for pure JS seems arbitrary.

Swiftb0y · 2024-04-11T12:33:32Z

src/controllers/midi/midicontroller.cpp

@@ -631,6 +631,12 @@ QJSValue MidiController::makeInputHandler(int status, int midino, const QJSValue
        return QJSValue();
    }

+    if (m_pMapping->getInputMappings().contains(status)) {
+        qDebug(m_logBase).nospace() << "Ignoring anonymous JS function for status "
+                                    << status << " because a previous binding exists";


not that I'd know of. I think its easy enough to identify if you also include the midino in the warning. Alternatively consider using ControllerScriptEngineBase::logOrThrowError.

christophehenry · 2024-04-23T09:57:21Z

I wonder why midino is not relevant here?

You're right. That's fixed.

Swiftb0y

lgtm otherwise

src/controllers/midi/midicontroller.cpp

Swiftb0y · 2024-04-23T10:35:28Z

src/controllers/midi/midicontroller.cpp

+        qCWarning(m_logBase) << QString(
+                "Ignoring anonymous JS function for status=%1,midino=%2 "
+                "because a previous XML binding exists")
+                                        .arg(status)
+                                        .arg(midino);


Suggested change

qCWarning(m_logBase) << QString(

"Ignoring anonymous JS function for status=%1,midino=%2 "

"because a previous XML binding exists")

.arg(status)

.arg(midino);

qCWarning(m_logBase) << QStringLiteral(

"Ignoring anonymous JS function for status=%1,midino=%2 "

"because a previous XML binding exists")

.arg(status, midino);

Erf, no, that works only for objects that can be casted to QString. When used with (status, midino), C++ select this one.

I can do:

Suggested change

qCWarning(m_logBase) << QString(

"Ignoring anonymous JS function for status=%1,midino=%2 "

"because a previous XML binding exists")

.arg(status)

.arg(midino);

qCWarning(m_logBase) << QString(

"Ignoring anonymous JS function for status=%1,midino=%2 "

"because a previous XML binding exists")

.arg(QString::number(status), QString::number(midino));

whoops, good point. yeah, then the double arg is probably fine. suboptimal Qt Api. Still, please wrap the string literal in a QStringLiteral

Swiftb0y

LGTM. wdyt @daschuer

daschuer

LGTM, Thank you.

christophehenry commented Apr 11, 2024

View reviewed changes

daschuer requested changes Apr 11, 2024

View reviewed changes

Swiftb0y reviewed Apr 11, 2024

View reviewed changes

christophehenry force-pushed the dont-override-XML-mapping branch from 43893b7 to c158455 Compare April 23, 2024 09:55

Swiftb0y reviewed Apr 23, 2024

View reviewed changes

christophehenry force-pushed the dont-override-XML-mapping branch from c158455 to 8cb1014 Compare April 29, 2024 13:32

Don't override XML mapping when using makeInputHandler

d070bae

christophehenry force-pushed the dont-override-XML-mapping branch from 8cb1014 to d070bae Compare April 29, 2024 13:33

JoergAtGithub added this to the 2.5-beta milestone Apr 29, 2024

JoergAtGithub added midi controller backend labels Apr 29, 2024

Swiftb0y approved these changes Apr 29, 2024

View reviewed changes

daschuer approved these changes May 1, 2024

View reviewed changes

daschuer merged commit 6170466 into mixxxdj:main May 1, 2024
13 checks passed

christophehenry deleted the dont-override-XML-mapping branch May 29, 2024 08:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't override XML mapping when using `makeInputHandler` #13089

Don't override XML mapping when using `makeInputHandler` #13089

christophehenry commented Apr 11, 2024

christophehenry Apr 11, 2024

Swiftb0y Apr 11, 2024

daschuer Apr 11, 2024

daschuer Apr 11, 2024

christophehenry Apr 29, 2024

christophehenry Apr 11, 2024

Swiftb0y Apr 11, 2024

daschuer Apr 11, 2024

Swiftb0y Apr 11, 2024 •

edited

Loading

daschuer Apr 11, 2024

christophehenry Apr 23, 2024

christophehenry commented Apr 11, 2024

daschuer commented Apr 11, 2024

daschuer left a comment

daschuer Apr 11, 2024

daschuer Apr 11, 2024

daschuer Apr 11, 2024

Swiftb0y left a comment

Swiftb0y Apr 11, 2024

Swiftb0y Apr 11, 2024

christophehenry commented Apr 23, 2024

Swiftb0y left a comment

Swiftb0y Apr 23, 2024

christophehenry Apr 23, 2024

christophehenry Apr 23, 2024

Swiftb0y Apr 23, 2024

christophehenry Apr 29, 2024

Swiftb0y left a comment

daschuer left a comment

Don't override XML mapping when using makeInputHandler #13089

Don't override XML mapping when using makeInputHandler #13089

Conversation

christophehenry commented Apr 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophehenry commented Apr 11, 2024

daschuer commented Apr 11, 2024

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christophehenry commented Apr 23, 2024

Swiftb0y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Swiftb0y left a comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

Don't override XML mapping when using `makeInputHandler` #13089

Don't override XML mapping when using `makeInputHandler` #13089

Swiftb0y Apr 11, 2024 •

edited

Loading