major fixes for MIDI patch change and note undo/redo. Patch change handling was completely broken because of the use of absolute floating point comparisons for time comparison, and serialization/deserialization of patch change property changes was borked because of int/char conversions by stringstream. Note undo/redo would fail for note removal if a note had been moved and/or had its note number changed as the next operation after it was added, because time-based lookup would fail. Similar small changes made for sysex messages, which just needed the musical_time comparisons and nothing else

4 files changed, 110 insertions, 38 deletions

diff --git a/libs/ardour/ardour/midi_model.h b/libs/ardour/ardour/midi_model.h
index 0d11f940b9..3ecfca7d1c 100644
--- a/libs/ardour/ardour/midi_model.h
+++ b/libs/ardour/ardour/midi_model.h
@@ -114,6 +114,8 @@ public:
 		struct NoteChange {
 			NoteDiffCommand::Property property;
 			NotePtr note;
+		        uint32_t note_id; 
+		    
 			union {
 				uint8_t  old_value;
 				TimeType old_time;
@@ -161,6 +163,7 @@ public:
 	private:
 		struct Change {
 			boost::shared_ptr<Evoral::Event<TimeType> > sysex;
+   		        gint sysex_id;
 			SysExDiffCommand::Property property;
 			TimeType old_time;
 			TimeType new_time;
@@ -204,6 +207,7 @@ public:
 		struct Change {
 			PatchChangePtr patch;
 			Property       property;
+		        gint           patch_id;
 			union {
 				TimeType   old_time;
 				uint8_t    old_channel;
diff --git a/libs/ardour/ardour/midi_track.h b/libs/ardour/ardour/midi_track.h
index be209bc0f6..3b75c0a51b 100644
--- a/libs/ardour/ardour/midi_track.h
+++ b/libs/ardour/ardour/midi_track.h
@@ -180,18 +180,29 @@ private:
 
         void filter_channels (BufferSet& bufs, ChannelMode mode, uint32_t mask); 
 
+/* if mode is ForceChannel, force mask to the lowest set channel or 1 if no
+ * channels are set.
+ */
+#define force_mask(mode,mask) (((mode) == ForceChannel) ? (((mask) ? (1<<(ffs((mask))-1)) : 1)) : mask)
+
 	void _set_playback_channel_mode(ChannelMode mode, uint16_t mask) {
+		mask = force_mask (mode, mask);
 		g_atomic_int_set(&_playback_channel_mask, (uint32_t(mode) << 16) | uint32_t(mask));
 	}
         void _set_playback_channel_mask (uint16_t mask) {
+		mask = force_mask (get_playback_channel_mode(), mask);
 		g_atomic_int_set(&_playback_channel_mask, (uint32_t(get_playback_channel_mode()) << 16) | uint32_t(mask));
 	}
 	void _set_capture_channel_mode(ChannelMode mode, uint16_t mask) {
+		mask = force_mask (mode, mask);
 		g_atomic_int_set(&_capture_channel_mask, (uint32_t(mode) << 16) | uint32_t(mask));
 	}
         void _set_capture_channel_mask (uint16_t mask) {
+		mask = force_mask (get_capture_channel_mode(), mask);
 		g_atomic_int_set(&_capture_channel_mask, (uint32_t(get_capture_channel_mode()) << 16) | uint32_t(mask));
 	}
+
+#undef force_mask
 };
 
 } /* namespace ARDOUR*/
diff --git a/libs/ardour/midi_model.cc b/libs/ardour/midi_model.cc
index 4c6f6633d5..5c1f65d96b 100644
--- a/libs/ardour/midi_model.cc
+++ b/libs/ardour/midi_model.cc
@@ -289,6 +289,15 @@ MidiModel::NoteDiffCommand::operator() ()
 
 		for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
 			Property prop = i->property;
+
+			if (!i->note) {
+				/* note found during deserialization, so try
+				   again now that the model state is different.
+				*/
+				i->note = _model->find_note (i->note_id);
+				assert (i->note);
+			}
+
 			switch (prop) {
 			case NoteNumber:
 				if (temporary_removals.find (i->note) == temporary_removals.end()) {
@@ -341,7 +350,7 @@ MidiModel::NoteDiffCommand::operator() ()
 				_removed_notes.push_back (*i);
 			}
 		}
-
+		
 		if (!side_effect_removals.empty()) {
 			cerr << "SER: \n";
 			for (set<NotePtr>::iterator i = side_effect_removals.begin(); i != side_effect_removals.end(); ++i) {
@@ -373,15 +382,25 @@ MidiModel::NoteDiffCommand::undo ()
 		/* notes we modify in a way that requires remove-then-add to maintain ordering */
 		set<NotePtr> temporary_removals;
 
+
+		/* lazily discover any affected notes that were not discovered when
+		 * loading the history because of deletions, etc.
+		 */
+
+		for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
+			if (!i->note) {
+				i->note = _model->find_note (i->note_id);
+				assert (i->note);
+			}
+		}
+				
 		for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
 			Property prop = i->property;
-			switch (prop) {
 
+			switch (prop) {
 			case NoteNumber:
-				if (
-					temporary_removals.find (i->note) == temporary_removals.end() &&
-					find (_removed_notes.begin(), _removed_notes.end(), i->note) == _removed_notes.end()
-					) {
+				if (temporary_removals.find (i->note) == temporary_removals.end() &&
+				    find (_removed_notes.begin(), _removed_notes.end(), i->note) == _removed_notes.end()) {
 
 					/* We only need to mark this note for re-add if (a) we haven't
 					   already marked it and (b) it isn't on the _removed_notes
@@ -396,10 +415,8 @@ MidiModel::NoteDiffCommand::undo ()
 				break;
 
 			case StartTime:
-				if (
-					temporary_removals.find (i->note) == temporary_removals.end() &&
-					find (_removed_notes.begin(), _removed_notes.end(), i->note) == _removed_notes.end()
-					) {
+				if (temporary_removals.find (i->note) == temporary_removals.end() &&
+				    find (_removed_notes.begin(), _removed_notes.end(), i->note) == _removed_notes.end()) {
 
 					/* See above ... */
 
@@ -410,10 +427,8 @@ MidiModel::NoteDiffCommand::undo ()
 				break;
 
 			case Channel:
-				if (
-					temporary_removals.find (i->note) == temporary_removals.end() &&
-					find (_removed_notes.begin(), _removed_notes.end(), i->note) == _removed_notes.end()
-					) {
+				if (temporary_removals.find (i->note) == temporary_removals.end() &&
+				    find (_removed_notes.begin(), _removed_notes.end(), i->note) == _removed_notes.end()) {
 
 					/* See above ... */
 
@@ -453,7 +468,7 @@ MidiModel::NoteDiffCommand::undo ()
 			_model->add_note_unlocked (*i);
 		}
 	}
-
+	
 	_model->ContentsChanged(); /* EMIT SIGNAL */
 }
 
@@ -651,15 +666,13 @@ MidiModel::NoteDiffCommand::unmarshal_change (XMLNode *xml_change)
 	}
 
 	/* we must point at the instance of the note that is actually in the model.
-	   so go look for it ...
+	   so go look for it ... it may not be there (it could have been
+	   deleted in a later operation, so store the note id so that we can
+	   look it up again later).
 	*/
 
 	change.note = _model->find_note (note_id);
-
-	if (!change.note) {
-		warning << "MIDI note #" << note_id << " not found in model - programmers should investigate this" << endmsg;
-		return change;
-	}
+	change.note_id = note_id;
 
 	return change;
 }
@@ -790,6 +803,15 @@ MidiModel::SysExDiffCommand::operator() ()
 			_model->remove_sysex_unlocked (*i);
 		}
 
+		/* find any sysex events that were missing when unmarshalling */
+
+		for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
+			if (!i->sysex) {
+				i->sysex = _model->find_sysex (i->sysex_id);
+				assert (i->sysex);
+			}
+		}
+
 		for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
 			switch (i->property) {
 			case Time:
@@ -811,6 +833,15 @@ MidiModel::SysExDiffCommand::undo ()
 			_model->add_sysex_unlocked (*i);
 		}
 
+		/* find any sysex events that were missing when unmarshalling */
+
+		for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
+			if (!i->sysex) {
+				i->sysex = _model->find_sysex (i->sysex_id);
+				assert (i->sysex);
+			}
+		}
+
 		for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
 			switch (i->property) {
 			case Time:
@@ -899,11 +930,7 @@ MidiModel::SysExDiffCommand::unmarshal_change (XMLNode *xml_change)
 	*/
 
 	change.sysex = _model->find_sysex (sysex_id);
-
-	if (!change.sysex) {
-		warning << "Sys-ex #" << sysex_id << " not found in model - programmers should investigate this" << endmsg;
-		return change;
-	}
+	change.sysex_id = sysex_id;
 
 	return change;
 }
@@ -1033,6 +1060,15 @@ MidiModel::PatchChangeDiffCommand::operator() ()
 			_model->remove_patch_change_unlocked (*i);
 		}
 
+		/* find any patch change events that were missing when unmarshalling */
+
+		for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
+			if (!i->patch) {
+				i->patch = _model->find_patch_change (i->patch_id);
+				assert (i->patch);
+			}
+		}
+
 		set<PatchChangePtr> temporary_removals;
 
 		for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
