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.

@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.

Nice!

I'd like to open a discussion:

1. Do you think a separate MIDI tab is useful or does it add clutter?
2. 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)

@ann0see

1. 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.
2. "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.

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

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.)

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.

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

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.

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.

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

• 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.

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.

On an unrelated note, could you rebase this PR to the current upstream main? Thanks!

Apart from that one change needed, all looking good. I've only tested on Windows so far.

@softins Done. It needed to be changed to -1 in settings.h too. Did you test pick-up mode? I'm curious as to how that is working with a commercial controller - I only have a DIY one I made myself to test with.

It needed to be changed to -1 in settings.h too.

No it didn't. iMidiMuteMyself is already qualified by bMidiMuteMyselfEnabled. It is only the value passed in to SetMIDIControllerMapping() that needs to be -1 when mute myself is not enabled.

Did you test pick-up mode? I'm curious as to how that is working with a commercial controller - I only have a DIY one I made myself to test with.

No, I haven't tried that yet. I was wondering earlier what it was, then I found the "What's this" for it :)

No it didn't. iMidiMuteMyself is already qualified by bMidiMuteMyselfEnabled. It is only the value passed in to SetMIDIControllerMapping() that needs to be -1 when mute myself is not enabled.

Oops - you're right. Undid that change.

Did you test pick-up mode? I'm curious as to how that is working with a commercial controller - I only have a DIY one I made myself to test with.

No, I haven't tried that yet. I was wondering earlier what it was, then I found the "What's this" for it :)

I've tried it now, and it seems to work fine.

I've also compiled and tested it successfully on macOS. Although the checkboxes appear to be smaller than normal.

I've just read through the historical conversation now. Thanks to @pljones and @ignotus666 for working together on this; I ignored it till very recently, as I was busy with other items.

I'm ready in principle to approve this. I've tested it on Windows and macOS, but not on Linux, but I'm happy that the main development was done on Linux anyway.

I'm conscious that I haven't tried the JSONRPC extensions that support this. @ignotus666, how have you tested them, and do you have any scripts or test harness?

@softins I'm using a test script that Claude kindly made for me. Put it in /tools and run bash tools/test_midi_rpc.sh from the root directory:

test_midi_rpc.sh

@softins I'm using a test script that Claude kindly made for me. Put it in /tools and run bash tools/test_midi_rpc.sh from the root directory:

Great, thanks! That all seems to work nicely.

Thank you very much!

@ignotus666 I assume this is fully documented on the website?

@ann0see yes, I think it's all been covered.