Add MIDI GUI tab and learn function by ignotus666 · Pull Request #3502 · jamulussoftware/jamulus

ignotus666 · 2025-05-22T11:02:04Z

Adds a MIDI tab to the Settings menu and exposes the MIDI parameters so they can be changed via the GUI. MIDI CC numbers can be automatically set using "learn" buttons that when pressed, listen for an incoming MIDI message and store it. Starting Jamulus with --ctrlmidich command line arguments overrides and replaces those set via the GUI.

The MIDI-in port can be enabled/disabled at runtime instead of it being determined at launch by the --ctrlmidich parameter or by making it permanently open. This means that user names are prepended by a number depending on whether the MIDI in port has been enabled or not (as is the case now).

I've also ditched the "Offset" term, at least for the GUI. It's a hangover from the days when that's what it was, a value hard-coded for a specific Behringer device, but in the current context it no longer makes sense.

EDIT: Added JSONRPC support.

CHANGELOG: Client: Added MIDI tab to Settings GUI exposing MIDI parameters. MIDI Learn feature also added.

Context: Fixes an issue?

Does this change need documentation? What needs to be documented and how?

Needs to be documented.

Status of this Pull Request

Working on Linux, Windows and macOS but should be tested more thoroughly.

What is missing until this pull request can be merged?

Further testing and code review.

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

softins · 2025-05-28T09:59:06Z

Thanks for this contribution. It looks like a useful feature.

I will be happy to test it in due course, on Linux, Windows and Mac, but at the moment most of my kit is packed away for a pending house move.

ignotus666 · 2025-05-28T10:45:16Z

@softins thanks - no worries, I guess there will be plenty of time for testing before this ever makes it in (if it does).

Hope the house move goes smoothly.

ann0see · 2025-05-31T11:39:34Z

Nice!

I'd like to open a discussion:

