View Issue Details

IDProjectCategoryView StatusLast Update
0004114ardourbugspublic2012-06-11 12:59
Reportercolinf Assigned To 
PrioritynormalSeverityminorReproducibilityalways
Status acknowledgedResolutionopen 
Target Version3.0 
Summary0004114: Adding xrun markers should not appear separately in undo history
DescriptionIf 'Create markers where xruns occur' is ticked and an xrun occurs during capture, then undoing the capture leaves the next item to be undone as 'add marker'.

I'd expect undoing the capture to also undo the xrun markers added during that capture.
TagsNo tags attached.

Activities

paul

2011-06-15 11:23

administrator   ~0010891

i mostly agree, but just as a note: the problem with the second sentence in the description is that the xrun markers are created *during* capture, whereas the changes to the session that represent "capture" occur only *after* the transport stops.

colinf

2011-06-15 11:32

updater   ~0010895

Yes, I already discovered this whilst trying to cook up a patch: I was about to post a note about it myself.

Would bad things happen if one were to move the call to

    begin_reversible_command (Operations::capture);

from its current place in Session::non_realtime_stop to somewhere where it would be called when capture is started rather than stopped? If not, where might a good place for that be?

paul

2011-06-15 13:08

administrator   ~0010900

its not obvious what that would break, since in theory we don't allow edits while recording. worth trying it.

colinf

2011-06-20 15:08

updater   ~0010912

Well, I can't get this to work how I'd want. Maybe someone less dim than me can work out why.

I tried moving begin_reversible_command (Operations::capture); to Session::enable_record(), but that doesn't seem to be enough to cause the addition of the xrun markers to be considered as part of the capture operation for the purpose of undo, even though it's called before any xrun markers are created.

Even if it did, it's probably not the right place to move it to, because Session::enable_record() is called every time record is enabled, so if you drop out of record and then in again without stopping the transport (e.g. when using punch in/out in loop mode) there would be multiple begin_reversible_command()s for only one commit_reversible_command() in Session::non_realtime_stop() when the transport is finally stopped.

I looked for, but couldn't find, a single place that is called when recording is enabled for the first time since the transport last started playing. I guess there must be such a place, but I don't know where.

I'll attach my current patch in a moment for completeness' sake, in case anyone wants to take a look.

2011-06-20 15:08

 

xrun-markers-9751.patch (6,095 bytes)   
Index: gtk2_ardour/public_editor.h
===================================================================
--- gtk2_ardour/public_editor.h	(revision 9751)
+++ gtk2_ardour/public_editor.h	(working copy)
@@ -271,7 +271,7 @@
 	virtual framepos_t get_preferred_edit_position (bool ignore_playhead = false) = 0;
 	virtual void toggle_meter_updating() = 0;
 	virtual void split_region_at_points (boost::shared_ptr<ARDOUR::Region>, ARDOUR::AnalysisFeatureList&, bool can_ferret, bool select_new = false) = 0;
-	virtual void mouse_add_new_marker (framepos_t where, bool is_cd=false, bool is_xrun=false) = 0;
+	virtual void mouse_add_new_marker (framepos_t where, bool is_cd=false) = 0;
 	virtual void foreach_time_axis_view (sigc::slot<void,TimeAxisView&>) = 0;
 	virtual void add_to_idle_resize (TimeAxisView*, int32_t) = 0;
 	virtual framecnt_t get_nudge_distance (framepos_t pos, framecnt_t& next) = 0;
