View Issue Details

IDProjectCategoryView StatusLast Update
0001084ardourbugspublic2020-04-19 20:12
Reporteraudun Assigned Tov2  
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Summary0001084: Undo/redo on meter/tempo changes, strange behaviour
DescriptionI think there is a bug in undo/redo functionality concerning meter and tempo changes. This is reproducible every single time in beta30 (not in the drop-down box), and I think it has been present in previous versions too.
To reproduce (in this description i use the tempo changes, but the same bug exist with meter changes):
1. start new ardour session
2. ADD a new tempo, E.G. at bar 2 (tempo value doesn't matter)
3. ADD a new tempo, E.G. at bar 3
4. MOVE the newest tempo change to bar 4
5. UNDO this, the tempo change is now at bar 3
6. ADD a new tempo at bar 4
7. UNDO once, nothing happens.
8. UNDO twice, both tempos at bar 3 AND bar 4 disappears, but the only one that should be removed is the one at bar 4.

I also have a small feature request concerning tempo/meter changes:
Moving bar/meter changes while holding down the Control-key should copy these changes.
TagsNo tags attached.

Activities

audun

2006-03-05 00:51

reporter   ~0002451

I submitted a patch to the mailinglist that adds my feature request.

I also have made several new observations regarding the undo bug:
Observation 0000001:
ADD a new marker
MOVE the marker
UNDO. The marker disappears, although it should move to its initial position.

Observation #2:
OPEN a new session with (initial) tempo != 120
ADD a new meter marker or tempo marker
UNDO. The initial tempo (160 in my test session) now changes to 120! (120 being the default tempo of a new session may have someting to do with this)

audun

2006-03-05 21:58

reporter   ~0002452

I'm working on a patch to fix this myself.

audun

2006-03-06 03:36

reporter   ~0002453

Last edited: 2006-03-15 01:47

Patch submitted to ardour-dev.

v2

2006-03-21 18:42

developer   ~0002464

Applied Audun's patch and some extra code as the original patch didn't fix all the issues reported in this report.

audun

2007-03-20 23:46

reporter   ~0003624

Last edited: 2007-03-21 02:37

Problems still remain with this issue.
After examining code a bit, I have found that when setting the state on the tempomap, the function Editor::draw_metric_marks never gets called.
This means if I add a tempo marker, it will get both an internal libardour representation and a gui representation by calling draw_metric_marks 'explicitly' (by that I mean that it doesn't get called later via various callbacks). When this command is undoed, a call to TempoMap::set_state is done which signals StateChanged. Editor::tempo_map_changed then gets called by callback, but that one never calls Editor::draw_metric_marks.

If the Change argument carried information about whether something was added or removed from the tempomap, draw_metric_marks could be called based on that information. I don't think it is wise to call it every time from tempo_map_changed, since that function gets called every time the canvas is scrolled (it removes all points and then creates them again, wouldn't this be a waste of cpu time?).

Anyway, I have a two-liner patch to resolve some problems with copy-drag metrics. All it does is to call draw_metric_marks when the item is released on the timeline. If that isn't called, the copied metric mark wouldn't be visible until draw_metric_marks gets called for another reason.

2007-03-20 23:54

 

ardour2fixcopydrag.patch (1,035 bytes)   
Index: gtk2_ardour/editor_mouse.cc
===================================================================
--- gtk2_ardour/editor_mouse.cc (revision 1631)
+++ gtk2_ardour/editor_mouse.cc (working copy)
@@ -2354,6 +2354,7 @@
                // delete the dummy marker we used for visual representation of copying.
                // a new visual marker will show up automatically.
                delete marker;
+               map.apply_with_metrics (*this, &Editor::draw_metric_marks);
        } else {
                begin_reversible_command (_("move meter mark"));
                XMLNode &before = map.get_state();
@@ -2486,6 +2487,7 @@
                // delete the dummy marker we used for visual representation of copying.
                // a new visual marker will show up automatically.
                delete marker;
+               map.apply_with_metrics (*this, &Editor::draw_metric_marks);
        } else {
                begin_reversible_command (_("move tempo mark"));
                 XMLNode &before = map.get_state();
ardour2fixcopydrag.patch (1,035 bytes)   

2007-03-27 01:07

 

fixmetricsundo.patch (5,207 bytes)   
Index: gtk2_ardour/editor_canvas.cc
===================================================================
--- gtk2_ardour/editor_canvas.cc	(revision 1640)
+++ gtk2_ardour/editor_canvas.cc	(working copy)
@@ -368,7 +368,7 @@
 	}
 		
 	update_fixed_rulers();
