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.

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

Add [Main] to the list of valid groups for HID. #12102

Merged
merged 3 commits into from Oct 17, 2023
Merged

Conversation

ywwg
Copy link
Member

@ywwg ywwg commented Oct 13, 2023

Fixes issues with calls to resolveGroup returning undefined

Fixes issues with calls to resolveGroup returning undefined
@@ -1252,6 +1252,7 @@ class HIDController {
"[Sampler6]",
"[Sampler7]",
"[Sampler8]",
"[Main]",
Copy link
Member

Choose a reason for hiding this comment

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

what group is this again? we also need to add [App] IIRC.

Copy link
Member Author

Choose a reason for hiding this comment

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

There seem to have been recent changes that moved, say, vu_meter_l to the "[Main]" group. This list needs to be kept up to date or HID controllers stop working.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added [App]

@JoergAtGithub
Copy link
Member

I think we have also more valid control group names:

  • [Skin]
  • [Sampler9...64]

@Holzhaus Could you have a look, if this list is complete?

@ywwg
Copy link
Member Author

ywwg commented Oct 16, 2023

I wonder if we can get rid of this list completely. it's only used in two places:

1: inside resolveGroup to decide if the group being resolved is already known. But all this function does is resolve "deck1" and "deck2" to the appropriate channel. As written, otherwise it returns the group as-is. So we could just replace the whole function with a test for deck1 and deck2. There is an indirect debugging effect that it returns undefined if the group is unknown, but in practice any typoed group name will be caught elsewhere with a missed engine connection or otherwise.

2: inside getActiveFieldGroup, where it is essentially doing the exact same thing -- calling resolvegroup.

So I am going to get rid of this list and see what happens

@JoergAtGithub
Copy link
Member

Code LGTM! Did vou check that no mapping uses this public list directly?

@daschuer
Copy link
Member

This seems not the case. LGTM.

@daschuer daschuer merged commit 5ca1c0b into mixxxdj:2.4 Oct 17, 2023
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants