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.

Sign up for GitHub

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

Jump to bottom

Pioneer DDJ-200: fix hotcue button release #3742

Merged
merged 1 commit into from
Mar 29, 2021
Merged

Pioneer DDJ-200: fix hotcue button release #3742

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 17 additions & 13 deletions res/controllers/Pioneer-DDJ-200-scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,22 +281,26 @@ DDJ200.cueGotoandstop = function(channel, control, value, status, group) {
};

DDJ200.hotcueNActivate = function(channel, control, value, status, group) {
var vDeckNo = DDJ200.vDeckNo[script.deckFromGroup(group)];
var vgroup = "[Channel" + vDeckNo + "]";
var hotcue = "hotcue_" + (control + 1);
engine.setValue(vgroup, hotcue + "_activate", true);
midi.sendShortMsg(status, control,
0x7F * engine.getValue(vgroup, hotcue + "_enabled"));
var deckNo = script.deckFromGroup(group);
midi.sendShortMsg(0x90 + deckNo - 1, 0x0B, 0x7F *
engine.getValue(vgroup, "play")); // set play LED
if (value) { // only if button pressed, not releases, i.e. value === 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should early exit here:

Suggested change
if (value) { // only if button pressed, not releases, i.e. value === 0
if (!value) {
return;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally this would be an option, but in the rest of the code I was doing it this way.
So it could be a bit inconsistent and confusing if at these two places I change the coding style.
So to go with minimal changes and maximum consistency I would suggest to keep it as is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another alternative solution would be to just unbind the note-off midi in the XML.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. How?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Swiftb0y Buttons might be used for different purposes, i.e. for layering. I don't recommend to ignore MIDI signals in the XML, which should unconditionally forward signals to JS. Except those controls that are connected directly to COs. Subsequent filtering in JS is more versatile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, as I understand this discussion, it should be fine either way (Holzhaus or mine).
So I would like to suggest to pull it as is.
Then I will mark this conversation as resolved, to proceed with the fix.
Is this ok?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. We'd still prefer if you could rebase on 2.3 though so the newer linters run. There is no benefit in merging into 2.2.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but Holzhaus also said it shouldn't hurt to merge it from 2.2.
Since we are talking just about just one line, the linters will run.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked it out locally, pre-commit checks passed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

var vDeckNo = DDJ200.vDeckNo[script.deckFromGroup(group)];
var vgroup = "[Channel" + vDeckNo + "]";
var hotcue = "hotcue_" + (control + 1);
engine.setValue(vgroup, hotcue + "_activate", true);
midi.sendShortMsg(status, control,
0x7F * engine.getValue(vgroup, hotcue + "_enabled"));
var deckNo = script.deckFromGroup(group);
midi.sendShortMsg(0x90 + deckNo - 1, 0x0B, 0x7F *
engine.getValue(vgroup, "play")); // set play LED
}
};

DDJ200.hotcueNClear = function(channel, control, value, status, group) {
var vDeckNo = DDJ200.vDeckNo[script.deckFromGroup(group)];
var vgroup = "[Channel" + vDeckNo + "]";
engine.setValue(vgroup, "hotcue_" + (control + 1) + "_clear", true);
midi.sendShortMsg(status-1, control, 0x00); // set hotcue LEDs
if (value) { // only if button pressed, not releases, i.e. value === 0
var vDeckNo = DDJ200.vDeckNo[script.deckFromGroup(group)];
var vgroup = "[Channel" + vDeckNo + "]";
engine.setValue(vgroup, "hotcue_" + (control + 1) + "_clear", true);
midi.sendShortMsg(status-1, control, 0x00); // set hotcue LEDs
}
};

DDJ200.pfl = function(channel, control, value, status, group) {
Expand Down