Better MidiModel command framework, ready to go for all your canvas editing needs.

Rewrote MidiEvent to be a well-behaved self-contained object that optionally owns it's buffer, has proper copying semantics, etc.
Fixed crazy bugs triggered by adding lots of events with varying times to a region.
Speed up initial session display significantly (don't redraw each MIDI region tons of times, though still happens more than once and can use fixing...).


git-svn-id: svn://localhost/ardour2/trunk@2213 d708f5d6-7413-0410-9779-e7cbd77b26cf

22 files changed, 318 insertions, 180 deletions

diff --git a/gtk2_ardour/audio_streamview.cc b/gtk2_ardour/audio_streamview.cc
index 1e66b29321..3490c9d6c2 100644
--- a/gtk2_ardour/audio_streamview.cc
+++ b/gtk2_ardour/audio_streamview.cc
@@ -396,7 +396,10 @@ AudioStreamView::redisplay_diskstream ()
 		if (!(*i)->is_valid()) {
 			delete *i;
 			region_views.erase (i);
-		} 
+		} else {
+			(*i)->enable_display(true);
+			(*i)->set_y_position_and_height(0, height);
+		}
 
 		i = tmp;
 	}
diff --git a/gtk2_ardour/canvas-midi-event.cc b/gtk2_ardour/canvas-midi-event.cc
index 73c2b29925..dafe94872c 100644
--- a/gtk2_ardour/canvas-midi-event.cc
+++ b/gtk2_ardour/canvas-midi-event.cc
@@ -60,7 +60,8 @@ CanvasMidiEvent::on_event(GdkEvent* ev)
 	case GDK_LEAVE_NOTIFY:
 		cerr << "LEAVE: " << ev->crossing.state << endl;
 		Keyboard::magic_widget_drop_focus();
-		//_item->drop_focus();
+		//_region.get_time_axis_view().editor.reset_focus();
+		_region.get_canvas_group()->grab_focus();
 		break;
 	
 	case GDK_KEY_PRESS:
diff --git a/gtk2_ardour/midi_region_view.cc b/gtk2_ardour/midi_region_view.cc
index 755e896f6c..d1156bb9c5 100644
--- a/gtk2_ardour/midi_region_view.cc
+++ b/gtk2_ardour/midi_region_view.cc
@@ -74,10 +74,10 @@ MidiRegionView::MidiRegionView (ArdourCanvas::Group *parent, RouteTimeAxisView &
 void
 MidiRegionView::init (Gdk::Color& basic_color, bool wfd)
 {
-	if (wfd)
-		_model = midi_region()->midi_source(0)->model();
+	_model = midi_region()->midi_source(0)->model();
+	_enable_display = false;
 	
-	RegionView::init(basic_color, /*wfd*/false);
+	RegionView::init(basic_color, false);
 
 	compute_colors (basic_color);
 
@@ -93,12 +93,17 @@ MidiRegionView::init (Gdk::Color& basic_color, bool wfd)
 
 	set_colors ();
 
-	if (wfd)
-		redisplay_model();
-		
-	_model->ContentsChanged.connect(sigc::mem_fun(this, &MidiRegionView::redisplay_model));
+	_enable_display = true;
+
+	if (_model) {
+		if (wfd) {
+			redisplay_model();
+		}
+		_model->ContentsChanged.connect(sigc::mem_fun(this, &MidiRegionView::redisplay_model));
+	}
 	
 	group->signal_event().connect (mem_fun (this, &MidiRegionView::canvas_event), false);
+
 }
 
 bool
@@ -229,14 +234,14 @@ MidiRegionView::create_note_at(double x, double y)
 
 	// Add a 1 beat long note (for now)
 	const MidiModel::Note new_note(stamp, dur, (uint8_t)note, 0x40);
-
-	_model->begin_command();
-	_model->add_note(new_note);
-	_model->finish_command();
-
+	
 	view->update_bounds(new_note.note());
 
-	add_note(new_note);
+	MidiModel::DeltaCommand* cmd = _model->new_delta_command("add note");
+	cmd->add(new_note);
+	_model->apply_command(cmd);
+	
+	//add_note(new_note);
 }
 
 
@@ -254,7 +259,9 @@ void
 MidiRegionView::display_model(boost::shared_ptr<MidiModel> model)
 {
 	_model = model;
-	redisplay_model();
+
+	if (_enable_display)
+		redisplay_model();
 }
 
 
@@ -271,7 +278,6 @@ MidiRegionView::redisplay_model()
 
 		end_write();
 	} else {
-		assert(false);
 		warning << "MidiRegionView::redisplay_model called without a model" << endmsg;
 	}
 }
@@ -299,7 +305,8 @@ MidiRegionView::region_resized (Change what_changed)
 
 	if (what_changed & ARDOUR::PositionChanged) {
 	
-		redisplay_model();
+		if (_enable_display)
+			redisplay_model();
 	
 	} else if (what_changed & Change (StartChanged)) {
 
@@ -318,7 +325,8 @@ MidiRegionView::reset_width_dependent_items (double pixel_width)
 	RegionView::reset_width_dependent_items(pixel_width);
 	assert(_pixel_width == pixel_width);
 		
-	redisplay_model();
+	if (_enable_display)
+		redisplay_model();
 }
 
 void
