major fixes for MIDI patch change and note undo/redo. Patch change handling was compl...
authorPaul Davis <paul@linuxaudiosystems.com>
Fri, 29 Mar 2013 15:52:25 +0000 (11:52 -0400)
committerPaul Davis <paul@linuxaudiosystems.com>
Fri, 29 Mar 2013 15:52:25 +0000 (11:52 -0400)
libs/ardour/ardour/midi_model.h
libs/ardour/ardour/midi_track.h
libs/ardour/midi_model.cc
libs/ardour/midi_track.cc
libs/evoral/evoral/ControlSet.hpp
libs/evoral/evoral/PatchChange.hpp
libs/evoral/evoral/Sequence.hpp
libs/evoral/evoral/types.hpp
libs/evoral/src/Sequence.cpp

index 0d11f940b9ddf6d219ddab6abe10ced11ff293a8..3ecfca7d1c94d59a257b72998d07da1e9213cbd9 100644 (file)
@@ -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;
index be209bc0f6d8ddf4b860e46c1add625345175686..3b75c0a51b9c18d232aa15fc5305a2b0c548ee81 100644 (file)
@@ -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*/
index 4c6f6633d5e2a7ec90cd2dc3f3822f198f2d43ac..5c1f65d96b2703aaf15399944976ab1b9bf429dc 100644 (file)
@@ -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
index 0f1b2b52afa29391652fc90a0b25c3530fb76f1a..1a618a01ddcb1709ba138fb9d22cd6b7f35a1e2f 100644 (file)
@@ -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 */
index 431885112b1d6c6ac22f82822751c0d88e26ef3b..716d199fecc458bde73fb2b2c7e0d18dc4e0aff8 100644 (file)
@@ -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 () {}
index 82e20239417d6cb094f38911168d63d388fb8a55..b1a42c6f2e7560025b9041e6a75d9815775b7cd2 100644 (file)
@@ -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
index 815d02f980ebb716d4adf4b80b35504e852d1b62..26bef20232a04a1ce5af757d8c9c312c887a0861 100644 (file)
@@ -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());
                }
        };
 
index 7bdbdc7a2e15b95bfb8885d1fa53b226f8ea4889..000b79bb94c621b58046e2054cf8c7cf822c583e 100644 (file)
@@ -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;
 
index 71bdcb7c03a7857d7fa05fe2594c4a8b0e76cf78..738d5a19c9183562f563d7c930867721c2b37cf0 100644 (file)
@@ -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;
 }