mixxx icon indicating copy to clipboard operation
mixxx copied to clipboard

Add midiThrough argument

Open eddsalkield opened this issue 3 years ago • 11 comments

Adds a --midiThrough argument to enable the display of Midi Through ports in Preferences/Controllers. Is the first proposed step to resolve #8356

eddsalkield avatar Jul 26 '21 20:07 eddsalkield

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.

Be-ing avatar Jul 26 '21 20:07 Be-ing

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.

ronso0 avatar Jul 26 '21 21:07 ronso0

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.

eddsalkield avatar Jul 26 '21 21:07 eddsalkield

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 Coverage Status
Change from base Build 1065232258: -0.01%
Covered Lines: 20079
Relevant Lines: 70199

💛 - Coveralls

coveralls avatar Jul 26 '21 21:07 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.

ronso0 avatar Jul 26 '21 21:07 ronso0

@eddsalkield Btw, thanks for your first contribution : ) Please sign the contributor agreement and comment here when done. Thank you!

ronso0 avatar Jul 26 '21 23:07 ronso0

@eddsalkield Any news on this? I think this would be an appreciated feature. Are you motivated to work on the checkbox implementation?

ronso0 avatar Sep 25 '21 20:09 ronso0

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

github-actions[bot] avatar Feb 12 '22 00:02 github-actions[bot]

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

digitalsignalperson avatar Mar 24 '23 23:03 digitalsignalperson

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?

neopostmodern avatar Feb 03 '24 07:02 neopostmodern

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?

daschuer avatar Feb 03 '24 10:02 daschuer