-	tempo_map_changed (Change (0), true);
+	redisplay_tempo (true);
 	
 	Resized (); /* EMIT_SIGNAL */
 
@@ -723,6 +723,6 @@
 	
 	update_fixed_rulers ();
 
-	tempo_map_changed (Change (0), !_dragging_hscrollbar);
+	redisplay_tempo (!_dragging_hscrollbar);
 }
 
Index: gtk2_ardour/editor_tempodisplay.cc
===================================================================
--- gtk2_ardour/editor_tempodisplay.cc	(revision 1640)
+++ gtk2_ardour/editor_tempodisplay.cc	(working copy)
@@ -93,14 +93,29 @@
 }
 
 void
-Editor::tempo_map_changed (Change ignored, bool immediate_redraw)
+Editor::tempo_map_changed (Change ignored)
 {
 	if (!session) {
 		return;
 	}
 
-        ENSURE_GUI_THREAD(bind (mem_fun (*this, &Editor::tempo_map_changed), ignored, immediate_redraw));
+	ENSURE_GUI_THREAD(bind (mem_fun (*this, &Editor::tempo_map_changed), ignored));
+	
+	redisplay_tempo (false); // redraw rulers and measures
+	session->tempo_map().apply_with_metrics (*this, &Editor::draw_metric_marks); // redraw metric markers
+}
 
+/**
+ * This code was originally in tempo_map_changed, but this is called every time the canvas scrolls horizontally. 
+ * That's why this is moved in here. The new tempo_map_changed is called when the ARDOUR::TempoMap actually changed.
+ */
+void
+Editor::redisplay_tempo (bool immediate_redraw)
+{
+	if (!session) {
+		return;
+	}
+
 	BBT_Time previous_beat, next_beat; // the beats previous to the leftmost frame and after the rightmost frame
 
 	session->bbt_time(leftmost_frame, previous_beat);
@@ -153,11 +168,6 @@
 }
 
 void
-Editor::redisplay_tempo ()
-{	
-}
-
-void
 Editor::hide_measures ()
 {
 	for (TimeLineList::iterator i = used_measure_lines.begin(); i != used_measure_lines.end(); ++i) {
@@ -307,8 +317,6 @@
 	commit_reversible_command ();
 	
 	map.dump (cerr);
-
-	session->tempo_map().apply_with_metrics (*this, &Editor::draw_metric_marks);
 }
 
 void
@@ -349,8 +357,6 @@
 	commit_reversible_command ();
 	
 	map.dump (cerr);
-
-	session->tempo_map().apply_with_metrics (*this, &Editor::draw_metric_marks);
 }
 
 void
@@ -401,8 +407,6 @@
         XMLNode &after = session->tempo_map().get_state();
 	session->add_command(new MementoCommand<TempoMap>(session->tempo_map(), &before, &after));
 	commit_reversible_command ();
-
-	session->tempo_map().apply_with_metrics (*this, &Editor::draw_metric_marks);
 }
 
 void
@@ -433,8 +437,6 @@
         XMLNode &after = session->tempo_map().get_state();
 	session->add_command (new MementoCommand<TempoMap>(session->tempo_map(), &before, &after));
 	commit_reversible_command ();
-
-	session->tempo_map().apply_with_metrics (*this, &Editor::draw_metric_marks);
 }
 
 void
@@ -485,8 +487,6 @@
 	session->add_command(new MementoCommand<TempoMap>(session->tempo_map(), &before, &after));
 	commit_reversible_command ();
 
-	session->tempo_map().apply_with_metrics (*this, &Editor::draw_metric_marks);
-
 	return FALSE;
 }
 
@@ -521,7 +521,5 @@
 	session->add_command(new MementoCommand<TempoMap>(session->tempo_map(), &before, &after));
 	commit_reversible_command ();
 
-	session->tempo_map().apply_with_metrics (*this, &Editor::draw_metric_marks);
-
 	return FALSE;
 }
Index: gtk2_ardour/editor.cc
===================================================================
--- gtk2_ardour/editor.cc	(revision 1640)
+++ gtk2_ardour/editor.cc	(working copy)
@@ -1075,7 +1075,7 @@
 
 	session_connections.push_back (session->SMPTEOffsetChanged.connect (mem_fun(*this, &Editor::update_just_smpte)));
 
