Add MIDI GUI tab and learn function #3502

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

linux/debian/rules

pljones · 2026-01-31T10:42:08Z

src/sound/asio/sound.h


    virtual float GetInOutLatencyMs() { return fInOutLatencyMs; }

+    // MIDI port toggle


As these are in CSoundBase, do they need to be in each inheriting class?

src/sound/soundbase.cpp

pljones · 2026-01-31T10:45:28Z

src/sound/soundbase.cpp

                        const CMidiCtlEntry& cCtrl  = aMidiCtls[vMIDIPaketBytes[1]];
                        const int            iValue = vMIDIPaketBytes[2];
+                        emit                 MidiCCReceived ( vMIDIPaketBytes[1] );
                        ;


And this shouldn't be here -- clang should have caught it.

src/client.cpp

src/clientdlg.h

src/clientrpc.cpp

ignotus666 · 2026-01-31T13:40:23Z

Removing the

    virtual void EnableMIDI ( bool bEnable );
    virtual bool IsMIDIEnabled() const;

declarations from the Asio, JACK and macOS headers prevents them from compiling. I added them back in as I don't really know what else to do.

ignotus666 · 2026-02-01T06:53:17Z

src/settings.cpp

        pClient->SetAudioQuality ( static_cast<EAudioQuality> ( iValue ) );
    }

+    // MIDI settings


@pljones would you prefer a loop here too?

Map from "whatever" to [this]() { GetNumericIniSet ( IniXMLDocument, "client", "whatever", minWhatever, maxWhatever, iValue ); this->whatever = iValue; }; and then loop over an array of them?

Or do "midichannel" and "usemidicontroller" separately, with everything in the loop with the 0, 127 hardcoded... I'd go with that, I think.

pljones · 2026-02-01T10:07:54Z

src/main.cpp

                             bMuteMeInPersonalMix );

-            // load settings from init-file (command line options override)
+            // Create Settings with the client pointer


The deleted line is correct -- but for the Settings.Load line.

pljones · 2026-02-01T10:24:13Z

src/main.cpp

+            CSoundBase::ParseMIDICommandLineParams ( strMIDISetup, Settings );
+            if ( Settings.bUseMIDIController )
+            {
+                Client.ApplyMIDIMapping ( Settings.GetMIDIMapString() );


This now seems counter-intuitive.

Maybe CSoundBase::ParseMIDICommandLineParams ( strMIDISetup, Settings ); should be in settings (it wouldn't need to keep calling into Settings, then).

Given the command line arg has been parsed out into separate values in Settings, turning it back into a string so that it can be parsed out into separate settings again... Client already has a Settings reference, so it can go read the values.

See other comment about this.

pljones · 2026-02-01T10:26:17Z

src/settings.h

        bWindowWasShownSettings ( false ),
        bWindowWasShownChat ( false ),
        bWindowWasShownConnect ( false ),
        bOwnFaderFirst ( false ),


You need to follow this style for all the new entries.

pljones · 2026-02-01T10:26:54Z

src/settings.h

    bool       bOwnFaderFirst;

+    // MIDI settings
+    int     midiChannel        = 0; // Default MIDI channel 0


These really should all be iMidi... by convention.

pljones · 2026-02-01T10:27:22Z

src/settings.h

+    int     midiMuteOffset     = 0;
+    int     midiMuteCount      = 0;
+    bool    bUseMIDIController = false;
+    QString GetMIDIMapString() const;


As other note, I'd like this to go away.

pljones · 2026-02-01T10:51:23Z

src/clientrpc.cpp

+    /// @param {object} params - Any subset of MIDI settings fields to set.
+    /// @result {string} result - Always "ok".
+    pRpcServer->HandleMethod ( "jamulusclient/setMidiSettings", [=] ( const QJsonObject& params, QJsonObject& response ) {
+        if ( params.contains ( "bUseMIDIController" ) )


Do I trust Gemini to write C++?

std::unordered_map<String, std::function<void(String)>> setters = { { "bUseMIDIController", [this](String name) { m_pSettings->bUseMIDIController = params[name].toBool(); }, { "midiChannel", [this](String name) { m_pSettings->midiChannel = params[name].toInt(); }, ... }; // Iterate through every defined setter in your map for (const auto& [name, setter] : setters) { // Check if the incoming params collection actually has this key if (params.contains(name)) { // Execute the lambda: [this](String n) { ... } setter(name); } }

(And it followed Jamulus coding style better than I did...)

pljones · 2026-02-01T10:55:32Z

src/sound/soundbase.cpp

 * MIDI handling                                                                *
 \******************************************************************************/
+
+void CSoundBase::ParseMIDICommandLineParams ( const QString& strMIDISetup, CClientSettings& Settings )


Yeah, sorry -- move to CSettings.

pljones · 2026-02-01T10:56:30Z

src/sound/soundbase.cpp

+        Settings.bUseMIDIController = true;
+    }
+}
 void CSoundBase::ParseCommandLineArgument ( const QString& strMIDISetup )


I think you need to make sure there's only one way of doing this. This existing method is "the one true agreed way" to parse the command line.

Ideally it needs to move to CSettings and only update the values you're now storing.

Then CClient should tell CSound what to do when it initialises:

set up the initial MIDI mapping

check whether MIDI needs to be enabled and, if so, turn it on

The UI and RPC should tell CClient what's going on and it should update CSettings - the UI and RPC shouldn't directly update Settings -- CSettings is meant to be fairly dumb with any logic in CClient or CServer. Currently, that's not generally how things are done though.

pljones · 2026-02-01T10:59:26Z

src/sound/soundbase.cpp

+void CSoundBase::SetMIDIMapping ( const QString& strMIDISetup )
+{
+    // Parse the MIDI mapping
+    ParseCommandLineArgument ( strMIDISetup );


I don't like passing this around as a string.

Actually, with the proposed change above, the whole thing can go.

pljones · 2026-02-01T11:11:08Z

src/main.cpp

                             iQosNumber,
                             strConnOnStartupAddress,
-                             strMIDISetup,
+                             "", // Always start with empty MIDI


So change the method signature.

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 prospose to 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.

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