Index: gtk2_ardour/ardour_ui.cc
===================================================================
--- gtk2_ardour/ardour_ui.cc	(revision 9751)
+++ gtk2_ardour/ardour_ui.cc	(working copy)
@@ -3212,7 +3212,26 @@
 void
 ARDOUR_UI::create_xrun_marker (framepos_t where)
 {
-	editor->mouse_add_new_marker (where, false, true);
+	// editor->mouse_add_new_marker (where, false, true);
+
+	if (_session) {
+		/* adding xrun markers doesn't need to be undoable:
+		 * just add it, and don't make_current either.
+		 *
+		 * XXX doesn't do the right thing yet, since the xrun 
+		 * markers are created during capture, 
+		 * but begin_reversible_command (Operations::capture);
+		 * doesn't happen until the transport is stopped after
+		 * the capture.
+		 */
+		 
+		string markername;
+
+		_session->locations()->next_available_name(markername, _("xrun"));
+		Location *location = new Location (*_session, where, where, markername, Location::IsMark);
+		_session->locations()->add (location, false);
+	}
+
 }
 
 void
Index: gtk2_ardour/editor_markers.cc
===================================================================
--- gtk2_ardour/editor_markers.cc	(revision 9751)
+++ gtk2_ardour/editor_markers.cc	(working copy)
@@ -626,24 +626,18 @@
 }
 
 void
