DDJ-200: Listen to changes from mixxx #3793

geovannimp · 2021-04-17T22:49:04Z

Those updates listen to changes from mixxx interface to replicate at the controller

Holzhaus

Thanks, I left some comments.

Holzhaus · 2021-04-17T22:54:57Z

res/controllers/Pioneer-DDJ-200-scripts.js

@@ -36,6 +48,15 @@ DDJ200.init = function() {
    }, true);
 };

+DDJ200.listemHotcues = function(vgroup) {


"listen"

Also, this method is only called in a single location, so I don't think it's needed at all and can just be I lined there.

I was trying just to make the code cleaner, now is inside init

What is listemHotcues supposed to mean?

This was to listen Hotcues changes from mixxx interface and apply to the controller, but already removed the method.

Holzhaus · 2021-04-17T22:56:18Z

res/controllers/Pioneer-DDJ-200-scripts.js

+    DDJ200.switchHotcueLEDs(vDeckNo);
+};
+
+DDJ200.switchHotcueLEDs = function(vDeckNo) {


Same here, no need to put it a separate method if it's only used once. That just makes the code harder to follow.

I refactored this whole thing to switch only leds that actually changed

Be-ing · 2021-04-17T23:00:03Z

res/controllers/Pioneer-DDJ-200-scripts.js

+    for (var i = 1; i <= 8; i++) {
+        // run updateDeckLeds after every hotcue update
+        engine.makeConnection(vgroup, "hotcue_" + i + "_enabled", function(ch, vgroup) {
+            DDJ200.updateDeckLeds(vgroup);


This looks like it sends a MIDI message for every hotcue when only one of them updates? That's inefficient.

I refactored this whole thing to switch only leds that actually changed

Holzhaus · 2021-04-17T23:06:44Z

@fkbreitl @dan-giddins can one of you verify that this is working as intended?

geovannimp · 2021-04-18T01:42:31Z

I refactored most of the code based on the code review, let me know if there's anything else I can improve.

dan-giddins · 2021-04-18T16:12:48Z

@Holzhaus sorry no, I currently don't have access to my ddj-200 controller :(

@fkbreitl looks like its up to you to test this, if you can!

uklotzde · 2021-04-18T19:14:34Z

As a First-time contributor please also sign our contributor agreement and comment when done.

uklotzde · 2021-04-18T19:19:10Z

Nice to see that there is a demand for supporting those popular entry-level controllers in Mixxx, even though they come with Rekordbox. Thanks for contributing your improvements.

geovannimp · 2021-04-18T20:35:23Z

Agreement signed :)

geovannimp · 2021-04-18T20:40:43Z

I started using mixxx before having a controller and I'm a developer and Linux user, those are some reasons to love mixxx and what to contribute.

Holzhaus · 2021-04-18T20:49:43Z

res/controllers/Pioneer-DDJ-200-scripts.js

+                var vDeckNo = script.deckFromGroup(vgroup);
+                var d = (vDeckNo % 2) ? 0 : 1;           // d = deckNo - 1
+                DDJ200.switchPadLED(d, pad, ch);


Let's not use one-character variable names (except i/j for counters maybe) and go with something more meaningful:

Suggested change

var vDeckNo = script.deckFromGroup(vgroup);

var d = (vDeckNo % 2) ? 0 : 1; // d = deckNo - 1

DDJ200.switchPadLED(d, pad, ch);

var deckNo = script.deckFromGroup(vgroup);

var deckOffset = (deckNo - 1) % 2;

DDJ200.switchPadLED(deckOffset, pad, ch);

I know the rest of the script does that too, but let's at least fix it for new code :D

I don't like one-character variables too, but at same time is good to keep a pattern at the code. Don't you think would be better open a new PR fixing all one-character variables after this one?

I second @Holzhaus, and I think it should probably be done in this PR

Just the new code or a full refactor?

var d = (vDeckNo % 2) ? 0 : 1; // d = deckNo - 1
seems clear and short to me.

var deckOffset = (deckNo - 1) % 2;
is neither short nor clear.
It is less obvious that the value is either 0 or 1.
And it is not correct, because it is not an offset. I don't know what to call this, but for sure not offset.

var controllerDeckIndex = (vDeckNo % 2) ? 0 : 1;?
Case this represents the controller deck and an index starting from 0 to 1

geovannimp · 2021-04-19T03:34:32Z

Also, I'm working in a mapping that adds loop/effect mode to pads. But mixxx don't have buttons at the screen as rekordbox and virtual dj, so I replaced the Transition FX of the controller to toggle between those modes.

I'm not confident that this should be shipped with mixxx because no other program use Transition FX for that and I'm changing the button function, what can cause confusion to users.

I already posted my mapping at the forum to anyone interested, but I want to know what mixxx team think about it.

Sorry if this is the wrong place to have this conversation, I was going to post in the issues at github, but it is disabled and lauchpad is just for bugs.

dan-giddins · 2021-04-19T13:08:11Z

res/controllers/Pioneer-DDJ-200-scripts.js

+                var vDeckNo = script.deckFromGroup(vgroup);
+                var d = (vDeckNo % 2) ? 0 : 1;           // d = deckNo - 1
+                DDJ200.switchPadLED(d, pad, ch);


I second @Holzhaus, and I think it should probably be done in this PR

dan-giddins · 2021-04-19T13:31:32Z

Also, I'm working in a mapping that adds loop/effect mode to pads. But mixxx don't have buttons at the screen as rekordbox and virtual dj, so I replaced the Transition FX of the controller to toggle between those modes.

I'm not confident that this should be shipped with mixxx because no other program use Transition FX for that and I'm changing the button function, what can cause confusion to users.

I already posted my mapping at the forum to anyone interested, but I want to know what mixxx team think about it.

Sorry if this is the wrong place to have this conversation, I was going to post in the issues at github, but it is disabled and lauchpad is just for bugs.

My vote would be to try and make the buttons act as accurately as they are labelled. I think this is the auto-dj function, however even this is debatable.... You could argue it should do nothing at all, as we don't have a 'transition fx' feature
From the official documentation:

WeDJ for iPhone:

WeDJ for Andriod

djay:

edjing mix for iOS:

edjing mix for Andriod:

rekordbox:

geovannimp · 2021-04-19T16:05:21Z

@dan-giddins Following documentation the correct is to do nothing, but we loose one button that could be used to add a ton of functionalities.

Should we follow documentation or delivery a full-featured map to users?

Please, do not understand this as a criticism. This is the reason why I'm not confident too if this feature should be official.

dan-giddins · 2021-04-19T19:04:32Z

Should we follow documentation or delivery a full-featured map to users?

@Holzhaus what do you think?

fkbreitl · 2021-04-19T19:13:23Z

Hello and sorry for my late response, but I was playing on the weekend.

I have some questions and remarks:

Do I understand correctly that you are changing this code without being able to test it because you don't even have this controller and it is only me who can do the testing?
Please consider that I am sill not using Mixxx because of some missing features such as marking every first bar of the beatgrid, a status bar (which I reported already and waiting to get fixed), which means it is not easy for me to test this and I am busy at the moment.
Experience says never touch a running system. Since this code was running perfectly fine I am wondering what you try to accomplish that could make it worth going through all the reviewing and testing while there are hundreds of open bugs and feature request desperately waiting for somebody to work on them. Why do you want to change this code instead of https://bugs.launchpad.net/mixxx/+bug/1899759 or https://bugs.launchpad.net/mixxx/+bug/1899759 for example?
I already disagree with some of the proposed changes, but I am hesitating to start the reviewing without convincing reasons. Anyway I will drop some thoughts above.

Be-ing · 2021-04-19T19:19:22Z

Sorry if this is the wrong place to have this conversation, I was going to post in the issues at github, but it is disabled and lauchpad is just for bugs.

Please post in the controller mapping stream on Zulip instead of expanding the scope of this PR.

geovannimp · 2021-04-19T19:49:57Z

@fkbreitl

I have this controller, but this is my first controller and I'm starting DJing as hobby. I tested this changes, but I can submit a video testing to check if everything is ok.
I'm a Javascript developer and this was my first change just to learn mapping script. As this worked I tried to contribute, but I don't think this is something extremely necessary.
I don't agree with a lot of code design of the original mapping, but tried to change less as possible to have less chances of breaking something (Your concern in question 2)

geovannimp · 2021-04-19T19:57:52Z

@Be-ing Thanks, I will post at zulip :D

fkbreitl · 2021-04-19T21:05:10Z

@geovannimp Ok, I understand now this is an exercise. But things are not as simple as they seem. I will try to explain a bit more about it above or at Zulip. But unfortunately I don't have too much time for code reviews and zulip discussions at the moment.

fkbreitl · 2021-04-19T21:45:33Z

Now from looking at Zulip I understand that your motivation for this pull request is to add more functionality to the Transition FX button.
In that case I would suggest to add this functionality first.
Once this is accomplished, you can think about code cosmetics or optimizations.
But doing both at the same time will create too much confusion and should be avoided.
Better do it in different pull requests and try to give it more meaningful names.
It is not clear to me what "Listen to changes from mixxx" is supposed to mean.

Holzhaus · 2021-04-19T22:10:15Z

@fkbreitl this PR is a completely different issue and just a Bugfix. What it does is that if you add/delete a hotcue via mouse, then this PR makes sure that the pad LED is switched on or off.

geovannimp · 2021-04-19T22:12:12Z

@fkbreitl Actually this PR has nothing to do with Transition FX. This was just some improvements I made to learn the "framework".

Sorry if this title is not clear, I will try to explain what my changes do:

In current implementation if I change the beat sync, play/pause or add/delete a hotcue from mixxx interface, leds from the controller won't change. With this PR, every one of those action in mixxx interface will update controller leds.

As I said before, those are not life change fixes, but improve the experience with the controller.

fkbreitl · 2021-04-19T22:56:52Z

Ok, now I see what you are trying to do and why you are adding these connections.
Yes, this is a useful fix. 👍

Holzhaus · 2021-04-27T20:56:34Z

Please rebase on 2.3.

Holzhaus · 2021-04-29T16:51:35Z

Please rebase on 2.3.

FYI here's how you can do that: https://github.com/mixxxdj/mixxx/wiki/using%20git#targeting-another-base-branch

Holzhaus · 2021-05-05T11:32:01Z

Ping @geovannimp

geovannimp · 2021-05-05T16:39:18Z

@Holzhaus Sorry, I forgot to answer.

I will be doing the rebase soon as I have time, probably this weekend.

uklotzde · 2021-05-11T17:37:49Z

@geovannimp Any progress? We should get this ready for 2.3.0.

Those updates listen to changes from mixxx interface to replicate at the controller

Holzhaus · 2021-05-11T17:54:07Z

@geovannimp ticked the "Allow edits from maintainers" checkbox when opening the pull request, so I took care of it. @fkbreitl @dan-giddins please retest.

geovannimp · 2021-05-11T18:04:41Z

I totally forgot to rebase. Thanks @Holzhaus

uklotzde · 2021-05-11T21:25:14Z

This should go into 2.3.0 to complete the initial support for the controller.

Swiftb0y · 2021-05-12T09:24:33Z

res/controllers/Pioneer-DDJ-200-scripts.js

+        for (var j = 1; j <= 8; j++) {
+            // run switchPadLED after every hotcue update
+            engine.makeConnection(vgroup, "hotcue_" + j + "_enabled", function(ch, vgroup, control) {
+                var pad = Number(control.split("_")[1]);


Suggested change

var pad = Number(control.split("_")[1]);

var pad = j;

Should have the same effect, but please test.

This don't work cause the callback function is in another context. As the function executes outside the for loop, j will be always 8.

I could write this instead:

engine.makeConnection(vgroup, "hotcue_" + j + "_enabled", function (pad) { return function(ch, vgroup, control) { var vDeckNo = script.deckFromGroup(vgroup); var d = (vDeckNo % 2) ? 0 : 1; // d = deckNo - 1 DDJ200.switchPadLED(d, pad, ch); }; }(j));

But I think the code readability would be compromised.

I guess you are right: Creating closures in loops: A common mistake

Interesting, I figured j was copied in the loop because its just number. Both of your solutions are fine. You can keep it as is if you like.

@Swiftb0y If I had to use JavaScript I would fail miserably. Looked it up out of curiosity and was surprised, unpleasantly of course.

Holzhaus · 2021-05-13T11:31:07Z

Do we wait for confirmation from @dan-giddins or @fkbreitl or do we merge now?

dan-giddins · 2021-05-13T12:46:16Z

@Holzhaus I'm not able to test this right now as I'm not at home and don't have my DDJ-200 to hand. It will need to be up to @fkbreitl

uklotzde

Do we need to update the manual? Otherwise let's merge this PR now.

Holzhaus · 2021-05-17T16:42:05Z

I don't think this needs manual changes.

github-actions bot added the controller mappings label Apr 17, 2021

Holzhaus reviewed Apr 17, 2021

View reviewed changes

Be-ing reviewed Apr 17, 2021

View reviewed changes

Holzhaus reviewed Apr 18, 2021

View reviewed changes

dan-giddins suggested changes Apr 19, 2021

View reviewed changes

Holzhaus approved these changes Apr 27, 2021

View reviewed changes

Holzhaus changed the base branch from main to 2.3 April 27, 2021 20:56

uklotzde added this to the 2.3.0 milestone May 5, 2021

geovannimp added 3 commits May 11, 2021 19:48

Listen to changes from mixxx

e27fd43

Those updates listen to changes from mixxx interface to replicate at the controller

Optimize code changing only necessary leds

1923a3d

Fix lint problems

f0ff2fc

Swiftb0y reviewed May 12, 2021

View reviewed changes

Swiftb0y approved these changes May 13, 2021

View reviewed changes

uklotzde approved these changes May 16, 2021

View reviewed changes

Holzhaus merged commit 915dc2f into mixxxdj:2.3 May 17, 2021

DDJ-200: Listen to changes from mixxx #3793

DDJ-200: Listen to changes from mixxx #3793

Conversation

geovannimp commented Apr 17, 2021

Holzhaus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus commented Apr 17, 2021

geovannimp commented Apr 18, 2021

dan-giddins commented Apr 18, 2021 • edited

uklotzde commented Apr 18, 2021

uklotzde commented Apr 18, 2021

geovannimp commented Apr 18, 2021

geovannimp commented Apr 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geovannimp commented Apr 19, 2021

Choose a reason for hiding this comment

dan-giddins commented Apr 19, 2021 • edited

geovannimp commented Apr 19, 2021

dan-giddins commented Apr 19, 2021

fkbreitl commented Apr 19, 2021

Be-ing commented Apr 19, 2021

geovannimp commented Apr 19, 2021

geovannimp commented Apr 19, 2021

fkbreitl commented Apr 19, 2021

fkbreitl commented Apr 19, 2021 • edited

Holzhaus commented Apr 19, 2021 • edited

geovannimp commented Apr 19, 2021

fkbreitl commented Apr 19, 2021

Holzhaus commented Apr 27, 2021

Holzhaus commented Apr 29, 2021

Holzhaus commented May 5, 2021

geovannimp commented May 5, 2021

uklotzde commented May 11, 2021

Holzhaus commented May 11, 2021

geovannimp commented May 11, 2021

uklotzde commented May 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Holzhaus commented May 13, 2021

dan-giddins commented May 13, 2021

uklotzde left a comment

Choose a reason for hiding this comment

Holzhaus commented May 17, 2021

dan-giddins commented Apr 18, 2021 •

edited

dan-giddins commented Apr 19, 2021 •

edited

fkbreitl commented Apr 19, 2021 •

edited

Holzhaus commented Apr 19, 2021 •

edited