Merge pull request #1159 from kushview/midi-monitor-display

fix: display correct note name in midi monitor and elsewhere

Author: mfisher31

CHANGELOG.md

@@ -2,8 +2,13 @@
 
 ## [1.2.x]
 
+### Changed
+- Note names throughout the UI now use scientific pitch notation (middle C = C4), matching the convention used by most DAWs.
+
 ### Fixed
 - Blocks embedded in the graph return to their embedded state when the plugin window is closed.
+- MIDI Monitor note names displayed two octaves too high.
+- MIDI Monitor logged Start/Stop/Continue messages twice.
 
 ## [1.2.0]
 

src/nodes/midimonitor.cpp

@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: GPL-3.0-or-later
 
 #include "nodes/midimonitor.hpp"
+#include "utils.hpp"
 
 namespace element {
 
@@ -72,6 +73,32 @@ void MidiMonitorNode::clearMessages()
     messagesLogged();
 }
 
+String MidiMonitorNode::describe (const MidiMessage& msg)
+{
+    if (msg.isMidiClock())
+        return {};
+
+    if (msg.isMidiStart())
+        return "Start";
+    if (msg.isMidiStop())
+        return "Stop";
+    if (msg.isMidiContinue())
+        return "Continue";
+
+    if (msg.isNoteOn() || msg.isNoteOff())
+    {
+        String text;
+        text << (msg.isNoteOn() ? "Note On " : "Note Off ")
+             << Util::noteValueToString (msg.getNoteNumber())
+             << " (" << msg.getNoteNumber() << ")"
+             << " Velocity " << (int) msg.getVelocity()
+             << " Channel " << msg.getChannel();
+        return text;
+    }
+
+    return msg.getDescription();
+}
+
 void MidiMonitorNode::timerCallback()
 {
     midiTemp.clear();
@@ -80,48 +107,13 @@ void MidiMonitorNode::timerCallback()
         return;
 
     int numLogged = 0;
-    String text;
     for (auto m : midiTemp)
     {
-        auto msg = m.getMessage();
-        if (msg.isMidiClock())
-        {
-            text.clear();
+        const auto text = describe (m.getMessage());
+        if (text.isEmpty())
             continue;
-        }
-
-        if (msg.isMidiStart())
-            text << "Start";
-        else if (msg.isMidiStop())
-            text << "Stop";
-        else if (msg.isMidiContinue())
-            text << "Continue";
-
-        if (text.isNotEmpty())
-        {
-            midiLog.add (text);
-        }
-        else if (msg.isNoteOn())
-        {
-            text.clear();
-            text << "Note On "
-                 << msg.getMidiNoteName (msg.getNoteNumber(), true, true, 5)
-                 << " (" << msg.getNoteNumber() << ") "
-                 << " Velocity " << msg.getVelocity()
-                 << " Channel " << msg.getChannel();
-        }
-        else if (msg.isNoteOff())
-        {
-            text.clear();
-            text << "Note Off "
-                 << msg.getMidiNoteName (msg.getNoteNumber(), true, true, 5)
-                 << " (" << msg.getNoteNumber() << ") "
-                 << " Velocity " << msg.getVelocity()
-                 << " Channel " << msg.getChannel();
-        }
-
-        midiLog.add (text.isNotEmpty() ? text : msg.getDescription());
-        text.clear();
+
+        midiLog.add (text);
         ++numLogged;
     }
 

src/nodes/midimonitor.hpp

@@ -58,6 +58,14 @@ class MidiMonitorNode : public MidiFilterNode,
 
     const StringArray& logger() const { return midiLog; }
 
+    /** Formats a MIDI message the way it appears in the monitor log.
+
+        @param msg the message to describe
+        @return the display text, or an empty string for messages that are not
+                logged (MIDI clock)
+    */
+    static juce::String describe (const juce::MidiMessage& msg);
+
 private:
     friend class MidiMonitorNodeEditor;
     friend class MidiMonitorBlock;

src/ui/virtualkeyboardview.cpp

@@ -4,6 +4,7 @@
 #include <element/audioengine.hpp>
 #include "ui/guicommon.hpp"
 #include "ui/virtualkeyboardview.hpp"
+#include "utils.hpp"
 
 namespace element {
 
@@ -36,6 +37,7 @@ static int getOctaveOffsetForKeyPress (const KeyPress& key, const int fallback =
 VirtualKeyboardComponent::VirtualKeyboardComponent (MidiKeyboardState& s, Orientation o)
     : MidiKeyboardComponent (s, o)
 {
+    setOctaveForMiddleC (Util::middleCOctave);
 }
 
 void VirtualKeyboardComponent::setKeypressOctaveOffset (int offset)

src/utils.hpp

@@ -34,22 +34,36 @@ inline static String secondsToString (const double input)
     return minutesToString (input / 60.0);
 }
 
+/** The octave number given to middle C (MIDI note 60) everywhere note names are
+    displayed or parsed.
+
+    Element uses scientific pitch notation, middle C = C4, which is what most DAWs
+    show. Anything naming a MIDI note should go through noteValueToString() or use
+    this constant rather than passing its own number to juce::MidiMessage.
+*/
+inline constexpr int middleCOctave = 4;
+
+/** Converts a MIDI note number to its note name, e.g. "C4", "F#2", "A0".
+
+    @param value the MIDI note number, rounded to the nearest integer
+    @return the note name including its octave number
+*/
 inline static String noteValueToString (double value)
 {
-    return juce::MidiMessage::getMidiNoteName (juce::roundToInt (value), true, true, 3);
+    return juce::MidiMessage::getMidiNoteName (juce::roundToInt (value), true, true, middleCOctave);
 }
 
-/** Parses a note name (e.g. "C3", "F#2", "Eb4") or a raw MIDI number into a
+/** Parses a note name (e.g. "C4", "F#2", "Eb4") or a raw MIDI number into a
     MIDI note number 0-127.
 
-    Uses the same octave-for-middle-C convention as noteValueToString (middle C = C3),
+    Uses the same octave-for-middle-C convention as noteValueToString (middle C = C4),
     so this is the inverse of that function.
 
     @param text             the text to parse
     @param octaveForMiddleC the octave number assigned to middle C (note 60)
     @return the MIDI note number 0-127, or -1 if the text cannot be parsed
 */
-inline static int noteValueFromString (const juce::String& text, int octaveForMiddleC = 3)
+inline static int noteValueFromString (const juce::String& text, int octaveForMiddleC = middleCOctave)
 {
     const auto trimmed = text.trim();
     if (trimmed.isEmpty())

test/CMakeLists.txt

@@ -77,6 +77,7 @@ add_test(NAME "MidiMappingSessionTests" COMMAND test_element --run_test=MidiMapp
 add_test(NAME "MappingEngineTests" COMMAND test_element --run_test=MappingEngineTests)
 add_test(NAME "MappingLearnTests" COMMAND test_element --run_test=MappingLearnTests)
 add_test(NAME "MidiClockTest" COMMAND test_element --run_test=MidiClockTest)
+add_test(NAME "MidiMonitorTests" COMMAND test_element --run_test=MidiMonitorTests)
 add_test(NAME "MidiProgramMapTests" COMMAND test_element --run_test=MidiProgramMapTests)
 add_test(NAME "MidiProgramTests" COMMAND test_element --run_test=MidiProgramTests)
 add_test(NAME "MidiScriptTests" COMMAND test_element --run_test=MidiScriptTests)

test/MidiMonitorTests.cpp

@@ -0,0 +1,62 @@
+// SPDX-FileCopyrightText: Copyright (C) Kushview, LLC.
+// SPDX-License-Identifier: GPL-3.0-or-later
+
+#include <boost/test/unit_test.hpp>
+
+#include "nodes/midimonitor.hpp"
+#include "utils.hpp"
+
+using namespace element;
+using namespace juce;
+
+BOOST_AUTO_TEST_SUITE (MidiMonitorTests)
+
+BOOST_AUTO_TEST_CASE (NoteOnUsesMiddleCAsC4)
+{
+    const auto text = MidiMonitorNode::describe (MidiMessage::noteOn (1, 60, 1.0f));
+    BOOST_REQUIRE (text.startsWith ("Note On C4"));
+    BOOST_REQUIRE (text.contains ("(60)"));
+    BOOST_REQUIRE (text.contains ("Channel 1"));
+}
+
+BOOST_AUTO_TEST_CASE (NoteNamesAtOctaveEdges)
+{
+    BOOST_REQUIRE (MidiMonitorNode::describe (MidiMessage::noteOn (1, 21, 1.0f))
+                       .startsWith ("Note On A0"));
+    BOOST_REQUIRE (MidiMonitorNode::describe (MidiMessage::noteOn (1, 0, 1.0f))
+                       .startsWith ("Note On C-1"));
+    BOOST_REQUIRE (MidiMonitorNode::describe (MidiMessage::noteOn (1, 127, 1.0f))
+                       .startsWith ("Note On G9"));
+}
+
+BOOST_AUTO_TEST_CASE (NoteOff)
+{
+    const auto text = MidiMonitorNode::describe (MidiMessage::noteOff (2, 60));
+    BOOST_REQUIRE (text.startsWith ("Note Off C4"));
+    BOOST_REQUIRE (text.contains ("Channel 2"));
+}
+
+BOOST_AUTO_TEST_CASE (ClockIsNotLogged)
+{
+    BOOST_REQUIRE (MidiMonitorNode::describe (MidiMessage::midiClock()).isEmpty());
+}
+
+BOOST_AUTO_TEST_CASE (TransportMessagesLogOnce)
+{
+    BOOST_REQUIRE_EQUAL (MidiMonitorNode::describe (MidiMessage::midiStart()).toStdString(), "Start");
+    BOOST_REQUIRE_EQUAL (MidiMonitorNode::describe (MidiMessage::midiStop()).toStdString(), "Stop");
+    BOOST_REQUIRE_EQUAL (MidiMonitorNode::describe (MidiMessage::midiContinue()).toStdString(), "Continue");
+}
+
+BOOST_AUTO_TEST_CASE (AgreesWithNoteValueConversions)
+{
+    BOOST_REQUIRE_EQUAL (Util::middleCOctave, 4);
+    BOOST_REQUIRE_EQUAL (Util::noteValueToString (60).toStdString(), "C4");
+    BOOST_REQUIRE_EQUAL (Util::noteValueToString (21).toStdString(), "A0");
+    BOOST_REQUIRE_EQUAL (Util::noteValueFromString ("C4"), 60);
+    BOOST_REQUIRE_EQUAL (Util::noteValueFromString ("A0"), 21);
+    BOOST_REQUIRE_EQUAL (Util::noteValueFromString ("C-1"), 0);
+    BOOST_REQUIRE_EQUAL (Util::noteValueFromString ("G9"), 127);
+}
+
+BOOST_AUTO_TEST_SUITE_END()