-Editor::mouse_add_new_marker (framepos_t where, bool is_cd, bool is_xrun)
+Editor::mouse_add_new_marker (framepos_t where, bool is_cd)
 {
-	string markername, markerprefix;
+	string markername;
 	int flags = (is_cd ? Location::IsCDMarker|Location::IsMark : Location::IsMark);
 
-	if (is_xrun) {
-		markerprefix = "xrun";
-		flags = Location::IsMark;
-	} else {
-		markerprefix = "mark";
-	}
-
 	if (_session) {
-		_session->locations()->next_available_name(markername, markerprefix);
-		if (!is_xrun && !choose_new_marker_name(markername)) {
+		_session->locations()->next_available_name(markername, _("mark"));
+		Location *location = new Location (*_session, where, where, markername, (Location::Flags) flags);
+
+		if (!choose_new_marker_name(markername)) {
 			return;
 		}
-		Location *location = new Location (*_session, where, where, markername, (Location::Flags) flags);
 		_session->begin_reversible_command (_("add marker"));
 		XMLNode &before = _session->locations()->get_state();
 		_session->locations()->add (location, true);
Index: gtk2_ardour/editor_rulers.cc
===================================================================
--- gtk2_ardour/editor_rulers.cc	(revision 9751)
+++ gtk2_ardour/editor_rulers.cc	(working copy)
@@ -337,7 +337,7 @@
 
 	switch (t) {
 	case MarkerBarItem:
-		ruler_items.push_back (MenuElem (_("New location marker"), sigc::bind ( sigc::mem_fun(*this, &Editor::mouse_add_new_marker), where, false, false)));
+		ruler_items.push_back (MenuElem (_("New location marker"), sigc::bind ( sigc::mem_fun(*this, &Editor::mouse_add_new_marker), where, false)));
 		ruler_items.push_back (MenuElem (_("Clear all locations"), sigc::mem_fun(*this, &Editor::clear_markers)));
 		ruler_items.push_back (MenuElem (_("Unhide locations"), sigc::mem_fun(*this, &Editor::unhide_markers)));
 		ruler_items.push_back (SeparatorElem ());
@@ -355,7 +355,7 @@
 
 	case CdMarkerBarItem:
 		// TODO
-		ruler_items.push_back (MenuElem (_("New CD track marker"), sigc::bind ( sigc::mem_fun(*this, &Editor::mouse_add_new_marker), where, true, false)));
+		ruler_items.push_back (MenuElem (_("New CD track marker"), sigc::bind ( sigc::mem_fun(*this, &Editor::mouse_add_new_marker), where, true)));
 		break;
 
 
Index: gtk2_ardour/editor.h
===================================================================
--- gtk2_ardour/editor.h	(revision 9751)
+++ gtk2_ardour/editor.h	(working copy)
@@ -605,7 +605,7 @@
 
 	void hide_marker (ArdourCanvas::Item*, GdkEvent*);
 	void clear_marker_display ();
-	void mouse_add_new_marker (framepos_t where, bool is_cd=false, bool is_xrun=false);
+	void mouse_add_new_marker (framepos_t where, bool is_cd=false);
 	bool choose_new_marker_name(std::string &name);
 	void update_cd_marker_display ();
 	void ensure_cd_marker_updated (LocationMarkers * lam, ARDOUR::Location * location);
Index: libs/ardour/session.cc
===================================================================
--- libs/ardour/session.cc	(revision 9751)
+++ libs/ardour/session.cc	(working copy)
@@ -985,6 +985,7 @@
 void
 Session::enable_record ()
 {
+
 	while (1) {
 		RecordState rs = (RecordState) g_atomic_int_get (&_record_status);
 
@@ -1005,6 +1006,11 @@
 			break;
 		}
 	}
+	
+	// XXX this doesn't belong here, but where will it owrk?
+	cerr << "begin_reversible_command (Operations::capture);" << endl;
+	begin_reversible_command (Operations::capture);
+
 }
 
 void
Index: libs/ardour/session_transport.cc
===================================================================
--- libs/ardour/session_transport.cc	(revision 9751)
+++ libs/ardour/session_transport.cc	(working copy)
@@ -463,7 +463,7 @@
 	reset_rf_scale (0);
 
 	if (did_record) {
-		begin_reversible_command (Operations::capture);
+		// begin_reversible_command (Operations::capture);
 		_have_captured = true;
 	}
 
@@ -495,6 +495,7 @@
 	}
 
 	if (did_record) {
+		cerr << "commit_reversible_command ();" << endl;
 		commit_reversible_command ();
 	}
 
xrun-markers-9751.patch (6,095 bytes)   

colinf

2012-03-19 20:05

updater   ~0012983

I thought about this a bit more, and it's a tricky one.

Really, begin_reversible_command (Operations::capture); must happen when the transport stops after a capture, since otherwise changes made after the begin_reversible_command() but before the transport stops end up entangled in the capture command in the undo history. So, if the addition of xrun markers should be considered a part of the capture operation, they must all be added at transport stop.

That would be simple enough to do (create a list of xrun locations as they occur, and create all the markers at stop), but it is good to see the xruns as they occur, too. I wonder whether creating the markers as now, but also keeping the list of locations, and then removing and re-adding the markers at stop, would be too gross?

cth103

2012-06-03 14:32

administrator   ~0013362

I think that's probably the least worst solution :)

colinf

2012-06-06 12:07

updater   ~0013394

I already got about half-way through doing this a couple of months ago before becoming distracted by something-or-other (it doesn't take very much to distract me, unfortunately).

I'll dust down the patch & see if I can get it working when I get a moment - maybe next week.

Issue History

Date Modified Username Field Change
2011-06-13 13:05 colinf New Issue
2011-06-13 22:54 cth103 cost => 0.00
2011-06-13 22:54 cth103 Target Version => 3.0-beta1
2011-06-15 11:23 paul Note Added: 0010891
2011-06-15 11:32 colinf Note Added: 0010895
2011-06-15 13:08 paul Note Added: 0010900
2011-06-20 15:08 colinf Note Added: 0010912
2011-06-20 15:08 colinf File Added: xrun-markers-9751.patch
2011-11-15 15:17 cth103 Target Version 3.0-beta1 => 3.0-beta2
2012-01-10 20:46 cth103 Target Version 3.0-beta2 => 3.0-beta3
2012-02-14 17:20 paul Target Version 3.0-beta3 => 3.0 beta4
2012-03-19 20:05 colinf Note Added: 0012983
2012-05-23 15:08 cth103 Target Version 3.0 beta4 => 3.0
2012-06-03 14:32 cth103 Note Added: 0013362
2012-06-03 14:32 cth103 Status new => feedback
2012-06-06 12:07 colinf Note Added: 0013394
2012-06-11 12:59 cth103 Status feedback => acknowledged