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

9 files changed, 254 insertions, 76 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 */
diff --git a/libs/evoral/evoral/ControlSet.hpp b/libs/evoral/evoral/ControlSet.hpp
index 431885112b..716d199fec 100644
--- a/libs/evoral/evoral/ControlSet.hpp
+++ b/libs/evoral/evoral/ControlSet.hpp
@@ -65,7 +65,7 @@ public:
 
 	void what_has_data(std::set<Parameter>&) const;
 
-	Glib::Threads::Mutex& control_lock() const { return _control_lock; }
+        Glib::Threads::Mutex& control_lock() const { return _control_lock; }
 
 protected:
 	virtual void control_list_marked_dirty () {}
diff --git a/libs/evoral/evoral/PatchChange.hpp b/libs/evoral/evoral/PatchChange.hpp
index 82e2023941..b1a42c6f2e 100644
--- a/libs/evoral/evoral/PatchChange.hpp
+++ b/libs/evoral/evoral/PatchChange.hpp
@@ -119,7 +119,7 @@ public:
 	uint8_t channel () const { return _program_change.buffer()[0] & 0xf; }
 
 	inline bool operator< (const PatchChange<Time>& o) const {
-		if (time() != o.time()) {
+		if (!musical_time_equal (time(), o.time())) {
 			return time() < o.time();
 		}
 
@@ -131,7 +131,7 @@ public:
 	}
 
 	inline bool operator== (const PatchChange<Time>& o) const {
-		return (time() == o.time() && program() == o.program() && bank() == o.bank());
+		return (musical_time_equal (time(), o.time()) && program() == o.program() && bank() == o.bank());
 	}
 
 	/** The PatchChange is made up of messages() MIDI messages; this method returns them by index.
@@ -165,4 +165,10 @@ private:
 
 }
 
+template<typename Time>
+std::ostream& operator<< (std::ostream& o, const Evoral::PatchChange<Time>& p) {
+	o << "Patch Change " << p.id() << " @ " << p.time() << " bank " << (int) p.bank() << " program " << (int) p.program();
+	return o;
+}
+
 #endif
diff --git a/libs/evoral/evoral/Sequence.hpp b/libs/evoral/evoral/Sequence.hpp
index 815d02f980..26bef20232 100644
--- a/libs/evoral/evoral/Sequence.hpp
+++ b/libs/evoral/evoral/Sequence.hpp
@@ -69,8 +69,7 @@ protected:
 	struct WriteLockImpl {
 		WriteLockImpl(Glib::Threads::RWLock& s, Glib::Threads::Mutex& c)
 			: sequence_lock(new Glib::Threads::RWLock::WriterLock(s))
-			, control_lock(new Glib::Threads::Mutex::Lock(c))
-		{ }
+			, control_lock(new Glib::Threads::Mutex::Lock(c)) { }
 		~WriteLockImpl() {
 			delete sequence_lock;
 			delete control_lock;
@@ -88,7 +87,7 @@ public:
 	typedef boost::shared_ptr<WriteLockImpl>            WriteLock;
 
 	virtual ReadLock  read_lock() const { return ReadLock(new Glib::Threads::RWLock::ReaderLock(_lock)); }
-	virtual WriteLock write_lock()      { return WriteLock(new WriteLockImpl(_lock, _control_lock)); }
+        virtual WriteLock write_lock()      { return WriteLock(new WriteLockImpl(_lock, _control_lock)); }
 
 	void clear();
 
@@ -126,7 +125,7 @@ public:
 	struct EarlierNoteComparator {
 		inline bool operator()(const boost::shared_ptr< const Note<Time> > a,
 		                       const boost::shared_ptr< const Note<Time> > b) const {
-			return a->time() < b->time();
+			return musical_time_less_than (a->time(), b->time());
 		}
 	};
 
@@ -134,6 +133,7 @@ public:
 		typedef const Note<Time>* value_type;
 		inline bool operator()(const boost::shared_ptr< const Note<Time> > a,
 		                       const boost::shared_ptr< const Note<Time> > b) const {
+			return musical_time_greater_than (a->time(), b->time());
 			return a->time() > b->time();
 		}
 	};
@@ -142,7 +142,7 @@ public:
 		typedef const Note<Time>* value_type;
 		inline bool operator()(const boost::shared_ptr< const Note<Time> > a,
 		                       const boost::shared_ptr< const Note<Time> > b) const {
-			return a->end_time() > b->end_time();
+			return musical_time_less_than (a->end_time(), b->end_time());
 		}
 	};
 
@@ -186,7 +186,7 @@ public:
 
 	struct EarlierSysExComparator {
 		inline bool operator() (constSysExPtr a, constSysExPtr b) const {
-			return a->time() < b->time();
+			return musical_time_less_than (a->time(), b->time());
 		}
 	};
 
@@ -199,7 +199,7 @@ public:
 
 	struct EarlierPatchChangeComparator {
 		inline bool operator() (constPatchChangePtr a, constPatchChangePtr b) const {
-			return a->time() < b->time();
+			return musical_time_less_than (a->time(), b->time());
 		}
 	};
 
diff --git a/libs/evoral/evoral/types.hpp b/libs/evoral/evoral/types.hpp
index 7bdbdc7a2e..000b79bb94 100644
--- a/libs/evoral/evoral/types.hpp
+++ b/libs/evoral/evoral/types.hpp
@@ -43,6 +43,33 @@ static inline bool musical_time_equal (MusicalTime a, MusicalTime b) {
 	return fabs (a - b) <= (1.0/1920.0);
 }
 
+static inline bool musical_time_less_than (MusicalTime a, MusicalTime b) {
+	/* acceptable tolerance is 1 tick. Nice if there was no magic number here */
+	if (fabs (a - b) <= (1.0/1920.0)) {
+		return false; /* effectively identical */
+	} else {
+		return a < b;
+	}
+}
+
+static inline bool musical_time_greater_than (MusicalTime a, MusicalTime b) {
+	/* acceptable tolerance is 1 tick. Nice if there was no magic number here */
+	if (fabs (a - b) <= (1.0/1920.0)) {
+		return false; /* effectively identical */
+	} else {
+		return a > b;
+	}
+}
+
+static inline bool musical_time_greater_or_equal_to (MusicalTime a, MusicalTime b) {
+	/* acceptable tolerance is 1 tick. Nice if there was no magic number here */
+	if (fabs (a - b) <= (1.0/1920.0)) {
+		return true; /* effectively identical, note the "or_equal_to" */
+	} else {
+		return a >= b;
+	}
+}
+
 /** Type of an event (opaque, mapped by application) */
 typedef uint32_t EventType;
 
