Add MIDI GUI tab and learn function #3502
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
|
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. |
Yes, that was already there.
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. |
|
#3502 (review)
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. |
ignotus666
force-pushed
the
midi-gui-learn
branch
from
4bd42e1 to
019e240
Compare
ignotus666
force-pushed
the
midi-gui-learn
branch
from
a620390 to
9ec1345
Compare
|
Browsing through the documentation to see what would need updating, I came across instructions on using the 'd' (device) option on the next-release branch, which I'd missed until now - I'd seen it and included it in the parsing while going through the code, but hadn't given it much thought. |
Yes, I added it. I think at this stage of the upcoming release process there isn't a good reason to add a UI chooser. Just let the user use the The selection and opening of the MIDI device(s) is only done once at startup. It could be a lot of complication to provide the user a means to close and open different MIDI devices while the program is running. It would also be Windows only (yes, I know the If there's demand, we could maybe revisit it for 4.0.0 |
ignotus666
force-pushed
the
midi-gui-learn
branch
from
9ec1345 to
bef04b1
Compare
Would need to make sure that specifying |
Fair enough.
Hmm, good point. Right now, it's resetting everything else to 0 (offset/1st note) and 1 (count). Same happens for other options - as you say, those not specified on the command line should be left alone. Will look into correcting that. |
ignotus666
force-pushed
the
midi-gui-learn
branch
from
bef04b1 to
2b92102
Compare
|
Fixed the command line override - settings not included in the command line options are not altered if they were set previously in the GUI. I also fixed a bug where the MIDI device was not getting picked up at launch when set with the 'd' option. |
Sorry, I don't agree with this at this time. This would unnecessarily complicate the already complicated explanation we have of how things work. We've got no pressing need for this PR -- it's in draft, it's not blocking 3.12.0 yet. I'd rather we got the UI support in and kept the existing "if you put a mention of an option on the command line, you don't get the config file entry" rule entirely intact, rather than trying to shoehorn a special case in here. |
ignotus666
force-pushed
the
midi-gui-learn
branch
from
2b92102 to
f8dfe4f
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. |
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