@@ -326,7 +334,8 @@ MidiRegionView::set_y_position_and_height (double y, double h)
 {
 	RegionView::set_y_position_and_height(y, h - 1);
 
-	redisplay_model();
+	if (_enable_display)
+		redisplay_model();
 
 	if (name_text) {
 		name_text->raise_to_top();
@@ -402,14 +411,14 @@ MidiRegionView::add_event (const MidiEvent& ev)
 	const double pixel_range = (trackview.height - footer_height - 5.0) / (double)note_range;
 
 	if (mtv->note_mode() == Sustained) {
-		if ((ev.buffer[0] & 0xF0) == MIDI_CMD_NOTE_ON) {
-			const Byte& note = ev.buffer[1];
+		if ((ev.buffer()[0] & 0xF0) == MIDI_CMD_NOTE_ON) {
+			const Byte& note = ev.buffer()[1];
 			const double y1 = trackview.height - (pixel_range * (note - view->lowest_note() + 1))
 				- footer_height - 3.0;
 
 			CanvasNote* ev_rect = new CanvasNote(*this, *group);
 			ev_rect->property_x1() = trackview.editor.frame_to_pixel (
-					(nframes_t)ev.time);
+					(nframes_t)ev.time());
 			ev_rect->property_y1() = y1;
 			ev_rect->property_x2() = trackview.editor.frame_to_pixel (
 					_region->length());
@@ -425,18 +434,18 @@ MidiRegionView::add_event (const MidiEvent& ev)
 			if (_active_notes)
 				_active_notes[note] = ev_rect;
 
-		} else if ((ev.buffer[0] & 0xF0) == MIDI_CMD_NOTE_OFF) {
-			const Byte& note = ev.buffer[1];
+		} else if ((ev.buffer()[0] & 0xF0) == MIDI_CMD_NOTE_OFF) {
+			const Byte& note = ev.buffer()[1];
 			if (_active_notes && _active_notes[note]) {
-				_active_notes[note]->property_x2() = trackview.editor.frame_to_pixel((nframes_t)ev.time);
+				_active_notes[note]->property_x2() = trackview.editor.frame_to_pixel((nframes_t)ev.time());
 				_active_notes[note]->property_outline_what() = (guint32) 0xF; // all edges
 				_active_notes[note] = NULL;
 			}
 		}
 	
 	} else if (mtv->note_mode() == Percussive) {
-		const Byte& note = ev.buffer[1];
-		const double x = trackview.editor.frame_to_pixel((nframes_t)ev.time);
+		const Byte& note = ev.buffer()[1];
+		const double x = trackview.editor.frame_to_pixel((nframes_t)ev.time());
 		const double y = trackview.height - (pixel_range * (note - view->lowest_note() + 1))
 			- footer_height - 3.0;
 
@@ -477,12 +486,6 @@ MidiRegionView::add_note (const MidiModel::Note& note)
 	assert(note.time() < _region->length());
 	//assert(note.time() + note.duration < _region->length());
 
-	/*printf("Event, time = %f, size = %zu, data = ", ev.time, ev.size);
-	for (size_t i=0; i < ev.size; ++i) {
-		printf("%X ", ev.buffer[i]);
-	}
-	printf("\n\n");*/
-
 	MidiTimeAxisView* const mtv = dynamic_cast<MidiTimeAxisView*>(&trackview);
 	MidiStreamView* const view = mtv->midi_view();
 	ArdourCanvas::Group* const group = (ArdourCanvas::Group*)get_canvas_group();
@@ -494,6 +497,10 @@ MidiRegionView::add_note (const MidiModel::Note& note)
 	const uint32_t fill = RGBA_TO_UINT(0xE0 + note.velocity()/127.0 * 0x10, 0xE0, 0xE0, fill_alpha);
 	const uint8_t outline_alpha = 0x80 + (uint8_t)(note.velocity()); 
 	const uint32_t outline = RGBA_TO_UINT(0xE0 + note.velocity()/127.0 * 0x10, 0xE0, 0xE0, outline_alpha);
+	
+	//printf("Range: %d\n", note_range);
+	//printf("Event, time = %f, note = %d\n", note.time(), note.note());
+
 
 	if (mtv->note_mode() == Sustained) {
 		const double y1 = trackview.height - (pixel_range * (note.note() - view->lowest_note() + 1))
diff --git a/gtk2_ardour/midi_streamview.cc b/gtk2_ardour/midi_streamview.cc
index 51a0543d18..31a36ebd39 100644
--- a/gtk2_ardour/midi_streamview.cc
+++ b/gtk2_ardour/midi_streamview.cc
@@ -76,7 +76,7 @@ MidiStreamView::~MidiStreamView ()
 
 
 RegionView*
-MidiStreamView::add_region_view_internal (boost::shared_ptr<Region> r, bool wait_for_waves)
+MidiStreamView::add_region_view_internal (boost::shared_ptr<Region> r, bool wfd)
 {
 	boost::shared_ptr<MidiRegion> region = boost::dynamic_pointer_cast<MidiRegion> (r);
 
@@ -93,6 +93,7 @@ MidiStreamView::add_region_view_internal (boost::shared_ptr<Region> r, bool wait
 			/* great. we already have a MidiRegionView for this Region. use it again. */
 
 			(*i)->set_valid (true);
+			(*i)->enable_display(wfd);
 			display_region(dynamic_cast<MidiRegionView*>(*i));
 
 			return NULL;
@@ -101,14 +102,14 @@ MidiStreamView::add_region_view_internal (boost::shared_ptr<Region> r, bool wait
 	
 	region_view = new MidiRegionView (canvas_group, _trackview, region, 
 			_samples_per_unit, region_color);
-
-	region_view->init (region_color, wait_for_waves);
+		
+	region_view->init (region_color, false);
 	region_views.push_front (region_view);
 	
 	/* follow global waveform setting */
 
-	// FIXME
-	//region_view->set_waveform_visible(_trackview.editor.show_waveforms());
+	if (wfd)
+		region_view->enable_display(true);
 
 	/* display events and find note range */
 	display_region(region_view);
@@ -150,6 +151,7 @@ MidiStreamView::redisplay_diskstream ()
 	list<RegionView *>::iterator i, tmp;
 
 	for (i = region_views.begin(); i != region_views.end(); ++i) {
+		(*i)->enable_display(false);
 		(*i)->set_valid (false);
 	}
 	
@@ -168,6 +170,7 @@ MidiStreamView::redisplay_diskstream ()
 			delete *i;
 			region_views.erase (i);
 		} else {
+			(*i)->enable_display(true);
 			(*i)->set_y_position_and_height(0, height); // apply note range
 		}
 
diff --git a/gtk2_ardour/region_view.cc b/gtk2_ardour/region_view.cc
index 87b384bcd7..27ae6ca7bf 100644
--- a/gtk2_ardour/region_view.cc
+++ b/gtk2_ardour/region_view.cc
@@ -72,6 +72,7 @@ RegionView::RegionView (ArdourCanvas::Group* parent,
 	  , editor(0)
 	  , current_visible_sync_position(0.0)
 	  , valid(false)
+	  , _enable_display(false)
 	  , _pixel_width(1.0)
 	  , in_destructor(false)
 	  , wait_for_data(false)
@@ -87,6 +88,7 @@ RegionView::RegionView (const RegionView& other)
 	editor = other.editor;
 	current_visible_sync_position = other.current_visible_sync_position;
 	valid = false;
+	_enable_display = false;
 	_pixel_width = other._pixel_width;
 }
 
@@ -102,6 +104,7 @@ RegionView::RegionView (ArdourCanvas::Group*         parent,
 	, editor(0)
 	, current_visible_sync_position(0.0)
 	, valid(false)
+	, _enable_display(false)
 	, _pixel_width(1.0)
 	, in_destructor(false)
 	, wait_for_data(false)
@@ -111,9 +114,10 @@ RegionView::RegionView (ArdourCanvas::Group*         parent,
 void
 RegionView::init (Gdk::Color& basic_color, bool wfd)
 {
-	valid         = true;
-	in_destructor = false;
-	wait_for_data = wfd;
+	valid           = true;
+	_enable_display = false;
+	in_destructor   = false;
+	wait_for_data   = wfd;
 
 	compute_colors (basic_color);
 
@@ -138,6 +142,9 @@ RegionView::init (Gdk::Color& basic_color, bool wfd)
 
 	reset_width_dependent_items ((double) _region->length() / samples_per_unit);
 
+	if (wfd)
+		_enable_display = true;
+
 	set_y_position_and_height (0, trackview.height - 2);
 
 	_region->StateChanged.connect (mem_fun(*this, &RegionView::region_changed));
diff --git a/gtk2_ardour/region_view.h b/gtk2_ardour/region_view.h
index 4bbd68eef0..426529c528 100644
--- a/gtk2_ardour/region_view.h
+++ b/gtk2_ardour/region_view.h
@@ -82,6 +82,8 @@ class RegionView : public TimeAxisViewItem
 
 	virtual void entered () {}
 	virtual void exited () {}
+
+	void enable_display(bool yn) { _enable_display = yn; }
 	
 	static sigc::signal<void,RegionView*> RegionViewGoingAway;
 	sigc::signal<void>                    GoingAway;
@@ -127,7 +129,8 @@ class RegionView : public TimeAxisViewItem
     vector<ControlPoint *> control_points;
     double current_visible_sync_position;
 
-    bool     valid; ///< see StreamView::redisplay_diskstream() 
+    bool    valid; ///< see StreamView::redisplay_diskstream() 
+    bool    _enable_display; ///< see StreamView::redisplay_diskstream() 
     double  _pixel_width;
     bool    in_destructor;
     
diff --git a/gtk2_ardour/streamview.cc b/gtk2_ardour/streamview.cc
index 5220b80c10..34c1e4a1c9 100644
--- a/gtk2_ardour/streamview.cc
+++ b/gtk2_ardour/streamview.cc
@@ -153,8 +153,7 @@ StreamView::set_samples_per_unit (gdouble spp)
 void
 StreamView::add_region_view (boost::shared_ptr<Region> r)
 {
-	add_region_view_internal (r, true);
-	update_contents_y_position_and_height ();
+	add_region_view_internal (r, false);
 }
 
 void
diff --git a/libs/ardour/ardour/midi_event.h b/libs/ardour/ardour/midi_event.h
index 8aca2d14da..a0816d5105 100644
--- a/libs/ardour/ardour/midi_event.h
+++ b/libs/ardour/ardour/midi_event.h
@@ -30,17 +30,75 @@ namespace ARDOUR {
  * tempo time, used in MidiModel) depending on context.
  */
 struct MidiEvent {
-	MidiEvent(double t=0, size_t s=0, Byte* b=NULL)
-		: time(t), size(s), buffer(b)
-	{}
+	MidiEvent(bool owns_buffer=false, double t=0, size_t s=0, Byte* b=NULL)
+		: _owns_buffer(owns_buffer)
+		, _time(t)
+		, _size(s)
+		, _buffer(b)
+	{
+		if (owns_buffer) {
+			_buffer = (Byte*)malloc(_size);
+			if (b)
+				memcpy(_buffer, b, _size);
+			else
+				memset(_buffer, 0, _size);
+		}
+	}
+	
+	MidiEvent(const MidiEvent& copy, bool owns_buffer)
+		: _owns_buffer(owns_buffer)
+		, _time(copy._time)
+		, _size(copy._size)
+		, _buffer(copy._buffer)
+	{
+		if (owns_buffer) {
+			_buffer = (Byte*)malloc(_size);
+			if (copy._buffer)
+				memcpy(_buffer, copy._buffer, _size);
+			else
+				memset(_buffer, 0, _size);
+		}
+	}
+	
+	~MidiEvent() {
+		if (_owns_buffer)
+			free(_buffer);
+	}
 
-	inline uint8_t type()     const { return (buffer[0] & 0xF0); }
-	inline uint8_t note()     const { return (buffer[1]); }
-	inline uint8_t velocity() const { return (buffer[2]); }
+	inline const MidiEvent& operator=(const MidiEvent& copy) {
+		_time = copy._time;
+		if (!_owns_buffer) {
+			_buffer = copy._buffer;
+		} else if (copy._buffer) {
+			if (!_buffer || _size < copy._size)
+				_buffer = (Byte*)realloc(_buffer, copy._size);
+			memcpy(_buffer, copy._buffer, copy._size);
+		} else {
+			free(_buffer);
+			_buffer = NULL;
+		}
+		_size = copy._size;
+		return *this;
+	}
 
-	double time;   /**< Sample index (or beat time) at which event is valid */
-	size_t size;   /**< Number of bytes of data in \a buffer */
-	Byte*  buffer; /**< Raw MIDI data */
+	inline bool        owns_buffer() const { return _owns_buffer; }
+	inline double      time()        const { return _time; }
+	inline double&     time()              { return _time; }
+	inline size_t      size()        const { return _size; }
+	inline size_t&     size()              { return _size; }
+	inline uint8_t     type()        const { return (_buffer[0] & 0xF0); }
+	inline uint8_t     note()        const { return (_buffer[1]); }
+	inline uint8_t     velocity()    const { return (_buffer[2]); }
+	inline const Byte* buffer()      const { return _buffer; }
+	inline Byte*       buffer()            { return _buffer; }
+
+	void set_buffer(Byte* buf) { assert(!_owns_buffer); _buffer = buf; }
+
+private:
+	bool   _owns_buffer; /**< Whether buffer is locally allocated */
+	double _time;         /**< Sample index (or beat time) at which event is valid */
+	size_t _size;         /**< Number of bytes of data in \a buffer */
+	Byte*  _buffer;       /**< Raw MIDI data */
 };
 
 
diff --git a/libs/ardour/ardour/midi_model.h b/libs/ardour/ardour/midi_model.h
index a2e2275157..994d15d5b8 100644
--- a/libs/ardour/ardour/midi_model.h
+++ b/libs/ardour/ardour/midi_model.h
@@ -43,17 +43,19 @@ public:
 	struct Note {
 		Note(double time=0, double dur=0, uint8_t note=0, uint8_t vel=0x40);
 		Note(const Note& copy);
+		
+		const MidiModel::Note& operator=(const MidiModel::Note& copy);
 
 		inline bool operator==(const Note& other)
 			{ return time() == other.time() && note() == other.note(); }
 
-		inline double  time()     const { return _on_event.time; }
-		inline double  end_time() const { return _off_event.time; }
+		inline double  time()     const { return _on_event.time(); }
+		inline double  end_time() const { return _off_event.time(); }
 		inline uint8_t note()     const { return _on_event.note(); }
 		inline uint8_t velocity() const { return _on_event.velocity(); }
-		inline double  duration() const { return _off_event.time - _on_event.time; }
+		inline double  duration() const { return _off_event.time() - _on_event.time(); }
 
-		inline void set_duration(double d) { _off_event.time = _on_event.time + d; }
+		inline void set_duration(double d) { _off_event.time() = _on_event.time() + d; }
 
 		inline MidiEvent& on_event()  { return _on_event; }
 		inline MidiEvent& off_event() { return _off_event; }
@@ -62,10 +64,9 @@ public:
 		inline const MidiEvent& off_event() const { return _off_event; }
 
 	private:
+		// Event buffers are self-contained
 		MidiEvent _on_event;
 		MidiEvent _off_event;
-		Byte      _on_event_buffer[3];
-		Byte      _off_event_buffer[3];
 	};
 
 	MidiModel(Session& s, size_t size=0);
@@ -106,23 +107,18 @@ public:
 
 	inline       Notes& notes()       { return _notes; }
 	inline const Notes& notes() const { return _notes; }
-
-	void     begin_command();
-	Command* current_command() { return _command; }
-	void     finish_command();
-
-	// Commands
-	void add_note(const Note& note);
-	void remove_note(const Note& note);
-
-	sigc::signal<void> ContentsChanged;
 	
-private:
-	class MidiEditCommand : public Command
+	/** Add/Remove notes.
+	 * Technically all operations can be implemented as one of these.
+	 */
+	class DeltaCommand : public Command
 	{
 	public:
-		MidiEditCommand (MidiModel& m) : _model(m) {}
-		//MidiEditCommand (MidiModel&, const XMLNode& node);
+		DeltaCommand (MidiModel& m, const std::string& name)
+			: Command(name), _model(m), _name(name) {}
+		//DeltaCommand (MidiModel&, const XMLNode& node);
+
+		const std::string& name() const { return _name; }
 		
 		void operator()();
 		void undo();
@@ -130,15 +126,28 @@ private:
 		/*int set_state (const XMLNode&);
 		XMLNode& get_state ();*/
 
-		void add_note(const Note& note);
-		void remove_note(const Note& note);
+		void add(const Note& note);
+		void remove(const Note& note);
 
 	private:
 		MidiModel&      _model;
+		std::string     _name;
 		std::list<Note> _added_notes;
 		std::list<Note> _removed_notes;
 	};
 
+	MidiModel::DeltaCommand* new_delta_command(const std::string name="midi edit");
+	void                     apply_command(Command* cmd);
+
+	sigc::signal<void> ContentsChanged;
+	
+private:
+	friend class DeltaCommand;
+	void add_note(const Note& note);
+	void remove_note(const Note& note);
+
+	bool is_sorted() const;
+
 	void append_note_on(double time, uint8_t note, uint8_t velocity);
 	void append_note_off(double time, uint8_t note);
 
@@ -150,8 +159,6 @@ private:
 	typedef std::vector<size_t> WriteNotes;
 	WriteNotes _write_notes;
 	bool       _writing;
-
-	MidiEditCommand* _command; ///< In-progress command
 };
 
 } /* namespace ARDOUR */
diff --git a/libs/ardour/ardour/midi_ring_buffer.h b/libs/ardour/ardour/midi_ring_buffer.h
index 6c57c78c3c..21beebacd2 100644
--- a/libs/ardour/ardour/midi_ring_buffer.h
+++ b/libs/ardour/ardour/midi_ring_buffer.h
@@ -288,24 +288,24 @@ MidiRingBuffer::read(MidiBuffer& dst, nframes_t start, nframes_t end, nframes_t
 
 	while (read_space() > sizeof(double) + sizeof(size_t)) {
 	
-		full_peek(sizeof(double), (Byte*)&ev.time);
+		full_peek(sizeof(double), (Byte*)&ev.time());
 	
-		if (ev.time > end)
+		if (ev.time() > end)
 			break;
 
-		bool success = MidiRingBufferBase<Byte>::full_read(sizeof(double), (Byte*)&ev.time);
+		bool success = MidiRingBufferBase<Byte>::full_read(sizeof(double), (Byte*)&ev.time());
 		if (success)
-			success = MidiRingBufferBase<Byte>::full_read(sizeof(size_t), (Byte*)&ev.size);
+			success = MidiRingBufferBase<Byte>::full_read(sizeof(size_t), (Byte*)&ev.size());
 		
 		if (!success) {
 			std::cerr << "MRB: READ ERROR (time/size)" << std::endl;
 			continue;
 		}
 
-		if (ev.time >= start) {
-			ev.time -= start;
-			Byte* write_loc = dst.reserve(ev.time, ev.size);
-			success = MidiRingBufferBase<Byte>::full_read(ev.size, write_loc);
+		if (ev.time() >= start) {
+			ev.time() -= start;
+			Byte* write_loc = dst.reserve(ev.time(), ev.size());
+			success = MidiRingBufferBase<Byte>::full_read(ev.size(), write_loc);
 		
 			if (success) {
 				++count;
@@ -315,7 +315,7 @@ MidiRingBuffer::read(MidiBuffer& dst, nframes_t start, nframes_t end, nframes_t
 			}
 			
 		} else {
-			printf("MRB - SKIPPING EVENT (with time %f)\n", ev.time);
+			printf("MRB - SKIPPING EVENT (with time %f)\n", ev.time());
 		}
 	}
 	
diff --git a/libs/ardour/ardour/session.h b/libs/ardour/ardour/session.h
index 5ba2e39526..05d87620f2 100644
--- a/libs/ardour/ardour/session.h
+++ b/libs/ardour/ardour/session.h
@@ -774,7 +774,7 @@ class Session : public PBD::StatefulDestructible
 	string next_undo() const { return _history.next_undo(); }
 	string next_redo() const { return _history.next_redo(); }
 
-	void begin_reversible_command (string cmd_name);
+	void begin_reversible_command (const string& cmd_name);
 	void commit_reversible_command (Command* cmd = 0);
 
 	void add_command (Command *const cmd) {
diff --git a/libs/ardour/meter.cc b/libs/ardour/meter.cc
index 84dac2ded4..74a3e61f9a 100644
--- a/libs/ardour/meter.cc
+++ b/libs/ardour/meter.cc
@@ -49,8 +49,8 @@ PeakMeter::run (BufferSet& bufs, nframes_t start_frame, nframes_t end_frame, nfr
 		// expressed through peaks alone
 		for (MidiBuffer::iterator i = bufs.get_midi(n).begin(); i != bufs.get_midi(n).end(); ++i) {
 			const MidiEvent& ev = *i;
-			if ((ev.buffer[0] & 0xF0) == MIDI_CMD_NOTE_ON) {
-				const float this_vel = log(ev.buffer[2] / 127.0 * (M_E*M_E-M_E) + M_E) - 1.0;
+			if ((ev.buffer()[0] & 0xF0) == MIDI_CMD_NOTE_ON) {
+				const float this_vel = log(ev.buffer()[2] / 127.0 * (M_E*M_E-M_E) + M_E) - 1.0;
 				//printf("V %d -> %f\n", (int)((Byte)ev.buffer[2]), this_vel);
 				if (this_vel > val)
 					val = this_vel;
diff --git a/libs/ardour/midi_buffer.cc b/libs/ardour/midi_buffer.cc
index 5b7c789ba0..07ae2abab0 100644
--- a/libs/ardour/midi_buffer.cc
+++ b/libs/ardour/midi_buffer.cc
@@ -90,11 +90,11 @@ MidiBuffer::read_from(const Buffer& src, nframes_t nframes, nframes_t offset)
 	// FIXME: slow
 	for (size_t i=0; i < src.size(); ++i) {
 		const MidiEvent& ev = msrc[i];
-		if (ev.time >= offset && ev.time < offset+nframes) {
-			//cerr << "MidiBuffer::read_from got event, " << ev.time << endl;
+		if (ev.time() >= offset && ev.time() < offset+nframes) {
+			//cerr << "MidiBuffer::read_from got event, " << ev.time() << endl;
 			push_back(ev);
 		} else {
-			//cerr << "MidiBuffer event out of range, " << ev.time << endl;
+			//cerr << "MidiBuffer event out of range, " << ev.time() << endl;
 		}
 	}
 
@@ -117,9 +117,9 @@ MidiBuffer::push_back(const MidiEvent& ev)
 
 	Byte* const write_loc = _data + (_size * MAX_EVENT_SIZE);
 
-	memcpy(write_loc, ev.buffer, ev.size);
+	memcpy(write_loc, ev.buffer(), ev.size());
 	_events[_size] = ev;
-	_events[_size].buffer = write_loc;
+	_events[_size].set_buffer(write_loc);
 	++_size;
 
 	//cerr << "MidiBuffer: pushed, size = " << _size << endl;
@@ -146,9 +146,9 @@ MidiBuffer::push_back(const jack_midi_event_t& ev)
 	Byte* const write_loc = _data + (_size * MAX_EVENT_SIZE);
 
 	memcpy(write_loc, ev.buffer, ev.size);
-	_events[_size].time = (double)ev.time;
-	_events[_size].size = ev.size;
-	_events[_size].buffer = write_loc;
+	_events[_size].time() = (double)ev.time;
+	_events[_size].size() = ev.size;
+	_events[_size].set_buffer(write_loc);
 	++_size;
 
 	//cerr << "MidiBuffer: pushed, size = " << _size << endl;
@@ -176,9 +176,9 @@ MidiBuffer::reserve(double time, size_t size)
 
 	Byte* const write_loc = _data + (_size * MAX_EVENT_SIZE);
 
-	_events[_size].time = time;
-	_events[_size].size = size;
-	_events[_size].buffer = write_loc;
+	_events[_size].time() = time;
+	_events[_size].size() = size;
+	_events[_size].set_buffer(write_loc);
 	++_size;
 
 	//cerr << "MidiBuffer: reserved, size = " << _size << endl;
@@ -238,7 +238,7 @@ MidiBuffer::merge(const MidiBuffer& a, const MidiBuffer& b)
 			const MidiEvent& a_ev = a[a_index];
 			const MidiEvent& b_ev = b[b_index];
 
-			if (a_ev.time <= b_ev.time) {
+			if (a_ev.time() <= b_ev.time()) {
 				push_back(a_ev);
 				++a_index;
 			} else {
diff --git a/libs/ardour/midi_diskstream.cc b/libs/ardour/midi_diskstream.cc
index 3fed0cf68c..7137772193 100644
--- a/libs/ardour/midi_diskstream.cc
+++ b/libs/ardour/midi_diskstream.cc
@@ -555,7 +555,7 @@ MidiDiskstream::process (nframes_t transport_frame, nframes_t nframes, nframes_t
 
 		for (size_t i=0; i < to_write; ++i) {
 			const MidiEvent& ev = *port_iter;
-			_capture_buf->write(ev.time + transport_frame, ev.size, ev.buffer);
+			_capture_buf->write(ev.time() + transport_frame, ev.size(), ev.buffer());
 			++port_iter;
 		}
 	
diff --git a/libs/ardour/midi_model.cc b/libs/ardour/midi_model.cc
index 7374916c14..27bb681c43 100644
--- a/libs/ardour/midi_model.cc
+++ b/libs/ardour/midi_model.cc
@@ -19,6 +19,7 @@
 */
 
 #include <iostream>
+#include <algorithm>
 #include <queue>
 #include <pbd/enumwriter.h>
 #include <ardour/midi_model.h>
@@ -32,21 +33,17 @@ using namespace ARDOUR;
 // Note
 
 MidiModel::Note::Note(double t, double d, uint8_t n, uint8_t v)
+	: _on_event(true, t, 3, NULL)
+	, _off_event(true, t + d, 3, NULL)
 {
-	_on_event.time = t;
-	_on_event.buffer = _on_event_buffer;
-	_on_event.size = 3;
-	_on_event.buffer[0] = MIDI_CMD_NOTE_ON;
-	_on_event.buffer[1] = n;
-	_on_event.buffer[2] = v;
+	_on_event.buffer()[0] = MIDI_CMD_NOTE_ON;
+	_on_event.buffer()[1] = n;
+	_on_event.buffer()[2] = v;
+	
+	_off_event.buffer()[0] = MIDI_CMD_NOTE_OFF;
+	_off_event.buffer()[1] = n;
+	_off_event.buffer()[2] = 0x40;
 	
-	_off_event.time = t + d;
-	_off_event.buffer = _off_event_buffer;
-	_off_event.size = 3;
-	_off_event.buffer[0] = MIDI_CMD_NOTE_OFF;
-	_off_event.buffer[1] = n;
-	_off_event.buffer[2] = 0x40;
-
 	assert(time() == t);
 	assert(duration() == d);
 	assert(note() == n);
@@ -55,16 +52,50 @@ MidiModel::Note::Note(double t, double d, uint8_t n, uint8_t v)
 
 
 MidiModel::Note::Note(const Note& copy)
-	: _on_event(copy._on_event)
-	, _off_event(copy._off_event)
+	: _on_event(copy._on_event, true)
+	, _off_event(copy._off_event, true)
 {
-	memcpy(_on_event_buffer, copy._on_event_buffer, 3);
-	memcpy(_off_event_buffer, copy._off_event_buffer, 3);
+	/*
+	assert(copy._on_event.size == 3);
 	_on_event.buffer = _on_event_buffer;
+	memcpy(_on_event_buffer, copy._on_event_buffer, 3);
+	
+	assert(copy._off_event.size == 3);
 	_off_event.buffer = _off_event_buffer;
+	memcpy(_off_event_buffer, copy._off_event_buffer, 3);
+	*/
+
+	assert(time() == copy.time());
+	assert(end_time() == copy.end_time());
+	assert(note() == copy.note());
+	assert(velocity() == copy.velocity());
+	assert(duration() == copy.duration());
 }
 
 
+const MidiModel::Note&
+MidiModel::Note::operator=(const Note& copy)
+{
+	_on_event = copy._on_event;
+	_off_event = copy._off_event;
+	/*_on_event.time = copy._on_event.time;
+	assert(copy._on_event.size == 3);
+	memcpy(_on_event_buffer, copy._on_event_buffer, 3);
+	
+	_off_event.time = copy._off_event.time;
+	assert(copy._off_event.size == 3);
+	memcpy(_off_event_buffer, copy._off_event_buffer, 3);
+	*/
+	
+	assert(time() == copy.time());
+	assert(end_time() == copy.end_time());
+	assert(note() == copy.note());
+	assert(velocity() == copy.velocity());
+	assert(duration() == copy.duration());
+
+	return *this;
+}
+
 // MidiModel
 
 MidiModel::MidiModel(Session& s, size_t size)
@@ -72,7 +103,6 @@ MidiModel::MidiModel(Session& s, size_t size)
 	, _notes(size)
 	, _note_mode(Sustained)
 	, _writing(false)
-	, _command(NULL)
 {
 }
 
@@ -105,8 +135,8 @@ MidiModel::read (MidiRingBuffer& dst, nframes_t start, nframes_t nframes, nframe
 			while ( ! active_notes.empty() ) {
 				const Note* const earliest_off = active_notes.top();
 				const MidiEvent& ev = earliest_off->off_event();
-				if (ev.time < start + nframes && ev.time <= n->time()) {
-					dst.write(ev.time + stamp_offset, ev.size, ev.buffer);
+				if (ev.time() < start + nframes && ev.time() <= n->time()) {
+					dst.write(ev.time() + stamp_offset, ev.size(), ev.buffer());
 					active_notes.pop();
 					++read_events;
 				} else {
@@ -117,7 +147,7 @@ MidiModel::read (MidiRingBuffer& dst, nframes_t start, nframes_t nframes, nframe
 			// Note on
 			if (n->time() >= start) {
 				const MidiEvent& ev = n->on_event();
-				dst.write(ev.time + stamp_offset, ev.size, ev.buffer);
+				dst.write(ev.time() + stamp_offset, ev.size(), ev.buffer());
 				active_notes.push(&(*n));
 				++read_events;
 			}
@@ -131,7 +161,7 @@ MidiModel::read (MidiRingBuffer& dst, nframes_t start, nframes_t nframes, nframe
 			if (n->time() >= start) {
 				if (n->time() < start + nframes) {
 					const MidiEvent& ev = n->on_event();
-					dst.write(ev.time + stamp_offset, ev.size, ev.buffer);
+					dst.write(ev.time() + stamp_offset, ev.size(), ev.buffer());
 					++read_events;
 				} else {
 					break;
@@ -209,12 +239,12 @@ MidiModel::append(const MidiBuffer& buf)
 	for (MidiBuffer::const_iterator i = buf.begin(); i != buf.end(); ++i) {
 		const MidiEvent& ev = *i;
 		
-		assert(_notes.empty() || ev.time >= _notes.back().time());
+		assert(_notes.empty() || ev.time() >= _notes.back().time());
 
 		if (ev.type() == MIDI_CMD_NOTE_ON)
-			append_note_on(ev.time, ev.note(), ev.velocity());
+			append_note_on(ev.time(), ev.note(), ev.velocity());
 		else if (ev.type() == MIDI_CMD_NOTE_OFF)
-			append_note_off(ev.time, ev.note());
+			append_note_off(ev.time(), ev.note());
 	}
 }
 
@@ -285,42 +315,60 @@ MidiModel::append_note_off(double time, uint8_t note_num)
 void
 MidiModel::add_note(const Note& note)
 {
-	// FIXME: take source lock
 	Notes::iterator i = upper_bound(_notes.begin(), _notes.end(), note, note_time_comparator);
 	_notes.insert(i, note);
-	if (_command)
-		_command->add_note(note);
 }
 
 
 void
 MidiModel::remove_note(const Note& note)
 {
-	// FIXME: take source lock
 	Notes::iterator n = find(_notes.begin(), _notes.end(), note);
 	if (n != _notes.end())
 		_notes.erase(n);
-	
-	if (_command)
-		_command->remove_note(note);
 }
 
+/** Slow!  for debugging only. */
+bool
+MidiModel::is_sorted() const
+{
+	bool t = 0;
+	for (Notes::const_iterator n = _notes.begin(); n != _notes.end(); ++n)
+		if (n->time() < t)
+			return false;
+		else
+			t = n->time();
+
+	return true;
+}
 
 
-void
-MidiModel::begin_command()
+/** Start a new command.
+ *
+ * This has no side-effects on the model or Session, the returned command
+ * can be held on to for as long as the caller wishes, or discarded without
+ * formality, until apply_command is called and ownership is taken.
+ */
+MidiModel::DeltaCommand*
+MidiModel::new_delta_command(const string name)
 {
-	assert(!_command);
-	_session.begin_reversible_command("midi edit");
-	_command = new MidiEditCommand(*this);
+	DeltaCommand* cmd =  new DeltaCommand(*this, name);
+	return cmd;
 }
 
 
+/** Apply a command.
+ *
+ * Ownership of cmd is taken, it must not be deleted by the caller.
+ * The command will constitute one item on the undo stack.
+ */
 void
-MidiModel::finish_command()
+MidiModel::apply_command(Command* cmd)
 {
-	_session.commit_reversible_command(_command);
-	_command = NULL;
+	_session.begin_reversible_command(cmd->name());
+	(*cmd)();
+	assert(is_sorted());
+	_session.commit_reversible_command(cmd);
 }
 
 
@@ -328,7 +376,7 @@ MidiModel::finish_command()
 
 
 void
-MidiModel::MidiEditCommand::add_note(const Note& note)
+MidiModel::DeltaCommand::add(const Note& note)
 {
 	//cerr << "MEC: apply" << endl;
 
@@ -338,7 +386,7 @@ MidiModel::MidiEditCommand::add_note(const Note& note)
 
 
 void
-MidiModel::MidiEditCommand::remove_note(const Note& note)
+MidiModel::DeltaCommand::remove(const Note& note)
 {
 	//cerr << "MEC: remove" << endl;
 
@@ -348,26 +396,26 @@ MidiModel::MidiEditCommand::remove_note(const Note& note)
 
 		
 void 
-MidiModel::MidiEditCommand::operator()()
+MidiModel::DeltaCommand::operator()()
 {
-	//cerr << "MEC: apply" << endl;
-	assert(!_model.current_command());
+	// This could be made much faster by using a priority_queue for added and
+	// removed notes (or sort here), and doing a single iteration over _model
 	
 	for (std::list<Note>::iterator i = _added_notes.begin(); i != _added_notes.end(); ++i)
 		_model.add_note(*i);
 	
 	for (std::list<Note>::iterator i = _removed_notes.begin(); i != _removed_notes.end(); ++i)
 		_model.remove_note(*i);
-
+	
 	_model.ContentsChanged(); /* EMIT SIGNAL */
 }
 
 
 void
-MidiModel::MidiEditCommand::undo()
+MidiModel::DeltaCommand::undo()
 {
-	//cerr << "MEC: undo" << endl;
-	assert(!_model.current_command());
+	// This could be made much faster by using a priority_queue for added and
+	// removed notes (or sort here), and doing a single iteration over _model
 
 	for (std::list<Note>::iterator i = _added_notes.begin(); i != _added_notes.end(); ++i)
 		_model.remove_note(*i);
diff --git a/libs/ardour/midi_port.cc b/libs/ardour/midi_port.cc
index 2dc90a09da..3f03cfe880 100644
--- a/libs/ardour/midi_port.cc
+++ b/libs/ardour/midi_port.cc
@@ -102,9 +102,9 @@ MidiPort::cycle_end()
 	for (MidiBuffer::iterator i = _buffer.begin(); i != _buffer.end(); ++i) {
 		const MidiEvent& ev = *i;
 		// event times should be frames, relative to cycle start
-		assert(ev.time >= 0);
-		assert(ev.time < _nframes_this_cycle);
-		jack_midi_event_write(jack_buffer, (jack_nframes_t)ev.time, ev.buffer, ev.size);
+		assert(ev.time() >= 0);
+		assert(ev.time() < _nframes_this_cycle);
+		jack_midi_event_write(jack_buffer, (jack_nframes_t)ev.time(), ev.buffer(), ev.size());
 	}
 	
 	_nframes_this_cycle = 0;
diff --git a/libs/ardour/midi_track.cc b/libs/ardour/midi_track.cc
index 55d195fab0..ae1c8f1a07 100644
--- a/libs/ardour/midi_track.cc
+++ b/libs/ardour/midi_track.cc
@@ -584,11 +584,10 @@ MidiTrack::write_controller_messages(MidiBuffer& output_buf, nframes_t start_fra
 	// Write immediate events (UI controls)
 	MidiBuffer& cc_buf = mix_buffers.get_midi(0);
 	cc_buf.silence(nframes, offset);
-	MidiEvent ev;
-	ev.size = 3; // CC = 3 bytes
-	Byte buf[ev.size];
+	
+	Byte buf[3]; // CC = 3 bytes
 	buf[0] = MIDI_CMD_CONTROL;
-	ev.buffer = buf;
+	MidiEvent ev(false, 0, 3, buf);
 
 	// Write controller automation
 	if (_session.transport_rolling()) {
@@ -611,9 +610,9 @@ MidiTrack::write_controller_messages(MidiBuffer& output_buf, nframes_t start_fra
 				assert(y >= 0.0);
 				assert(y <= 127.0);
 
-				ev.time = stamp;
-				ev.buffer[1] = (Byte)list->parameter().id();
-				ev.buffer[2] = (Byte)y;
+				ev.time() = stamp;
+				ev.buffer()[1] = (Byte)list->parameter().id();
+				ev.buffer()[2] = (Byte)y;
 
 				cc_buf.push_back(ev);
 
diff --git a/libs/ardour/session_state.cc b/libs/ardour/session_state.cc
index 94aae01a53..f6debff123 100644
--- a/libs/ardour/session_state.cc
+++ b/libs/ardour/session_state.cc
@@ -2027,7 +2027,7 @@ Session::edit_group_by_name (string name)
 }
 
 void
-Session::begin_reversible_command (string name)
+Session::begin_reversible_command (const string& name)
 {
 	current_trans = new UndoTransaction;
 	current_trans->set_name (name);
diff --git a/libs/ardour/smf_source.cc b/libs/ardour/smf_source.cc
index d9a46ec782..4ae1cd34ab 100644
--- a/libs/ardour/smf_source.cc
+++ b/libs/ardour/smf_source.cc
@@ -382,10 +382,10 @@ SMFSource::write_unlocked (MidiRingBuffer& src, nframes_t cnt)
 	
 	for (MidiBuffer::iterator i = buf.begin(); i != buf.end(); ++i) {
 		MidiEvent& ev = *i;
-		assert(ev.time >= _timeline_position);
-		ev.time -= _timeline_position;
-		assert(ev.time >= _last_ev_time);
-		const uint32_t delta_time = (uint32_t)((ev.time - _last_ev_time) / frames_per_beat * _ppqn);
+		assert(ev.time() >= _timeline_position);
+		ev.time() -= _timeline_position;
+		assert(ev.time() >= _last_ev_time);
+		const uint32_t delta_time = (uint32_t)((ev.time() - _last_ev_time) / frames_per_beat * _ppqn);
 		
 		/*printf("SMF - writing event, delta = %u, size = %zu, data = ",
 			delta_time, ev.size);
@@ -395,12 +395,12 @@ SMFSource::write_unlocked (MidiRingBuffer& src, nframes_t cnt)
 		printf("\n");
 		*/
 		size_t stamp_size = write_var_len(delta_time);
-		fwrite(ev.buffer, 1, ev.size, _fd);
+		fwrite(ev.buffer(), 1, ev.size(), _fd);
 
-		_track_size += stamp_size + ev.size;
-		_write_data_count += ev.size;
+		_track_size += stamp_size + ev.size();
+		_write_data_count += ev.size();
 		
-		_last_ev_time = ev.time;
+		_last_ev_time = ev.time();
 	}
 
 	fflush(_fd);
diff --git a/libs/pbd/pbd/command.h b/libs/pbd/pbd/command.h
index 5d45f85ac3..a66485bc0d 100644
--- a/libs/pbd/pbd/command.h
+++ b/libs/pbd/pbd/command.h
@@ -30,11 +30,20 @@ public:
 
 	virtual void operator() () = 0;
 	
+	void set_name (const std::string& str) { _name = str; }
+	const std::string& name() const { return _name; }
+	
 	virtual void undo() = 0;
 	virtual void redo() { (*this)(); }
 	
 	virtual XMLNode &get_state();
 	virtual int set_state(const XMLNode&) { /* noop */ return 0; }
+
+protected:
+	Command() {}
+	Command(const std::string& name) : _name(name) {}
+
+	std::string _name;
 };
 
 #endif // __lib_pbd_command_h_
diff --git a/libs/pbd/pbd/undo.h b/libs/pbd/pbd/undo.h
index ea9a6d0e8b..5bfccf5a06 100644
--- a/libs/pbd/pbd/undo.h
+++ b/libs/pbd/pbd/undo.h
@@ -50,11 +50,6 @@ class UndoTransaction : public Command
 	void redo();
 
 	XMLNode &get_state();
-	
-	void set_name (const std::string& str) {
-		_name = str;
-	}
-	const std::string& name() const { return _name; }
 
 	void set_timestamp (struct timeval &t) {
 		_timestamp = t;
@@ -67,7 +62,6 @@ class UndoTransaction : public Command
   private:
 	std::list<Command*>    actions;
 	struct timeval        _timestamp;
-	std::string           _name;
 	bool                  _clearing;
 
 	friend void command_death (UndoTransaction*, Command *);
diff --git a/libs/pbd/undo.cc b/libs/pbd/undo.cc
index 5c7178b0d6..6db85e6ab3 100644
--- a/libs/pbd/undo.cc
+++ b/libs/pbd/undo.cc
@@ -32,14 +32,14 @@ using namespace std;
 using namespace sigc;
 
 UndoTransaction::UndoTransaction ()
+	: _clearing(false)
 {
-	_clearing = false;
 }
 
 UndoTransaction::UndoTransaction (const UndoTransaction& rhs)
+	: Command(rhs._name)
+	, _clearing(false)
 {
-	_name = rhs._name;
-	_clearing = false;
 	clear ();
 	actions.insert(actions.end(),rhs.actions.begin(),rhs.actions.end());
 }