diff --git a/libs/evoral/src/Sequence.cpp b/libs/evoral/src/Sequence.cpp
index 71bdcb7c03..738d5a19c9 100644
--- a/libs/evoral/src/Sequence.cpp
+++ b/libs/evoral/src/Sequence.cpp
@@ -720,25 +720,27 @@ void
 Sequence<Time>::remove_note_unlocked(const constNotePtr note)
 {
 	bool erased = false;
+	bool id_matched = false;
 
-	_edited = true;
-
-	DEBUG_TRACE (DEBUG::Sequence, string_compose ("%1 remove note %2 @ %3\n", this, (int)note->note(), note->time()));
+	DEBUG_TRACE (DEBUG::Sequence, string_compose ("%1 remove note #%2 %3 @ %4\n", this, note->id(), (int)note->note(), note->time()));
 
-	typename Sequence<Time>::Notes::iterator i = note_lower_bound(note->time());
-	while (i != _notes.end() && (*i)->time() == note->time()) {
+	/* first try searching for the note using the time index, which is
+	 * faster since the container is "indexed" by time. (technically, this
+	 * means that lower_bound() can do a binary search rather than linear)
+	 *
+	 * this may not work, for reasons explained below.
+	 */
 
-		typename Sequence<Time>::Notes::iterator tmp = i;
-		++tmp;
+	typename Sequence<Time>::Notes::iterator i;
+		
+	for (i = note_lower_bound(note->time()); i != _notes.end() && musical_time_equal ((*i)->time(), note->time()); ++i) {
 
 		if (*i == note) {
 
-			NotePtr n = *i;
-
-			DEBUG_TRACE (DEBUG::Sequence, string_compose ("%1\terasing note %2 @ %3\n", this, (int)(*i)->note(), (*i)->time()));
+			DEBUG_TRACE (DEBUG::Sequence, string_compose ("%1\terasing note #%2 %3 @ %4\n", this, (*i)->id(), (int)(*i)->note(), (*i)->time()));
 			_notes.erase (i);
 
-			if (n->note() == _lowest_note || n->note() == _highest_note) {
+			if (note->note() == _lowest_note || note->note() == _highest_note) {
 
 				_lowest_note = 127;
 				_highest_note = 0;
@@ -752,30 +754,100 @@ Sequence<Time>::remove_note_unlocked(const constNotePtr note)
 			}
 
 			erased = true;
+			break;
 		}
+	}
 
-		i = tmp;
+	DEBUG_TRACE (DEBUG::Sequence, string_compose ("%1\ttime-based lookup did not find note #%2 %3 @ %4\n", this, note->id(), (int)note->note(), note->time()));
+
+	if (!erased) {
+
+		/* if the note's time property was changed in tandem with some
+		 * other property as the next operation after it was added to
+		 * the sequence, then at the point where we call this to undo
+		 * the add, the note we are targetting currently has a
+		 * different time property than the one we we passed via
+		 * the argument.
+		 *
+		 * in this scenario, we have no choice other than to linear
+		 * search the list of notes and find the note by ID.
+		 */
+		
+		for (i = _notes.begin(); i != _notes.end(); ++i) {
+
+			if ((*i)->id() == note->id()) {
+				
+				DEBUG_TRACE (DEBUG::Sequence, string_compose ("%1\tID-based pass, erasing note #%2 %3 @ %4\n", this, (*i)->id(), (int)(*i)->note(), (*i)->time()));
+				_notes.erase (i);
+				
+				if (note->note() == _lowest_note || note->note() == _highest_note) {
+					
+					_lowest_note = 127;
+					_highest_note = 0;
+					
+					for (typename Sequence<Time>::Notes::iterator ii = _notes.begin(); ii != _notes.end(); ++ii) {
+						if ((*ii)->note() < _lowest_note)
+							_lowest_note = (*ii)->note();
+						if ((*ii)->note() > _highest_note)
+							_highest_note = (*ii)->note();
+					}
+				}
+				
+				erased = true;
+				id_matched = true;
+				break;
+			}
+		}
 	}
+	
+	if (erased) {
 
-	Pitches& p (pitches (note->channel()));
+		Pitches& p (pitches (note->channel()));
+		
+		typename Pitches::iterator j;
 
-	NotePtr search_note(new Note<Time>(0, 0, 0, note->note(), 0));
+		/* if we had to ID-match above, we can't expect to find it in
+		 * pitches via note comparison either. so do another linear
+		 * search to locate it. otherwise, we can use the note index
+		 * to potentially speed things up.
+		 */
 
-	typename Pitches::iterator j = p.lower_bound (search_note);
-	while (j != p.end() && (*j)->note() == note->note()) {
-		typename Pitches::iterator tmp = j;
-		++tmp;
+		if (id_matched) {
+
+			for (j = p.begin(); j != p.end(); ++j) {
+				if ((*j)->id() == note->id()) {
+					p.erase (j);
+					break;
+				}
+			}
+
+		} else {
 
-		if (*j == note) {
-			DEBUG_TRACE (DEBUG::Sequence, string_compose ("%1\terasing pitch %2 @ %3\n", this, (int)(*j)->note(), (*j)->time()));
-			p.erase (j);
+			/* Now find the same note in the "pitches" list (which indexes
+			 * notes by channel+time. We care only about its note number
+			 * so the search_note has all other properties unset.
+			 */
+			
+			NotePtr search_note (new Note<Time>(0, 0, 0, note->note(), 0));
+
+			for (j = p.lower_bound (search_note); j != p.end() && (*j)->note() == note->note(); ++j) {
+				
+				if ((*j) == note) {
+					DEBUG_TRACE (DEBUG::Sequence, string_compose ("%1\terasing pitch %2 @ %3\n", this, (int)(*j)->note(), (*j)->time()));
+					p.erase (j);
+					break;
+				}
+			}
 		}
 
-		j = tmp;
-	}
+		if (j == p.end()) {
+			warning << string_compose ("erased note %1 not found in pitches for channel %2", *note, (int) note->channel()) << endmsg;
+		}
 
-	if (!erased) {
-		cerr << "Unable to find note to erase matching " << *note.get() << endl;
+		_edited = true;
+	
+	} else {
+		cerr << "Unable to find note to erase matching " << *note.get() << endmsg;
 	}
 }
 
@@ -784,12 +856,13 @@ void
 Sequence<Time>::remove_patch_change_unlocked (const constPatchChangePtr p)
 {
 	typename Sequence<Time>::PatchChanges::iterator i = patch_change_lower_bound (p->time ());
-	while (i != _patch_changes.end() && (*i)->time() == p->time()) {
+
+	while (i != _patch_changes.end() && (musical_time_equal ((*i)->time(), p->time()))) {
 
 		typename Sequence<Time>::PatchChanges::iterator tmp = i;
 		++tmp;
 
-		if (*i == p) {
+		if (**i == *p) {
 			_patch_changes.erase (i);
 		}
 
@@ -1148,7 +1221,7 @@ Sequence<Time>::patch_change_lower_bound (Time t) const
 {
 	PatchChangePtr search (new PatchChange<Time> (t, 0, 0, 0));
 	typename Sequence<Time>::PatchChanges::const_iterator i = _patch_changes.lower_bound (search);
-	assert (i == _patch_changes.end() || (*i)->time() >= t);
+	assert (i == _patch_changes.end() || musical_time_greater_or_equal_to ((*i)->time(), t));
 	return i;
 }