DDJ-200 support for 4 decks #3193

fkbreitl · 2020-10-20T13:09:48Z

No description provided.

Holzhaus · 2020-10-20T15:34:38Z

Does this PR change anything from the user's perspective? If so, please document that with a PR to the manual repo.

fkbreitl · 2020-10-20T15:47:58Z

Everything in the manual is still valid. There are some additional features which I will add to the manual soon. In the meantime the files could already be updated.Am 20.10.2020 17:34 schrieb Jan Holthuis <[email protected]>: Does this PR change anything from the user's perspective? If so, please document that with a PR to the manual repo. —You are receiving this because you authored the thread.Reply to this email directly, view it on GitHub, or unsubscribe.

Holzhaus · 2020-10-20T16:17:31Z

In the meantime the files could already be updated.

Without the documentation, we can't review this properly. Also, I think the policy should be to only merge changes if the documentation is ready, too. Otherwise contributors might get busy or lose interest after the merge and we end up with even undocumented or not properly documented controller mappings in the mixxx repo :D

fkbreitl · 2020-10-20T22:02:49Z

Ok, I have updated the documentation (the PR is still open) and fixed all issues from CodeFactor.
Code should be ready for review.

Holzhaus · 2020-10-20T22:50:56Z

I have updated the documentation (the PR is still open)

Can you post a link? I don't see the PR.

fkbreitl · 2020-10-20T22:58:00Z

Sorry, it always goes to your repo. Here is the correct one: mixxxdj/manual#237

Holzhaus

Thank you. Some first comments.

Holzhaus · 2020-10-23T11:06:09Z

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

+    engine.beginTimer(500, "engine.setValue(\"[Library]\", " +
+                      "\"MoveFocus\", 1);", true);


Please don't use a string here, pass a function expression.

Holzhaus · 2020-10-23T11:07:24Z

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

+        1: this.vDeck1, 2: _.clone(this.vDeck1),
+        3: _.clone(this.vDeck1), 4: _.clone(this.vDeck1)


I'm really not a fan of lodash. This is very unreadable IMHO.

What alternative do you suggest?

Something like this maybe?

this.vDeck = {} for (var i = 1; i <= 4; i++) { this.vDeck[i] = { syncEnabled: false, volMSB: 0, rateMSB: 0, jogDisabled: false, } }

Holzhaus · 2020-10-23T11:09:19Z

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

+    for (var i = 1; i <= 4; i++) {
+        var vgroup = "[Channel" + i + "]";
+        // run onTrackLoad after every track load to e.g. set LEDs accordingly
+        engine.connectControl(vgroup, "track_loaded", "DDJ200.onTrackLoad");


Please use engine.makeConnection instead and don't use a string, pass the actual function.

Holzhaus · 2020-10-23T11:10:17Z

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

+        var deckNo = script.deckFromGroup(group);
+        var vDeckNo = DDJ200.vDeckNo[deckNo];
+        var vgroup = "[Channel" + vDeckNo +"]";
+        engine.setValue(vgroup, "LoadSelectedTrack", true);


Use script.triggerControl here.

Holzhaus · 2020-10-23T11:11:12Z

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

+        engine.beginTimer(900, "DDJ200.vDeck[" + vDeckNo +
+                          "][\"jogDisabled\"] = false;", true);


use a function expression

Holzhaus · 2020-10-23T11:27:37Z

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

+            if (deckNo === 1) {
+                // toggle virtual deck of controller deck 1
+                DDJ200.vDeckNo[1] = 4 - DDJ200.vDeckNo[1];
+                if (DDJ200.vDeckNo[1] === 1) LED = 0;


missing braces

Holzhaus · 2020-10-23T11:27:46Z

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

+            } else { // deckNo === 2
+                // toggle virtual deck of controller deck 2
+                DDJ200.vDeckNo[2] = 6 - DDJ200.vDeckNo[2];
+                if (DDJ200.vDeckNo[2] === 2) LED = 0;


missing braces

Holzhaus · 2020-10-23T11:27:55Z

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

+            0x7F * engine.getValue(vgroup, "pfl"));
+    }
+
+    if (vDeckNo % 2) c = 7; else c = 9;


missing braces

Holzhaus · 2020-10-23T13:46:05Z

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

+        1: this.vDeck1, 2: _.clone(this.vDeck1),
+        3: _.clone(this.vDeck1), 4: _.clone(this.vDeck1)


Something like this maybe?

this.vDeck = {} for (var i = 1; i <= 4; i++) { this.vDeck[i] = { syncEnabled: false, volMSB: 0, rateMSB: 0, jogDisabled: false, } }

