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

Reloop Mixage MK1, MK2, Controller Edition mappings for Mixxx 2.4 #12296

Merged
merged 49 commits into from
Oct 21, 2024

Conversation

HorstBaerbel
Copy link
Contributor

@HorstBaerbel HorstBaerbel commented Nov 12, 2023

Much improved and cleaned up version of PR #10892 by @gqzomer (Gersom Zomer) using Mixxx 2.4 functions like effect presets. Documentation in a new manual PR here #592.
Please consider this for inclusion in Mixxx 2.4. If you have improvements or need adjustments we're happy to improve the PR.

Bim Overbohm and others added 30 commits September 9, 2022 17:55
…able. Map PAN button. Refactor rotatry button presses
ronso0
ronso0 previously requested changes Mar 8, 2024
res/controllers/Reloop-Mixage.scripts.js Outdated Show resolved Hide resolved
@ronso0
Copy link
Member

ronso0 commented Mar 8, 2024

@HorstBaerbel Did you alread sign the Mixxx Contributor Agreement?
If yes, which alias did you pick?

@ronso0
Copy link
Member

ronso0 commented Mar 8, 2024

I just made another attempt to review this. I only did a brief check, still no js expert.
Would appreciate some help by @JoergAtGithub or @Swiftb0y

@HorstBaerbel
Copy link
Contributor Author

@HorstBaerbel Did you alread sign the Mixxx Contributor Agreement? If yes, which alias did you pick?

Signed as HorstBaerbel 👍

@gqzomer
Copy link
Contributor

gqzomer commented Mar 11, 2024

@ronso0 Is it enough for the owner of the pull request to sign the contributor agreement? Or should I also sign the agreement as a contributor to this mapping?

@ronso0
Copy link
Member

ronso0 commented Mar 11, 2024

This PR contains many commits of yours so it would be very welcome if sign it as well, thank you!

@gqzomer
Copy link
Contributor

gqzomer commented Mar 11, 2024

This PR contains many commits of yours so it would be very welcome if sign it as well, thank you!

Signed as gqzomer

@ronso0 ronso0 added this to the 2.4.2 milestone Apr 13, 2024
@HorstBaerbel HorstBaerbel requested a review from ronso0 April 13, 2024 16:35
@gqzomer
Copy link
Contributor

gqzomer commented Sep 5, 2024

The requested changes have been applied a while back. @ronso0 can you maybe resolve your request for changes so this pull request correctly reflects it is ready to be merged?

@ronso0
Copy link
Member

ronso0 commented Sep 5, 2024

Thanks @gqzomer for the reminder.
I dismissed my review (won't approve though since I'm still no js expert)

I'll leave some more notes.

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.

I'm not a fan of this style of review anyways and since I don't have the controller I also can't really make any suggestion from a mapping user perspective. If this mapping leveraged ComponentsJS I could make better suggestions but I understand if thats too much to ask. I can make a bunch of code-style nitpicks but that wouldn't make it more readable either so I guess LGTM?

@daschuer
Copy link
Member

Merge?

@gqzomer
Copy link
Contributor

gqzomer commented Sep 16, 2024

@Swiftb0y I have looked into ComponentsJS in the past for this controller. But as I understood it Component style mappings are mostly beneficial for controllers that use a different channel for each deck. Sadly this controller uses only one channel and opts for different midi inputs for the left and right deck. Either way the starting point for this pull request was a non-ComponentsJS mapping submitted in an earlier pull request by @HorstBaerbel

@Swiftb0y
Copy link
Member

But as I understood it Component style mappings are mostly beneficial for controllers that use a different channel for each deck.

No that doesn't particularly matter. ComponentJS is useful for encapsulating state which applies to almost every mapping (this one included as you can see by the many global state-tracking variables at the top). Anyways, doesn't particularly matter now.

@daschuer
Copy link
Member

@Swiftb0y merge?

@Swiftb0y
Copy link
Member

waiting for @ronso0 to give his LGTM on the manual PR.

@ronso0 ronso0 merged commit 04ba685 into mixxxdj:2.4 Oct 21, 2024
14 checks passed
@ronso0
Copy link
Member

ronso0 commented Oct 21, 2024

Thanks to everyone involved!

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.

6 participants