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

ControllerEngine: Add test that invalid CO keys do not cause debug assertion #2947

Merged
merged 1 commit into from Aug 16, 2020
Merged

ControllerEngine: Add test that invalid CO keys do not cause debug assertion #2947

merged 1 commit into from Aug 16, 2020

Conversation

Holzhaus
Copy link
Member

No description provided.

@Holzhaus Holzhaus requested a review from daschuer July 16, 2020 11:28
@Holzhaus Holzhaus added this to In progress in 2.3 release via automation Jul 16, 2020
@Holzhaus Holzhaus added this to In progress in 2.4 release via automation Jul 16, 2020
@Holzhaus Holzhaus removed this from In progress in 2.3 release Jul 16, 2020
@Holzhaus Holzhaus added this to the 2.4.0 milestone Jul 16, 2020
@Be-ing
Copy link
Contributor

Be-ing commented Jul 16, 2020

I don't understand. What problem is this fixing? Controller scripts shouldn't be connecting to non-existent ControlObjects.

@Holzhaus
Copy link
Member Author

They shouldn't, but third Party controller mappings are considered "user data" and therefore shouldn't cause a debug assertion IMHO. Instead, we can throw a warning or (or a JS exception).

@Be-ing
Copy link
Contributor

Be-ing commented Jul 16, 2020

Okay, let's throw a JS exception.

@Be-ing
Copy link
Contributor

Be-ing commented Jul 16, 2020

On second thought, just log a warning. Throwing an exception could break old scripts.

@Holzhaus
Copy link
Member Author

Agreed, but let's throw a JS exception in the new module-based scripting engine.

@Be-ing
Copy link
Contributor

Be-ing commented Jul 16, 2020

Okay, then we need to merge #2920.

@Holzhaus Holzhaus moved this from In progress to Needs review in 2.4 release Aug 5, 2020
@uklotzde
Copy link
Contributor

uklotzde commented Aug 6, 2020

LGTM @Be-ing @daschuer Ready to merge?

@Holzhaus
Copy link
Member Author

Merge?

@Holzhaus Holzhaus mentioned this pull request Aug 14, 2020
Merged
@Holzhaus Holzhaus changed the title ControllerEngine: Fix DEBUG_ASSERT if CO with empty key is requested ControllerEngine: Add test that invalid CO keys do not cause debug assertion Aug 16, 2020
@Holzhaus
Copy link
Member Author

I rebased on master and noticed that the issue has already been fixed while this PR was open. I removed the fixes from this branch, but let's merge the test that checks that requesting an empty key does not cause a debug assertion and avoid breakage in the future.

@uklotzde
Copy link
Contributor

@daschuer Merging although not formally approved. Rejection does not apply any longer.

@uklotzde
Copy link
Contributor

LGTM

@uklotzde uklotzde merged commit c3d2ecf into mixxxdj:master Aug 16, 2020
2.4 release automation moved this from Needs review to Done Aug 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daschuer daschuer daschuer requested changes

@uklotzde uklotzde uklotzde approved these changes

Assignees
No one assigned
Labels
controller mappings minor bug
Projects
2.4 release
  
Done
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.

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