Holzhaus · 2020-10-23T13:46:37Z

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

+        var vgroup = "[Channel" + vDeckNo +"]";
+        var pfl = ! engine.getValue(vgroup, "pfl");
+        engine.setValue(vgroup, "pfl", pfl);
+        if (DDJ200.fourDeckMode === false) {


Suggested change

if (DDJ200.fourDeckMode === false) {

if (!DDJ200.fourDeckMode) {

Holzhaus · 2020-10-23T13:47:29Z

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

+
+DDJ200.switchLEDs = function(vDeckNo) {
+    // set LEDs of controller deck according to virtual deck
+    var c = 1; if (vDeckNo % 2) c = 0;


missing braces and multiple statements on the same line. Also, please use more meaningful variable names, e.g.: channelOffset or something like that.

fkbreitl

I have don't the changes you requested

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

Co-authored-by: Jan Holthuis <[email protected]>

…-deck

fkbreitl · 2020-10-24T20:01:50Z

I guess I have to pull the changes from my git repo first and then copy and past the patch in the file (since I don't see a save option).

fkbreitl · 2020-10-24T20:10:48Z

Unfortunately after the above:
error: corrupt patch at line 6

Holzhaus · 2020-10-24T20:18:19Z

Are you sure you're in the correct branch?

Holzhaus · 2020-10-24T20:21:04Z

Maybe it's a copy and paste issue. Here's a file you can download and use for git apply: patch.txt

fkbreitl · 2020-10-24T20:42:37Z

Maybe. Thank you.
I have now corrected my flymake indentation for Javascript and indented the whole buffer instead of applying your patch, which should have done the same thing.
And I fixed all remaining +"]".

fkbreitl · 2020-10-24T21:11:44Z

I am also wondering what is the best way to keep my github repo up to date?
Should I pull the updates from mixxx to my laptop and then push them to my github repo or can I update github first and then pull it to my laptop, which would avoid the uploading part?

Holzhaus · 2020-10-24T21:21:26Z

Why would you need to keep your GitHub repo up-to-date? I just have configured an upstream remote:

$ git remote add upstream https://github.com/mixxxdj/mixxx.git

When you start a new branch, just fetch the latest changes from the remote and start a new branch from there:

$ git fetch upstream
$ git checkout -b mynewbranch upstream/2.3

You only need to push your branch, there's no reason to update other branches:

$ git push origin mynewbranch

fkbreitl · 2020-10-24T21:59:55Z

Aright. Thanks!

Holzhaus · 2020-10-25T10:56:42Z

Unfortunately the indentation doesn't conform to our style rules yet, here's another patch:
patch.txt

fkbreitl · 2020-10-25T12:35:00Z

Oh no, what are your style rules called? Is it not Javascript?

fkbreitl · 2020-10-25T12:48:33Z

How did you detect these issues? My pre-commit hook did not complain.

Holzhaus · 2020-10-25T12:51:02Z

How did you detect these issues? My pre-commit hook did not complain.

This branch is based on 2.2, but the pre-commit hooks only exist on 2.3 and onwards. I just merged this branch into 2.3 locally and ran the pre-commit hooks there, and used git diff to generate the patch.

fkbreitl · 2020-10-25T14:15:53Z

Should I write a bug report that the Mixxx style rules do not comply with JS conventions?

Holzhaus · 2020-10-25T14:33:55Z

Should I write a bug report that the Mixxx style rules do not comply with JS conventions?

What do you mean? Because we don't use hanging indents?

fkbreitl · 2020-10-25T15:31:30Z

Yes, I thought it was a rule, but apparently its not...

res/controllers/Pioneer DDJ-200.midi.xml

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

Co-authored-by: Jan Holthuis <[email protected]>

Holzhaus

Just a minor comment, otherwise LGTM.

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

Co-authored-by: Jan Holthuis <[email protected]>

Holzhaus

Thank you, LGTM.

fkbreitl · 2020-11-01T22:21:03Z

Thank you!

support for 4 decks

b42a5f6

github-actions bot added the controller mappings label Oct 20, 2020

fkbreitl mentioned this pull request Oct 20, 2020

Ddj 200 support #3185

Merged

Improvements from CodeFactor

8c07343

fkbreitl mentioned this pull request Oct 20, 2020

Pioneer DDJ 200 support #2377

Closed

Holzhaus requested changes Oct 23, 2020

View reviewed changes

Changes suggested by Holzhaus

eab186e

fkbreitl commented Oct 23, 2020

View reviewed changes

Changes from CodeFactor

cd74f9e

fkbreitl requested a review from Holzhaus October 23, 2020 15:12

More changes from CodeFactor

840b092

Holzhaus requested changes Oct 23, 2020

View reviewed changes

fkbreitl and others added 11 commits October 23, 2020 19:49

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

27e4e86

Co-authored-by: Jan Holthuis <[email protected]>

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

cf384fa

Co-authored-by: Jan Holthuis <[email protected]>

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

544dc7d

Co-authored-by: Jan Holthuis <[email protected]>

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

8d6c49d

Co-authored-by: Jan Holthuis <[email protected]>

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

2291f71

Co-authored-by: Jan Holthuis <[email protected]>

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

88e168b

Co-authored-by: Jan Holthuis <[email protected]>

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

185d969

Co-authored-by: Jan Holthuis <[email protected]>

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

c5ec405

Co-authored-by: Jan Holthuis <[email protected]>

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

c7f5b55

Co-authored-by: Jan Holthuis <[email protected]>

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

94b9cbd

Co-authored-by: Jan Holthuis <[email protected]>

Merge branch '2.2' of https://github.com/mixxxdj/mixxx into ddj-200-4…

28782a0

…-deck

Fixed Javascrip formatting issues

de4d322

applyed Mixxx indentation rule

c6ef0f5

Holzhaus requested changes Oct 28, 2020

View reviewed changes

fkbreitl and others added 6 commits October 29, 2020 15:51

Update res/controllers/Pioneer DDJ-200.midi.xml

20c2a1e

Co-authored-by: Jan Holthuis <[email protected]>

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

67fca05

Co-authored-by: Jan Holthuis <[email protected]>

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

8d3883a

Co-authored-by: Jan Holthuis <[email protected]>

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

5503b22

Co-authored-by: Jan Holthuis <[email protected]>

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

a2997c2

Co-authored-by: Jan Holthuis <[email protected]>

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

7720b63

Co-authored-by: Jan Holthuis <[email protected]>

Holzhaus approved these changes Oct 30, 2020

View reviewed changes

res/controllers/Pioneer-DDJ-200-scripts.js Outdated Show resolved Hide resolved

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

39a82bb

Co-authored-by: Jan Holthuis <[email protected]>

fkbreitl requested a review from Holzhaus November 1, 2020 22:04

Holzhaus approved these changes Nov 1, 2020

View reviewed changes

Holzhaus merged commit 81f5e67 into mixxxdj:2.2 Nov 1, 2020

		engine.beginTimer(500, "engine.setValue(\"[Library]\", " +
		"\"MoveFocus\", 1);", true);

		1: this.vDeck1, 2: _.clone(this.vDeck1),
		3: _.clone(this.vDeck1), 4: _.clone(this.vDeck1)

		engine.beginTimer(900, "DDJ200.vDeck[" + vDeckNo +
		"][\"jogDisabled\"] = false;", true);

	if (DDJ200.fourDeckMode === false) {
	if (!DDJ200.fourDeckMode) {

DDJ-200 support for 4 decks #3193

DDJ-200 support for 4 decks #3193

Conversation

fkbreitl commented Oct 20, 2020

Holzhaus commented Oct 20, 2020

fkbreitl commented Oct 20, 2020 via email

Holzhaus commented Oct 20, 2020 • edited Loading

fkbreitl commented Oct 20, 2020

Holzhaus commented Oct 20, 2020

fkbreitl commented Oct 20, 2020

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

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

fkbreitl left a comment

Choose a reason for hiding this comment

fkbreitl commented Oct 24, 2020

fkbreitl commented Oct 24, 2020

Holzhaus commented Oct 24, 2020

Holzhaus commented Oct 24, 2020

fkbreitl commented Oct 24, 2020

fkbreitl commented Oct 24, 2020

Holzhaus commented Oct 24, 2020

fkbreitl commented Oct 24, 2020

Holzhaus commented Oct 25, 2020

fkbreitl commented Oct 25, 2020

fkbreitl commented Oct 25, 2020

Holzhaus commented Oct 25, 2020

fkbreitl commented Oct 25, 2020

Holzhaus commented Oct 25, 2020

fkbreitl commented Oct 25, 2020

Holzhaus left a comment

Choose a reason for hiding this comment

Holzhaus left a comment

Choose a reason for hiding this comment

fkbreitl commented Nov 1, 2020

Holzhaus commented Oct 20, 2020 •

edited

Loading