Do you think a separate MIDI tab is useful or does it add clutter?
Is there a better word instead of "Learn" for the detector? How can we make the feature clear to the user? Would a dynamic text field showing currently pressed buttons be better? (I'm unsure as this could be more complicated)

ignotus666 · 2025-05-31T12:02:40Z

@ann0see

Given the number of MIDI settings, I don't see how it would fit in somewhere else without cluttering up that section in a worse way. I think it merits its own tab.
"Learn" is a standard term for this feature. I see and use it in many other MIDI applications. Maybe "MIDI learn"? But I think that might make the button too large.

In any event, this is still under development as I'm finding that some of my implementations are far from perfect. E.g. the way the MIDI in port is currently kept open I think has been done wrong, so I'm working on that. I'm testing a version where the MIDI port can be enabled/disabled at runtime, as among other things it affects whether numbers are prepended to user names in the mixer board - if you're not using MIDI you won't want that.

The main purpose for opening this PR was to generate builds for other platforms for testing, but for now it's just a PoC that's far from finished, so any suggestions are welcome.

ignotus666 · 2025-06-02T20:21:24Z

Tested on Linux, Windows (no JACK) and macOS (legacy and non-legacy), and it seems to be working on these three platforms at least.

src/clientsettingsdlg.cpp

pljones · 2026-01-25T13:37:06Z

OK just stumbled onto this. As this is a new feature, the following need to be adhered to:

All updates in the UI should only work by emitting a signal to the main application, where state is held -- no state to be held in the UI (I've not reviewed the code yet)
Any UI enhancement should be mirrored by an enhancement to the JSONRPC API, if the feature is missing there

(The "learn" feature for JSONRPC would just be the eventual "set X to Y" update, the visual behaviour would be for the JSONRPC user to supply - it just needs to be able to send the same update to the core code as the Client UI.)

pljones

Get rid of all the unnecessary changes in spacing and resubmit the PR. As it stands, I can't review it.

ignotus666 · 2026-01-25T20:28:18Z

Get rid of all the unnecessary changes in spacing and resubmit the PR. As it stands, I can't review it.

Done. Those keep getting added in by clang.

All updates in the UI should only work by emitting a signal to the main application, where state is held -- no state to be held in the UI (I've not reviewed the code yet)

It's been almost a year since I did this, but as far as I recall it updates the main application state via pSettings and pClient.

ann0see · 2026-01-25T21:53:40Z

I believe this should be rebased and squashed into one commit.

pljones · 2026-01-26T18:27:36Z

It's been almost a year since I did this, but as far as I recall it updates the main application state via pSettings and pClient.

It looks pretty good -- as @ann0see says, please rebase and squash. Then check the resultant diff for anything obvious 😄 - it looks mostly okay to me but there's a few places I'm not sure what was being intended, so I might have some more questions.

One thing I might ask is for the command line parsing code to be moved into soundbase.cpp (as a static), just to remove a bit of clutter.

ignotus666 · 2026-01-27T08:30:06Z

as @ann0see says, please rebase and squash.

Done.

One thing I might ask is for the command line parsing code to be moved into soundbase.cpp (as a static), just to remove a bit of clutter.

Done. I don't know what clang is whining about - I can't use it because it reformats a ton of stuff that shouldn't be.

ann0see · 2026-01-27T08:59:07Z

I don't know what clang is whining about - I can't use it because it reformats a ton of stuff that shouldn't be.

Known issue. We should really bump the clang format version. Otherwise be sure to use the same clang format version as used in the CI

ignotus666 · 2026-01-29T06:12:02Z

Any UI enhancement should be mirrored by an enhancement to the JSONRPC API, if the feature is missing there

I've been working on this - should the UI immediately reflect changes from JSONRPC commands sent? It doesn't seem to do this for the existing commands but just to confirm.

ignotus666 · 2026-01-29T09:45:59Z

Known issue. We should really bump the clang format version. Otherwise be sure to use the same clang format version as used in the CI

The problem is that in the TODO blocks, the TODO lines don't have a space after //, but all other comments do. I couldn't find a way to preserve that distinction other than by adding CommentPragmas: '^ TODO:|^### TODO:' to clang-format. It works (I'll use it myself for now), but maybe there's a better way.

src/sound/asio/sound.h

src/clientdlg.cpp

src/clientsettingsdlg.cpp

src/main.cpp

ignotus666 · 2026-02-03T10:49:15Z

@pljones I had a go at addressing most of your comments - the biggest change being moving the MIDI command line parsing to CSettings, then some cleaning up due to the move and other bits and pieces. I didn't implement the change you proposed for clientrpc.cpp as it requires std::unordered_map, which apparently may cause issues depending on the version of Qt used to compile.
Did some tests and it seems to work ok.

src/settings.cpp

src/main.cpp

src/settings.cpp

src/client.h

src/main.cpp

src/settings.cpp

src/sound/soundbase.cpp

src/sound/soundbase.h

src/clientsettingsdlg.cpp

src/main.cpp

pljones · 2026-02-04T18:12:42Z

Can the client know whether it will be possible to enable a MIDI port before trying?

Should it try if --ctrlmidich is set on the command line (i.e. as part of the parse) and issue a warning "your request was ignored as there's no MIDI port available" or something?

Should the tab be visible without a MIDI port? If that tab is there, should the checkbox be enabled?

ignotus666 · 2026-02-04T21:38:15Z

Can the client know whether it will be possible to enable a MIDI port before trying?

Should it try if --ctrlmidich is set on the command line (i.e. as part of the parse) and issue a warning "your request was ignored as there's no MIDI port available" or something?

Should the tab be visible without a MIDI port? If that tab is there, should the checkbox be enabled?

So I suppose in either case (launched via command line or launcher) it should check if it can create a port, and if not, disable the checkbox (and thus the MIDI settings), and issue a warning? I'm not sure it's practical or necessary to hide the tab altogether though.

ann0see · 2026-02-04T21:46:51Z

Either hide it or add gray it out.
Alternatively make the tab say what exactly needs to happen to enable midi.

I'd prefer to fully hide it if it's unavailable.

ignotus666 · 2026-02-05T06:21:54Z

Alternatively make the tab say what exactly needs to happen to enable midi.

But in this scenario if a MIDI port can't be created it's an OS problem, isn't it? It could be any number of things (permissions, missing libraries...), so how could Jamulus diagnose it and say what the solution is?

I'd prefer to fully hide it if it's unavailable.

If a user intends to use MIDI but launches Jamulus and finds the MIDI tab isn't even there, won't that lead to more confusion? At least if the tab's there (but the content is greyed out) and displays an error message when they try to enable the port they know they need to check their OS configuration.

BTW, there are already error messages output to the command line if a MIDI port can't be opened. I can add a check, and if the port can't be opened, it will uncheck the MIDI-in (if enabled) checkbox, grey out the settings, and display an error message. The thing is, depending on the OS, the probable cause is likely different. macOS/Linux allow multiple clients to connect to the same MIDI device, but Windows only grants exclusive access. A solution would be to make the error messages platform-specific in order to give the user the best chance to identify the problem.

pljones · 2026-02-05T17:26:22Z

Yes, I agree. Once the MIDI Tab exists, it should always appear. If it's not available because Jamulus cannot detect a MIDI port, the check box to enable the feature should be disabled with a message that makes it clear that all Jamulus knows is that it can't detect a MIDI port.

Are the existing errors issued by Jamulus (i.e. as part of trying to open the port)? If so, that's probably all that needs to be logged. For the UI, just indicate there's a problem opening the port, uncheck and disable the checkbox.

ignotus666 · 2026-02-05T18:08:34Z

Are the existing errors issued by Jamulus (i.e. as part of trying to open the port)?

Yes, that was already there.

For the UI, just indicate there's a problem opening the port, uncheck and disable the checkbox.

Done. If the port can't be created, the checkbox is disabled, the MIDI settings are greyed out, and an error message is displayed in the UI.

pljones · 2026-02-05T18:34:52Z

src/sound/asio/sound.cpp

        {
            Midi.MidiStart();
-            bMidiEnabled = true;
+            bMidiEnabled = Midi.IsActive(); // Only set to true if MIDI actually started


Maybe put this after the if / else and get rid of the other assign?

pljones · 2026-02-05T18:45:11Z

src/clientsettingsdlg.cpp

+            pSettings->bUseMIDIController = false;
+            chbUseMIDIController->setChecked ( false );
+            SetMIDIControlsEnabled ( false );
+            QMessageBox::warning ( this, tr ( "Could not open MIDI port" ), tr ( "Please check your OS configuration." ) );


Can the JSONRPC code return an error indication in this situation?

pljones · 2026-02-05T18:47:00Z

src/clientsettingsdlg.cpp

+    QObject::connect ( chbUseMIDIController, &QCheckBox::toggled, this, [this] ( bool checked ) {
+        pSettings->bUseMIDIController = checked;
+        SetMIDIControlsEnabled ( checked );
+        pClient->SetSettings ( pSettings );


Only do this on success?

pljones · 2026-02-05T18:47:18Z

src/clientsettingsdlg.cpp

+            QMessageBox::warning ( this, tr ( "Could not open MIDI port" ), tr ( "Please check your OS configuration." ) );
+        }
+
+        emit MIDIControllerUsageChanged ( pSettings->bUseMIDIController );


Only do this if it changed successfully?

pljones · 2026-02-05T19:00:40Z

#3502 (review)
I can't find where you replied to this, @ignotus666 -- I'm pretty sure it was something about supported versions? The example given should work from C++11 (clang++ and g++) according to Github CoPilot. We have a contribution guideline explicitly saying

Maintain C++11 compatibility throughout the code.

so you should be okay to go with it.

ignotus666 · 2026-02-06T06:29:08Z

#3502 (review) I can't find where you replied to this, @ignotus666 -- I'm pretty sure it was something about supported versions? The example given should work from C++11 (clang++ and g++) according to Github CoPilot. We have a contribution guideline explicitly saying

Maintain C++11 compatibility throughout the code.

so you should be okay to go with it.

The issue apparently is with std::unordered_map, which isn't Qt native, so certain Qt versions will have compilation issues. I've used what is claimed to be a Qt-native approach. See what you think.

softins added this to the Release 4.0.0 milestone May 22, 2025

ignotus666 marked this pull request as draft May 22, 2025 14:07

ann0see reviewed Jun 3, 2025

View reviewed changes

src/clientsettingsdlg.cpp Outdated Show resolved Hide resolved

ignotus666 added the needs documentation PRs requiring documentation changes or additions label Jul 1, 2025

pljones added this to Tracking Jan 25, 2026

github-project-automation bot moved this to Triage in Tracking Jan 25, 2026

pljones requested changes Jan 25, 2026

View reviewed changes

github-project-automation bot moved this from Triage to Waiting externally in Tracking Jan 25, 2026

ignotus666 force-pushed the midi-gui-learn branch 2 times, most recently from fe82247 to 7097b29 Compare January 27, 2026 08:21

ignotus666 force-pushed the midi-gui-learn branch 2 times, most recently from 26f4096 to 1f2f394 Compare January 29, 2026 16:02

pljones reviewed Jan 30, 2026

View reviewed changes

src/sound/asio/sound.h Show resolved Hide resolved

pljones reviewed Jan 30, 2026

View reviewed changes

src/clientdlg.cpp Show resolved Hide resolved

pljones reviewed Jan 30, 2026

View reviewed changes

src/clientsettingsdlg.cpp Outdated Show resolved Hide resolved

pljones reviewed Jan 30, 2026

View reviewed changes

src/clientsettingsdlg.cpp Show resolved Hide resolved

pljones reviewed Jan 30, 2026

View reviewed changes

src/clientsettingsdlg.cpp Outdated Show resolved Hide resolved

pljones reviewed Feb 1, 2026

View reviewed changes

src/main.cpp Outdated Show resolved Hide resolved

Refactor: move MIDI parsing to CSettings; misc.

4dacd92