Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix: Allow midino 0 in MidiController::makeInputHandler #14266

Open
wants to merge 1 commit into
base: 2.5
Choose a base branch
from
Open

Fix: Allow midino 0 in MidiController::makeInputHandler #14266

wants to merge 1 commit into from
+11 −4

Conversation

git-developer
Copy link
Contributor

Fix for #14265

daschuer
daschuer reviewed Jan 31, 2025
View reviewed changes
src/controllers/midi/midicontroller.cpp
@@ -622,7 +622,7 @@ QJSValue MidiController::makeInputHandler(int status, int midino, const QJSValue
return QJSValue();
}

if (status <= 0 || midino <= 0) {
if (status <= 0 || midino < 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both can only be a unsigned char. I wonder why the function even allows int. What would happen if one passes -1 form the js code if this would be unsigned char?
Can't status be 0? I think from the Mixxx perspective it is allowed and we are not the midi police.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't comment on that due to my limited knowledge. If you're right, the check would be obsolete. Maybe @christophehenry can help out.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm… I don't remember. I think it was because passing -1 to unsigned char doesn't raise an exception in JS engine but crashes the app which is not something we want.

Swiftb0y
Swiftb0y approved these changes Jan 31, 2025
View reviewed changes
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Pesky off-by-one errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christophehenry christophehenry christophehenry left review comments

@daschuer daschuer daschuer left review comments

@Swiftb0y Swiftb0y Swiftb0y approved these changes

Assignees
No one assigned
Labels
code quality controller backend
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@git-developer @daschuer @Swiftb0y @christophehenry