-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(controllers): add new controller mapping for Numark NS6II #11075
base: 2.4
Are you sure you want to change the base?
Conversation
1f3330e
to
6f16bba
Compare
tests are failing because of the EffectUnits. I guess I'll have to redo them when I find the time. Works locally so far. |
This PR is marked as stale because it has been open 90 days with no activity. |
7872f2b
to
fccd316
Compare
This PR is marked as stale because it has been open 90 days with no activity. |
Ah yes, good old stale. I have had and used this controller for about 5 years now, one day I'll get around to polishing the mapping enough for merge. Still, a surface-level review in the meantime would be appreciated. |
This PR is marked as stale because it has been open 90 days with no activity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard to review without the device to test. but it is looking good overall. Also I'm sure that if you've been using it this long, you probably had a chance to polish most of the small bug lying around :)
Some comment cruft here and there that might be worth removing or documenting if useful for alternate behaviours (note that if the latter, you might want to consider the controller settings).
Did you have a chance to prepare the manual for that?
I'll try to give this another look and ping you when I think its ready. Thanks for the review. |
@acolombier please re-review ;) |
@@ -1,4 +1,6 @@ | |||
|
|||
type MidiInputHandler = (channel: number, control: number, value:number, status:number, group:string) => void; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't spot the place, where this type is defined in our API. Is this a mapping specific type, that doesn't belong to the API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its the type our input handlers are expected to be https://github.com/mixxxdj/mixxx/wiki/midi%20scripting#midi-input-handling-functions
friendly ping... ;) |
// grey could be an alternative as well as a backlight color. | ||
off: NS6II.USE_BUTTON_BACKLIGHT ? NS6II.PAD_COLORS.RED.DIMMER : NS6II.PAD_COLORS.OFF, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be worth adding a dynamic setting for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I'll add that as a follow up, but I wanted to keep this branch targetted at 2.4
}); | ||
this.scratch = new components.Button({ | ||
midi: [0x90 + channelOffset, 0x07], | ||
// shift: [0x90+channelOffset,0x46] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be removed as this is redundant with line 409/414?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically yes, but I would've preferred this to stay here instead of having it buried in the implementation.
this.parameterLeft = new components.Button({ | ||
midi: [0x90, 0x28], | ||
unshift: function() { | ||
// TODO change hotcue page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this outstanding? Or should it be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nah, its technically outstanding, but I really didn't want to bother implementing that because components.HotcueButton
is not setup for that.
this.parameterRight = new components.Button({ | ||
midi: [0x90, 0x29], | ||
unshift: function() { | ||
// TODO change hotcue page |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
88b88a0
to
38762b8
Compare
I had a thorough look at that |
well the build failure is fortunately unrelated and fixed with #13904. I'll investigate whats wrong with pre-commit. |
Otherwise the eslint stdout is only printed if eslint actually fails. In the case of warnings, we don't want pre-commit to fail, but we still want to bring attention to the users code that something is suboptimal. In order for that to work the output still needs to be shown even when the hook does not fail. Setting a hook to be `verbose` is the only way to make that happen as far as I can tell.
9810bbb
to
cf579f6
Compare
cf579f6
to
691831b
Compare
now depends on #13913 |
Manual still TODOmanual PR