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

Controller: ION Discover DJ Pro #2893

Merged
merged 1 commit into from
Mar 11, 2021
Merged

Controller: ION Discover DJ Pro #2893

merged 1 commit into from
Mar 11, 2021

Conversation

meltedpianoman
Copy link

@meltedpianoman meltedpianoman commented Jun 20, 2020

An initial attempt to have support for the ION Discover DJ Pro. This file
was taken from https://mixxx.org/forums/viewtopic.php?f=7&t=3343 and modified
to make it work with a recent version of Mixxx.

Signed-off-by: meltedpianoman [email protected]


This is actually a follow up of pull-request: #2867
It has been reworked using the pre-commit scripts to comply with the code style.
It also has been rebased on the 2.2 branch.
After the rebase I did not known how to update the original pull request, and therefore I created this new one.


See also topic: https://mixxx.org/forums/viewtopic.php?f=7&t=3343
It would be nice to see this supported by Mixx by default, so that it can be improved by the community and it will be maintained in case XML format / JS script changes are introduced.

I took the great work of joachim and made some small changes:

  • Updated the XML to the new format
  • Inverted both speed sliders
  • Corrected the filename of the script file in the midi.xml file (as I use a Linux I have a case sensitive file system and Mixxx was unable to open the script)
  • Renamed the files a bit

I'm not a DJ and I only performed some quick tests. Basic operation seems to work for me. I tested it on:

  • Kubuntu 19.10 with Mixxx version 2.2.1
  • Lubuntu 20.04 with Mixxx version 2.2.3

Regards,
Ivo Sieben

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

res/controllers/Ion-Discover-DJ-Pro-scripts.js Outdated Show resolved Hide resolved
res/controllers/Ion-Discover-DJ-Pro-scripts.js Outdated Show resolved Hide resolved
res/controllers/Ion-Discover-DJ-Pro-scripts.js Outdated Show resolved Hide resolved
res/controllers/Ion-Discover-DJ-Pro.midi.xml Show resolved Hide resolved
res/controllers/Ion-Discover-DJ-Pro.midi.xml Outdated Show resolved Hide resolved
@Holzhaus Holzhaus added this to the 2.2.5 milestone Jun 24, 2020
An initial attempt to have support for the ION Discover DJ Pro. This file
was taken from https://mixxx.org/forums/viewtopic.php?f=7&t=3343 and modified
to make it work with a recent version of Mixxx.

Signed-off-by: meltedpianoman <[email protected]>
@meltedpianoman
Copy link
Author

Hi @Holzhaus,

I tried to rework on your code review. I pushed the changes.
I did that with an amend + force push, so you won't be able to see the exact changes that I've made.
I hope that is alright.

Thank you for reviewing!

Regards,
Ivo

@meltedpianoman
Copy link
Author

Hi @Holzhaus ,

What is the status of this pull request. I see some checks are failing.
Can it be merged to the 2.2 branch, or does it require rework?

Regards,
Ivo

@Be-ing
Copy link
Contributor

Be-ing commented Jul 10, 2020

Is there a wiki page for this controller? I don't see one on https://github.com/mixxxdj/mixxx/wiki/Hardware%20compatibility

@meltedpianoman
Copy link
Author

Is there a wiki page for this controller? I don't see one on https://github.com/mixxxdj/mixxx/wiki/Hardware%20compatibility

Huh? I'm pretty confident that I've created one. The link should be: https://mixxx.org/wiki/doku.php/ion_discover_dj_pro
But has the WIKI section moved to the Github in the meantime?
If so: is the old WIKI still accessible so that I copy the changes to GitHub?

@Holzhaus
Copy link
Member

I can check if the wiki article is in the history of the wiki git repo. If not, it's possible that you created the wiki article after we created the dump and before we disabled editing in the old wiki. In that case we need to ask @RJ to migrate the wiki article manually.

@meltedpianoman
Copy link
Author

Hi @Holzhaus , did you find the wiki article in the repository history?

@Holzhaus
Copy link
Member

Hi @Holzhaus , did you find the wiki article in the repository history?

Unfortunately I can't find it in the history. Maybe @rryan can have a look at the old DokuWiki files?

@Be-ing
Copy link
Contributor

Be-ing commented Oct 24, 2020

I just took a look in the archive of DokuWiki and I only see the Ion Discover DJ page. If I understand correctly, that's not the same controller as this, right?

@Holzhaus
Copy link
Member

I just took a look in the archive of DokuWiki and I only see the Ion Discover DJ page. If I understand correctly, that's not the same controller as this, right?

No, that wiki page is from 2016. I think @meltedpianoman created the page after we dumped the DokuWiki DB and started the Github wiki migration but before we disabled DokuWiki.

@Be-ing
Copy link
Contributor

Be-ing commented Oct 24, 2020

That's unfortunate. In that case I don't think it can be recovered... 😞

@Holzhaus
Copy link
Member

Holzhaus commented Oct 24, 2020

Did we actually shut down DokuWiki and delete the files? I thought we just added a rewrite rule to redirect requests to GitHub Wiki, but the files would still be on the server?

@Be-ing
Copy link
Contributor

Be-ing commented Oct 24, 2020

Oh, good point. Perhaps we can remove the rewrite rule then.

@Be-ing
Copy link
Contributor

Be-ing commented Oct 24, 2020

Ah, I SSHed into the web server and found the page. Here it is on the GitHub wiki. There isn't much there though.

@Holzhaus
Copy link
Member

Holzhaus commented Oct 24, 2020

Ah OK, thanks.

@meltedpianoman Please open a PR against the manual-2.3.x branch of the https://github.com/mixxxdj/manual repo and document all controls that are working. List anything that is not working yet in a "Known Issues" section.

@meltedpianoman
Copy link
Author

Ah, I SSHed into the web server and found the page. Here it is on the GitHub wiki. There isn't much there though.

OK, I've update the https://github.com/mixxxdj/mixxx/wiki/Hardware-Compatibility wiki page and linked to the restored ION Discover DJ Pro page. Again... I'm not an expert, so I had to guess some properties of this controller:

  • I think the "2 deck all-in-one" description is correct
  • It has some audo interfaces via USB, so my guess was that "Integrated Audio Interface" should be "yes"
  • Balanced Outputs? No clue
  • I hope the support can make it for Mixxx release 2.2.5.... but maybe I should have put there "future" instead?
  • Not sure when this controller was released, but my guess is 2012

@meltedpianoman
Copy link
Author

Ah OK, thanks.

@meltedpianoman Please open a PR against the manual-2.3.x branch of the https://github.com/mixxxdj/manual repo and document all controls that are working. List anything that is not working yet in a "Known Issues" section.

OK... this is all very new to me. Really great how you guys set this up by the way.

I created a pull request for this: mixxxdj/manual#266

@github-actions
Copy link

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Jan 28, 2021
@meltedpianoman
Copy link
Author

@Holzhaus : Hi, I see this PR is now marked as stale. I guess this is because I did not find the time to continue working on the manual. I'll try to do this in the coming period.

But I have a question: what will it take for the PR to get approved? Is the update to the manual the last step in the process. When the manual PR is approved, both PR will get merged? Or are there more to come. I would really get this PR approved, as I would like this controller to be supported in a future release of Mixxx.

@Holzhaus
Copy link
Member

Holzhaus commented Jan 28, 2021

I don't remember if this has already passed code review or not.

Generelly, we need proper documentation for our review to check if the button/knob mappings make sense and the code matches the description.

If both the documentation and the mapping are fine, we merge ;-)

@github-actions github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Jan 29, 2021
@meltedpianoman
Copy link
Author

meltedpianoman commented Feb 6, 2021

Hi @Holzhaus ,

I think this code already got passed the code review: the code review was already performed with my previous pull-request: #2867. But due to me being inexperienced with Github I created a new Pull Requeest for the processed review changes. But if you have any other remarks on this controller mapping, let me know.

Furthermore I updated the mixxxdj/manual#266 pull request with the button/knob mappings using the SVG drawings that you provided. I hope that when the manual changes get approved, this controller mapping can also be approved.

Regards,
Ivo

@JoergAtGithub
Copy link
Member

I can't see any open task. What prevents merging of this mapping?

@Holzhaus
Copy link
Member

Holzhaus commented Mar 7, 2021

The manual PR still needs a tiny bit of work.

@meltedpianoman
Copy link
Author

I pushed the requested changes to the manual (mixxxdj/manual#266). Hopefully this controller mapping is then go to go.

Thanks,
Ivo

@Holzhaus Holzhaus changed the base branch from 2.2 to 2.3 March 11, 2021 20:55
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Thank you for you continued work on this! LGTM.

@Holzhaus Holzhaus merged commit d72af1b into mixxxdj:2.3 Mar 11, 2021
@uklotzde uklotzde modified the milestones: 2.2.5, 2.3.0 May 11, 2021
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