mixxx
mixxx copied to clipboard
Add midiThrough argument
Adds a --midiThrough
argument to enable the display of Midi Through ports in Preferences/Controllers.
Is the first proposed step to resolve #8356
Rereading the prior discussion, I am confused why you chose this implementation. It seemed the consensus was to add a checkbox in the preferences rather than a command line argument.
Indeed. Implementing the option as cla makes it less discoverable for 'regular users' who look for a way to hook up lighting to Mixxx. I think putting a checkbox into the 'Controllers' root page makes more sense, though it is obviously more work to handle toggling MIDI Through after start, e.g. first uncheck the Enabled box in 'MIDI Through', then allow to disable it entirely.
I agree that a checkbox would be a better long term solution, but I couldn't discern particular consensus from that thread. On 2021-01-20, rons0 commented that:
Therefore I propose to add the command line switch first so experienced users can test it and we can collect feedback. After we have all issues sorted we can continue to add it to the Preferences incl. additional safety guards for restarts in case a loaded mapping locks up Mixxx (happened to me).
This was the last comment in the thread, at the time of writing, that was relevant to the implementation, and it didn't seem to get contested.
I'm happy to look into a checkbox in the menu, but I wrote this patch in part because I needed to work around this problem quickly.
Pull Request Test Coverage Report for Build 1068985745
- 5 of 6 (83.33%) changed or added relevant lines in 2 files are covered.
- 11 unchanged lines in 2 files lost coverage.
- Overall coverage decreased (-0.01%) to 28.603%
Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
---|---|---|---|
src/controllers/midi/portmidienumerator.cpp | 0 | 1 | 0.0% |
<!-- | Total: | 5 | 6 |
Files with Coverage Reduction | New Missed Lines | % |
---|---|---|
src/engine/cachingreader/cachingreaderworker.cpp | 2 | 63.48% |
src/engine/cachingreader/cachingreader.cpp | 9 | 71.38% |
<!-- | Total: | 11 |
Totals | |
---|---|
Change from base Build 1065232258: | -0.01% |
Covered Lines: | 20079 |
Relevant Lines: | 70199 |
💛 - Coveralls
Yes, that was my last statement there. But various support requests on the forums from less tech-savvy users made me change my mind. IMO this option needs to be accessible without having to link to instructions on how to run Mixxx from the command line first, or modify the Mixxx launcher. Also, a preference option would also allow to make that option persist.
I understand that the current implementation is the easiest way to do it. If you don't know how to implement the checkbox solution you can take a look at how for example the 'fullscreen' option is handled in Pref >Interface https://github.com/mixxxdj/mixxx/blob/a5f0e5ff05fa6774f42fb974ebcca7052c07bbfe/src/preferences/dialog/dlgprefinterface.cpp Should be easy as it's just a checkbox and no runtime actions, since I suppose a first implementation that enables MIDI Through on next start (like fullscreen) is a good first step forward.
@eddsalkield Btw, thanks for your first contribution : ) Please sign the contributor agreement and comment here when done. Thank you!
@eddsalkield Any news on this? I think this would be an appreciated feature. Are you motivated to work on the checkbox implementation?
This PR is marked as stale because it has been open 90 days with no activity.
as a new Mixx user this is something I've stumbled over right now it would be great to have more flexible midi routing with this
Also ran into this issue now. Since there is so little activity on this, wouldn't it be better to just merge this feature as is, rather than not having it for another couple of years?
We can't merge this right now. It is conflicting and we have also no formal permission. You must also not underestimate the work that is involved to Dokument this feature and later do the transition to the checkbox solution. With the --developer flag we already have a flag for it.
Do you have interest to start over with the checkbox?