-	session_connections.push_back (session->tempo_map().StateChanged.connect (bind (mem_fun(*this, &Editor::tempo_map_changed), false)));
+	session_connections.push_back (session->tempo_map().StateChanged.connect (mem_fun(*this, &Editor::tempo_map_changed)));
 
 	edit_groups_changed ();
 
@@ -3665,7 +3665,7 @@
 			/* the signal handler will do the rest */
 		} else {
 			update_fixed_rulers();
-			tempo_map_changed (Change (0), true);
+			redisplay_tempo (true);
 		}
 	}
 
Index: gtk2_ardour/editor_rulers.cc
===================================================================
--- gtk2_ardour/editor_rulers.cc	(revision 1640)
+++ gtk2_ardour/editor_rulers.cc	(working copy)
@@ -709,7 +709,7 @@
 	
 	update_fixed_rulers();
 	//update_tempo_based_rulers();
-	tempo_map_changed(Change (0), false);
+	redisplay_tempo (false);
 
 	time_canvas_event_box.show_all();
 	time_button_frame.show_all();
Index: gtk2_ardour/editor.h
===================================================================
--- gtk2_ardour/editor.h	(revision 1640)
+++ gtk2_ardour/editor.h	(working copy)
@@ -1275,8 +1275,8 @@
 	void remove_metric_marks ();
 	void draw_metric_marks (const ARDOUR::Metrics& metrics);
 
-	void tempo_map_changed (ARDOUR::Change, bool immediate_redraw);
-	void redisplay_tempo ();
+	void tempo_map_changed (ARDOUR::Change);
+	void redisplay_tempo (bool immediate_redraw);
 	
 	void snap_to (nframes_t& first, int32_t direction = 0, bool for_mark = false);
 	uint32_t bbt_beat_subdivision;
fixmetricsundo.patch (5,207 bytes)   

audun

2007-03-27 01:19

reporter   ~0003669

Last edited: 2007-03-27 02:42

The new patch, fixmetricsundo.patch (ignore the first one), fixes the problem. What it does is that it moves the code previously in tempo_map_changed (which only redrew rulers and bars) into redisplay_tempo. The new tempo_map_changed does two things: it redraws metric marks (draw_metric_marks), and redraws rulers and bars (redisplay_tempo), and is only called when ARDOUR::TempoMap signals StateChanged.

There is probably a better descriptive name for the new redisplay_tempo function. It already existed, but it was empty, that's why i just stole that name. The function signature changed to add the immediate_redraw argument though.

audun

2007-04-03 02:58

reporter   ~0003699

The patch was kindly committed to svn by paul :) I guess it's time to resolve again..

seablade

2009-07-05 04:52

manager   ~0006320

Resolving issue as requested quite a while ago as the issue was fixed;)

system

2020-04-19 20:12

developer   ~0021468

Issue has been closed automatically, by Trigger Close Plugin.
Feel free to re-open with additional information if you think the issue is not resolved.

Issue History

Date Modified Username Field Change
2005-09-14 18:12 audun New Issue
2006-03-05 00:51 audun Note Added: 0002451
2006-03-05 21:58 audun Note Added: 0002452
2006-03-06 03:36 audun Note Added: 0002453
2006-03-15 01:47 audun Note Edited: 0002453
2006-03-21 18:42 v2 Status new => resolved
2006-03-21 18:42 v2 Resolution open => fixed
2006-03-21 18:42 v2 Assigned To => v2
2006-03-21 18:42 v2 Note Added: 0002464
2007-03-20 23:46 audun Status resolved => feedback
2007-03-20 23:46 audun Resolution fixed => reopened
2007-03-20 23:46 audun Note Added: 0003624
2007-03-20 23:54 audun File Added: ardour2fixcopydrag.patch
2007-03-20 23:58 audun Note Edited: 0003624
2007-03-21 02:37 audun Note Edited: 0003624
2007-03-27 01:07 audun File Added: fixmetricsundo.patch
2007-03-27 01:19 audun Note Added: 0003669
2007-03-27 02:42 audun Note Edited: 0003669
2007-04-03 02:58 audun Note Added: 0003699
2009-07-05 04:52 seablade Note Added: 0006320
2009-07-05 04:52 seablade Status feedback => resolved
2009-07-05 04:52 seablade Resolution reopened => fixed
2009-07-05 04:52 seablade Description Updated
2020-04-19 20:12 system Note Added: 0021468
2020-04-19 20:12 system Status resolved => closed