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

midi-components: Fix undefined CO group on EffectUnit initialization #2948

Merged
merged 3 commits into from
Aug 16, 2020
Merged

midi-components: Fix undefined CO group on EffectUnit initialization #2948

merged 3 commits into from
Aug 16, 2020

Conversation

Holzhaus
Copy link
Member

Before, the code caused a warning like this for each EffectUnit on
script load:

Warning [Controller]: ControlDoublePrivate::getControl returning NULL for ( "" , "controller_input_active" )

This was caused by PR #2322. The code tries to use the component's group
in setCurrentUnit even though it's not set until later in the code.

@Holzhaus
midi-components: Fix undefined CO group on EffectUnit initialization
21cafe1 
Before, the code caused a warning like this for each EffectUnit on
script load:

    Warning [Controller]: ControlDoublePrivate::getControl returning NULL for ( "" , "controller_input_active" )

This was caused by PR mixxxdj#2322. The code tries to use the component's group
in `setCurrentUnit` even though it's not set until later in the code.
@Holzhaus Holzhaus added this to the 2.3.0 milestone Jul 17, 2020
@Holzhaus Holzhaus requested a review from Be-ing July 17, 2020 10:04
@Holzhaus Holzhaus added this to In progress in 2.3 release via automation Jul 17, 2020
Be-ing
Be-ing reviewed Jul 17, 2020
View reviewed changes
res/controllers/midi-components-0.0.js
@@ -818,21 +818,20 @@
this.setCurrentUnit(this.unitNumbers[index]);
};

if (unitNumbers !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you remove this check?

Copy link
Member Author

@Holzhaus Holzhaus Jul 17, 2020 •
edited

Choose a reason for hiding this comment

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

Because it's superfluous and insufficient. The warning should be printed if unitNumbers is neither an array or a number. This is the case now. Before, the warning was only printed if it was undefined, but not if it was null or an object.

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that QScriptEngine stops executing silently when trying to do anything with an undefined variable besides check it is undefined.

Copy link
Member Author

Choose a reason for hiding this comment

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

Really? That sounds like an absolutely critical bug... Is that fixed with QJSEngine?

Copy link
Contributor

Choose a reason for hiding this comment

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

This bug has always been there...

Be-ing
Be-ing reviewed Jul 17, 2020
View reviewed changes
res/controllers/midi-components-0.0.js
@@ -818,9 +818,9 @@
this.setCurrentUnit(this.unitNumbers[index]);
};

if (Array.isArray(unitNumbers)) {
if (unitNumbers !== undefined && Array.isArray(unitNumbers)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this.unitNumbers is still accessed below. How about checking unitNumbers === undefined once before this if/else block?

Copy link
Member Author

@Holzhaus Holzhaus Jul 17, 2020 •
edited

Choose a reason for hiding this comment

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

... But it returns in line 826, before it's accessed?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Be-ing ping

@Holzhaus
Copy link
Member Author

Holzhaus commented Aug 2, 2020

Merge?

@Holzhaus Holzhaus moved this from In progress to In Review in 2.3 release Aug 5, 2020
@Holzhaus
Copy link
Member Author

Holzhaus commented Aug 6, 2020

@Be-ing ping

@Holzhaus
Copy link
Member Author

pong

Swiftb0y
Swiftb0y approved these changes Aug 13, 2020
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.

Seems like Be is very busy atm so I took the liberties to review instead.
The changes seem reasonable apart from the unitNumbers check.
I didn't test whether this PR actually fixes the issue reported, but at least it doesn't seem to break anything else.

res/controllers/midi-components-0.0.js
// These must be unique to each instance,
// so they cannot be in the prototype.
this.deckNumbers = deckNumbers;
} else if (deckNumbers !== undefined && typeof deckNumbers === "number" &&
Copy link
Member

Choose a reason for hiding this comment

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

Can we skip checking deckNumbers !== undefined twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

You have an idea how? I introduced that check in 6f0c690 because according to Be the QtScriptEngine behaves weird if it's undefined: https://github.com/mixxxdj/mixxx/pull/2948/files/21cafe18eba551ddb970299fef40458b6e2adb40#diff-9bedde251ff7d14d89b40d05168275e4

Copy link
Member

Choose a reason for hiding this comment

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

I was just thinking about extracting that into its own check like there was before.

if (deckNumbers !== undefined) {
  if (Array.isArray(deckNumbers)) { ... }
  else if (typeof deckNumbers === "number" && ...) {...}
  }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

And in case deckNumbers is not undefined and neither an array nor a number?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right, that would result in more duplicated code. You are right, the code is fine as-is.

res/controllers/midi-components-0.0.js
}
if (unitNumbers !== undefined && Array.isArray(unitNumbers)) {
this.unitNumbers = unitNumbers;
} else if (unitNumbers !== undefined && typeof unitNumbers === "number" &&
Copy link
Member

Choose a reason for hiding this comment

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

also double unitNumbers check again

@Holzhaus
Copy link
Member Author

OK, can we merge then?

@uklotzde
Copy link
Contributor

uklotzde commented Aug 16, 2020 •
edited

No need to wait any longer. @Swiftb0y already approved the PR.

LGTM

@uklotzde uklotzde merged commit 35342d7 into mixxxdj:2.3 Aug 16, 2020
2.3 release automation moved this from In Review to Done Aug 16, 2020
@Holzhaus Holzhaus mentioned this pull request Aug 16, 2020
Merged
@mixxxbot mixxxbot mentioned this pull request Aug 23, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Be-ing Be-ing Be-ing left review comments

@Swiftb0y Swiftb0y Swiftb0y approved these changes

Assignees
No one assigned
Labels
controller mappings minor bug
Projects
No open projects
2.3 release
  
Done
Milestone
2.3.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Holzhaus @uklotzde @Be-ing @Swiftb0y