@@ -1081,6 +1117,15 @@ MidiModel::PatchChangeDiffCommand::undo ()
 			_model->add_patch_change_unlocked (*i);
 		}
 
+		/* find any patch change events that were missing when unmarshalling */
+
+		for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
+			if (!i->patch) {
+				i->patch = _model->find_patch_change (i->patch_id);
+				assert (i->patch);
+			}
+		}
+
 		set<PatchChangePtr> temporary_removals;
 
 		for (ChangeList::iterator i = _changes.begin(); i != _changes.end(); ++i) {
@@ -1207,10 +1252,10 @@ MidiModel::PatchChangeDiffCommand::unmarshal_patch_change (XMLNode* n)
 	XMLProperty* prop;
 	Evoral::event_id_t id;
 	Evoral::MusicalTime time = 0;
-	uint8_t channel = 0;
-	uint8_t program = 0;
+	int channel = 0;
+	int program = 0;
 	int bank = 0;
-
+	
 	if ((prop = n->property ("id")) != 0) {
 		istringstream s (prop->value());
 		s >> id;
@@ -1246,6 +1291,7 @@ MidiModel::PatchChangeDiffCommand::unmarshal_change (XMLNode* n)
 {
 	XMLProperty* prop;
 	Change c;
+	int an_int;
 
 	prop = n->property ("property");
 	assert (prop);
@@ -1255,6 +1301,10 @@ MidiModel::PatchChangeDiffCommand::unmarshal_change (XMLNode* n)
 	assert (prop);
 	Evoral::event_id_t const id = atoi (prop->value().c_str());
 
+	/* we need to load via an int intermediate for all properties that are 
+	   actually uint8_t (char/byte).
+	*/
+
 	prop = n->property ("old");
 	assert (prop);
 	{
@@ -1262,11 +1312,14 @@ MidiModel::PatchChangeDiffCommand::unmarshal_change (XMLNode* n)
 		if (c.property == Time) {
 			s >> c.old_time;
 		} else if (c.property == Channel) {
-			s >> c.old_channel;
+			s >> an_int;
+			c.old_channel = an_int;
 		} else if (c.property == Program) {
-			s >> c.old_program;
+			s >> an_int;
+			c.old_program = an_int;
 		} else if (c.property == Bank) {
-			s >> c.old_bank;
+			s >> an_int;
+			c.old_bank = an_int;
 		}
 	}
 
@@ -1274,19 +1327,23 @@ MidiModel::PatchChangeDiffCommand::unmarshal_change (XMLNode* n)
 	assert (prop);
 	{
 		istringstream s (prop->value ());
+
 		if (c.property == Time) {
 			s >> c.new_time;
 		} else if (c.property == Channel) {
-			s >> c.new_channel;
+			s >> an_int;
+			c.new_channel = an_int;
 		} else if (c.property == Program) {
-			s >> c.new_program;
+			s >> an_int;
+			c.new_program = an_int;
 		} else if (c.property == Bank) {
-			s >> c.new_bank;
+			s >> an_int;
+			c.new_bank = an_int;
 		}
 	}
 
 	c.patch = _model->find_patch_change (id);
-	assert (c.patch);
+	c.patch_id = id;
 
 	return c;
 }
@@ -1584,7 +1641,7 @@ MidiModel::write_lock()
 	assert (ms);
 
 	assert (!ms->mutex().trylock ());
-	return WriteLock(new WriteLockImpl(NULL, _lock, _control_lock));
+	return WriteLock(new WriteLockImpl(0, _lock, _control_lock));
 }
 
 int
diff --git a/libs/ardour/midi_track.cc b/libs/ardour/midi_track.cc
index 0f1b2b52af..1a618a01dd 100644
--- a/libs/ardour/midi_track.cc
+++ b/libs/ardour/midi_track.cc
@@ -169,7 +169,7 @@ MidiTrack::set_state (const XMLNode& node, int version)
 		playback_channel_mode = ChannelMode (string_2_enum(prop->value(), playback_channel_mode));
 	}
 	if ((prop = node.property ("capture-channel-mode")) != 0) {
-		playback_channel_mode = ChannelMode (string_2_enum(prop->value(), capture_channel_mode));
+		capture_channel_mode = ChannelMode (string_2_enum(prop->value(), capture_channel_mode));
 	}
 	if ((prop = node.property ("channel-mode")) != 0) {
 		/* 3.0 behaviour where capture and playback modes were not separated */