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

Add midiThrough argument #4148

Closed
wants to merge 2 commits into from
Closed

Add midiThrough argument #4148

wants to merge 2 commits into from

Conversation

eddsalkield
Copy link

@eddsalkield eddsalkield commented Jul 26, 2021

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

src/util/cmdlineargs.h Outdated Show resolved Hide resolved
@Be-ing
Copy link
Contributor

Be-ing commented Jul 26, 2021

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.

@ronso0
Copy link
Member

ronso0 commented Jul 26, 2021

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.

@eddsalkield
Copy link
Author

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.

@coveralls
Copy link

coveralls commented Jul 26, 2021

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%
Files with Coverage Reduction New Missed Lines %
src/engine/cachingreader/cachingreaderworker.cpp 2 63.48%
src/engine/cachingreader/cachingreader.cpp 9 71.38%
Totals Coverage Status
Change from base Build 1065232258: -0.01%
Covered Lines: 20079
Relevant Lines: 70199

💛 - Coveralls

@ronso0
Copy link
Member

ronso0 commented Jul 26, 2021

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 ronso0 added this to the 2.4.0 milestone Jul 26, 2021
@ronso0
Copy link
Member

ronso0 commented Jul 26, 2021

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

@ronso0
Copy link
Member

ronso0 commented Sep 25, 2021

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

@Be-ing Be-ing removed this from the 2.4.0 milestone Nov 14, 2021
@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 Feb 12, 2022
@digitalsignalperson
Copy link

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

@github-actions github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Dec 27, 2023
@neopostmodern
Copy link

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?

@daschuer
Copy link
Member

daschuer commented Feb 3, 2024

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?

@ronso0
Copy link
Member

ronso0 commented Nov 19, 2024

Closing this now.
Replaced by #13909

@ronso0 ronso0 closed this Nov 19, 2024
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.

Display MIDI Through device in non-developer mode
7 participants