Add Hercules DJControl Inpulse 500 mapping#14316
Conversation
|
Welcome at Mixxx! |
cr7pt0gr4ph7
left a comment
There was a problem hiding this comment.
A few small formatting nitpicks.
cr7pt0gr4ph7
left a comment
There was a problem hiding this comment.
A few actual code change suggestions as well as some more formatting nitpicks.
cr7pt0gr4ph7
left a comment
There was a problem hiding this comment.
I can't actually test this mapping as I don't have the associated hardware, though this seems to look good so far. The indentation should be fixed to only use spaces, though.
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
Co-authored-by: Lukas Waslowski <cr7pt0gr4ph7@gmail.com>
cr7pt0gr4ph7
left a comment
There was a problem hiding this comment.
Code looks good. I can't give any input on the problems brought up by @m0dB, though these should definitely be resolved before merging. I'll be happy to do a final round of review after these issues have been confirmed to be resolved, but I can't be of much help until then.
|
No problem @cr7pt0gr4ph7 just pushed (and verified as working) both volume and pitch faders with shift pressed (both weren't working when shift was pressed and the fix for both was the same and quick) |
cr7pt0gr4ph7
left a comment
There was a problem hiding this comment.
LGTM with respect to the code itself. Can't test the functionality as I don't have the hardware, so someone else should retest.
|
I will retest tomorrow. It would be great if we can also get this into 2.5.1. Once merged to main, could you create a PR for 2.5 as well? Same for the manual. |
|
A minor discrepancy between mapping and manual: in transport mode, slider range reset is pad 5, in the manual it's pad 8. Do you want me to correct the manual? (Including the svg) |
|
And another discrepancy: the values for Loop with shift are different than what the manual says. |
|
Assuming that in these cases the manual is wrong and the mapping is correct, everything works perfectly! Great job, @resetreboot, very much appreciated! IMO this can be merged but maybe we should first correct the manual? If you do the .rst, I will do the .svgs. I tried to use the mapping with 2.5 but it doesn't work. I am pretty sure an earlier version did work, so I am not sure if this is expected. Is this something you want to address? It would be great to have this mapping included with 2.5.1! |
|
Hm, I just noticed that I get a ton of warnings on startup: Details
warning [Controller] file:///Users/maarten/git/mixxx/res/controllers/lodash.mixxx.js:2397:19 Variable "isArray" is used before its declaration at 11284:9. |
|
That's strange, I'm on 2.5.0 and I've been rocking it no problem. About the discrepancies, yeah, I think we need to fix the manual. Trust the code. I will take a look along the week. THank you for the extra testing! |
|
Ok, I will try again with 2.5 branch tomorrow. The warning are with this branch too. You don’t get them? |
|
I will take a look tomorrow, I have a session coming so they should pop out if they are there. I'm about to head to bed here. |
|
And a final note, in the Setting page it still has the 1d postfix in the name, and only list DJ Phatso as the author. |
|
@m0dB I can confirm I have those messages about extra parameters. I will look at the docs of the Mixxx API to see how these callbacks have changed since 2.4 and adjust them so we have 0 warnings. Also, the lodash stuff is this code's fault? Because we aren't loading it anywhere. |
|
Fixed the warnings and the author data, and version name. |
|
I tested on main and 2.5 and all good! Do you want me to create the MR for 2.5? I have a branch ready. I see there is a eslint warning left. I see there is a comment to disable the check but apparently it doesn't work... No idea why. Can you try
? |
|
I have created the PR for 2.5 here #14491 In that PR I changed the line to: in order to satisfy pre-commit. |
|
I created a PR with final fixes for manual to match the mapping in resetreboot/manual#3 |
|
Okay, that should be it, also thanks @m0dB for the PR for the 2.5 branch and the fixes to the manual, much appreciated. |
Manual PR