Skip to content

KORG KAOSS DJ Revamp - #11639

Closed
uklotzde wants to merge 16 commits into
mixxxdj:2.4from
uklotzde:korg-kaoss-dj-revamp
Closed

KORG KAOSS DJ Revamp#11639
uklotzde wants to merge 16 commits into
mixxxdj:2.4from
uklotzde:korg-kaoss-dj-revamp

Conversation

@uklotzde

@uklotzde uklotzde commented Jun 9, 2023

Copy link
Copy Markdown
Contributor

Continuation of #4301 which seems to be abandoned and is in a bad state.

I haven't touched the corresponding manual PR mixxxdj/manual#430 yet.

@uklotzde uklotzde changed the title [WIP] KORG KAOSS DJ Revamp KORG KAOSS DJ Revamp Jun 10, 2023
@uklotzde
uklotzde marked this pull request as ready for review June 10, 2023 13:58

@Swiftb0y Swiftb0y left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

mostly lgtm. some of the new parts could be a little DRYer, are you interested in some suggestions or do you just want this merged?

Comment thread res/controllers/Korg-KAOSS-DJ-scripts.js
@uklotzde

Copy link
Copy Markdown
Contributor Author

mostly lgtm. some of the new parts could be a little DRYer, are you interested in some suggestions or do you just want this merged?

Yes please. I have tried to simplify the code a bit without rewriting it while preserving the proposed behavior.

@Swiftb0y Swiftb0y left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Anything more would involve quite large refactorings unfortunately.

Comment thread res/controllers/Korg-KAOSS-DJ-scripts.js Outdated
Comment thread res/controllers/Korg-KAOSS-DJ-scripts.js Outdated
@pi43r

pi43r commented Jul 4, 2023

Copy link
Copy Markdown
Contributor

Hey,
Thanks for taking this on you.
I started a PR for the manual. It's a bit of a mess but I can try to revise it.
Did you change any mappings?

@uklotzde

uklotzde commented Jul 4, 2023

Copy link
Copy Markdown
Contributor Author

Did you change any mappings?

I only cleaned up the code a bit and fixed/removed non-working parts as a starting point for future improvements.

raphaelquast and others added 12 commits July 4, 2023 13:27
<TAP>                : open folder
<TAP> + <TAP>        : double-tap to close folder
<SHIFT LEFT> + <TAP> : tap bpm of LEFT track
<SHIFT RIGHT> + <TAP> : tap bpm of RIGHT track

<browseKnob>           : browse library up & down
<SHIFT> + <browseKnob> : toggle focus between Playlist and File-Browser

<LOAD A/B>           : load track
<SHIFT> + <LOAD A/B> : open/close folder in file-browser
<SHIFT> + <TOUCHPAD X> : control super knob of QuickEffectRack for deck 1
<SHIFT> + <TOUCHPAD Y> : control super knob of QuickEffectRack for deck 2
@uklotzde
uklotzde changed the base branch from 2.3 to 2.4 July 4, 2023 11:28
@uklotzde

uklotzde commented Jul 4, 2023

Copy link
Copy Markdown
Contributor Author

Now targeting 2.4.

@uklotzde

uklotzde commented Jul 4, 2023

Copy link
Copy Markdown
Contributor Author

Those selective CI checks that fail in various circumstances by not covering all use cases are a real burden. A brute force enforcement once would have been much easier.

@Swiftb0y Swiftb0y left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for investing some time in the cleanup. Got one more modernization opportunity for you. the rest LGTM.

Comment thread res/controllers/Korg-KAOSS-DJ-scripts.js Outdated
// initialise decks
// Seb Dooris, Fayaaz Ahmed, Lee Arromba, Raphael Quast

// Using `const`/`let` causes ReferenceError: KAOSSDJ is not defined

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@Swiftb0y But why?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We expect the "controller namespace" (KAOSSDJ in this case) to be accessible via the global object. That only happens when variable is declared via var in the global context. Declaring it using const or let won't do that. You can confirm this in node js using the following snippet:

const a = "a";
let b = "b";
var c = "c";

console.log(`a: ${a}, global.a: ${global.a}\nb: ${b}, global.b: ${global.b}\nc: ${c}, global.c: ${global.c}`);

I'm not sure if we can access these variables without the global object, but we could try.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fix shouldn't be particularly difficult, I expect about 10-20 extra lines. Though I don't have time to look into this right now.

Swiftb0y

This comment was marked as off-topic.

@Swiftb0y Swiftb0y self-assigned this Sep 13, 2023
@uklotzde uklotzde closed this by deleting the head repository Jan 23, 2024
@pi43r

pi43r commented Jan 29, 2024

Copy link
Copy Markdown
Contributor

Sorry to pitch in on this again.
What needs to be done so it can go into 2.4?
It would be sad to see this not getting integrated

@daschuer

Copy link
Copy Markdown
Member

Unfortunately it looks like that Uwe dies not have interest to continue working on this mapping.
Do you have interest to help?

A first step would be to issue a new pull request, test it and test it. Than we need to consider what us left for a merge to the 2.4 branch.

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