View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0004114 | ardour | bugs | public | 2011-06-13 13:05 | 2012-06-11 12:59 |
| Reporter | colinf | Assigned To | |||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | acknowledged | Resolution | open | ||
| Target Version | 3.0 | ||||
| Summary | 0004114: Adding xrun markers should not appear separately in undo history | ||||
| Description | If '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. | ||||
| Tags | No tags attached. | ||||
|
|
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. |
|
|
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? |
|
|
its not obvious what that would break, since in theory we don't allow edits while recording. worth trying it. |
|
|
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 ();
}
|
|
|
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? |
|
|
I think that's probably the least worst solution :) |
|
|
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. |
| 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 |
