Add MIDI GUI tab and learn function#3502
Add MIDI GUI tab and learn function#3502ignotus666 wants to merge 1 commit intojamulussoftware:mainfrom
Conversation
|
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:
|
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. |
ignotus666
added
the
needs documentation
|
OK just stumbled onto this. As this is a new feature, the following need to be adhered to:
(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
left a comment
pljones
left a comment
There was a problem hiding this comment.
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.
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 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 |
ignotus666
force-pushed
the
midi-gui-learn
branch
2 times, most recently
from
fe82247 to
7097b29
Compare
Done.
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. |
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 |
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. |
The problem is that in the TODO blocks, the TODO lines don't have a space after |
ignotus666
force-pushed
the
midi-gui-learn
branch
2 times, most recently
from
26f4096 to
1f2f394
Compare
| int& iMidiMuteCount, | ||
| int& iMidiMuteMyself, | ||
| bool& bUseMIDIController, | ||
| QString* strMIDIDevice ) |
There was a problem hiding this comment.
Shouldn't this be a QString& so it's safely settable? I can't remember the syntax well enough to be clear but writing to a pointer feels dangerous. Of course, if this is just the old code moved... I guess it works...
There was a problem hiding this comment.
When used it's checked before dereferencing:
if ( cType == 'd' )
{
if ( strMIDIDevice != nullptr ) // Check before writing
{
*strMIDIDevice = sParm.mid ( 1 );
}
continue;
}
So it should be safe.
There was a problem hiding this comment.
I'm more worried about:
QString str = QString("abc");
QString* _str = &str;
*_str = QString("really long string");
That's like trying QString("abc") = QString("really long string") isn't it?
Saying
QString str = QString("abc");
QString& _str = &str;
&_str = QString("really long string");
gets you str == QString("really long string"), I think. But, like I say, I'd have to run it to check (or see what the compiler says).
| { | ||
| if ( GetNumericIniSet ( IniXMLDocument, "client", "midichannel", 0, 16, iValue ) ) | ||
| iMidiChannel = iValue; | ||
|
|
There was a problem hiding this comment.
Why doesn't the MIDI device round trip to settings?
There was a problem hiding this comment.
Do you mean why isn't it saved? There's no way to set it other than by using it on the command line, it's a per-session option.
There was a problem hiding this comment.
Ah... right, so as there's only the command line and that prevents reading or writing the settings, it's not there.
Mmm, I really would like it to be in the UI in that case, with a drop-down... But then I'd want it cross-platform... and that's not for this change. So OK, this'll do for now.
Please feel free to do more work here 😄 !
OK fair enough. In that case, this GUI implementation would need to provide the equivalent of |
Settings are applied before opening MIDI devices. I've come up with this (not finished yet but mostly working):
Is this something you guys would agree to, or how should I proceed? |
Looks good to me. I'll be happy to compile it and try it out when it's ready to do that. |
|
Yeah but as I say, if it's Windows only... I might prefer to split out the work on the UI bit and have it given the UI and cross-platform support. I'm torn... but probably more towards getting cross-platform support. |
|
I know you suggested adding it in a future update but I went ahead and had a go at adding MIDI device discovery/selection for Linux and macOS too (not to this PR yet - you can find builds here).
I've been testing on Linux and Windows (non-Jack), but it'd be great if someone could do tests on macOS. |
|
Wow. Nice! I'll try to get time over the weekend. |
|
@pljones here's what I came up with - not exactly what you proposed but closer I think: 
|
|
That's actually closer to what I had in mind -- Markdown wasn't letting me get everything right. |
|
Here's what it looks like on Windows 11 from the That's not your branch, though, that's the last one on the PR. So I'll see if the next one looks better. It might need the "First CC" and "Count" turned into column headings or something. 
That's not going to work -- the padding around each section is too much compared with the actual controls on screen. You can see how busy the Audio/Network Setup tab is in comparison, yet it all fits: |
|
You can check out the latest version here. I just pushed it so it'll take a while to build though. |
|
Could you make sure you've got the latest upstream and rebase -- I'm concerned over the iOS failure. |
|
Ah, that's great! |
|
It shouldn't fail on the PR. I can't see why it fails on yours, though... Try clearing any worker caches you've got in your repo. |
|
Ugh - this definitely isn't down to you.... 
What have they done to the tabs... They're trying to do what Firefox did and turn them into floating buttons! I think it's dreadful. I had to use userChrome on Firefox to fix it. |
I just went with the old behaviour - aside from the default 70 value for faders, the rest were 0. I see what you mean though, so maybe we can make the default "disabled" (represented by "--" or something similar to fit)? |
I've been holding back on adding the latest MIDI device selection code to this PR pending a go-ahead by you guys. If it's ok by you I'll add it and I'm fairly confident the iOS build won't fail. |
|
I think the default "count" was also zero, though? Anyway, please push what's here to the PR (do rebase and squash if needed). I don't think you can put "--" into the spinner (it would look odd) but on the heading line you could add a checkbox (it doesn't need a visible label) and have that enable the settings for that control? (Breaks your panel-based approach, though.) EDIT: Mmm, bold labelled checkbox with "Fader", or whatever as the label, rather than the panel border title? If you can get a checkbox in as the title, even better! If you do change the layout, you could change "MIDI-in" to "MIDI Control" and move the device selector next to it, labelled "Device", perhaps, to save a bit of space. (Or are MIDI device names long sometimes?) |
|
I'm pretty certain "count" was 1 as default, but what about this: Make the default count zero, which means that the affected item is disabled in practice ("zero controls in this category will react to a MIDI message"). This saves us cluttering up the UI with more items. It could be explained in the tooltip and manual - "Setting 'count' to zero will disable that control". |
|
Mute Myself doesn't have a count though. Somehow it's not active with the (And it's bad UX to conflate concepts like "count" and "active".) ... even if it's stored in the settings as a count of zero (and we store a dummy count for Mute Myself), it should display separately. |
My reasoning is that a "count" of zero is simply that - zero controls will react to it. I don't think it undermines the logic attached to the "count" concept and provides a way to "disable" controls (set to whatever note) that is far easier to implement than having a bool flag and checkbox for every control... As for Mute Myself: make the default CC -1. That way it doesn't get mapped and won't react to anything. |
|
I know it makes the work easier. @gilgongo, as our resident UI/UX arbitrator, what do you reckon? Is it okay to go with this? |
ignotus666
force-pushed
the
midi-gui-learn
branch
from
f8dfe4f to
03adfbc
Compare
ignotus666
force-pushed
the
midi-gui-learn
branch
from
03adfbc to
0bd87d6
Compare
|
Here's what Gemini made of it: Full text of prompt:
It sounds like Gemini thinks there's a "Checkable Groupbox" -- but AI can make mistakes (and I wouldn't trust Gemini with coding, to be honest). The winning point for me is one I didn't think of, though -- if I disable a control temporarily, I don't want to have to go through the process of setting the value again to get it working. Surprisingly fast response by one of my local LLMs under Ollama: |
|
Fair enough, it makes good points. Will get to work on it, and yes, checkable groupboxes are a thing. I'll make one for Mute Myself too to make it consistent. iOS build succeeded BTW. |
4 participants
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
--ctrlmidichcommand 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
--ctrlmidichparameter 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