View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0001084 | ardour | bugs | public | 2005-09-14 18:12 | 2020-04-19 20:12 |
| Reporter | audun | Assigned To | v2 | ||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | closed | Resolution | fixed | ||
| Summary | 0001084: Undo/redo on meter/tempo changes, strange behaviour | ||||
| Description | I 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. | ||||
| Tags | No tags attached. | ||||
|
|
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) |
|
|
I'm working on a patch to fix this myself. |
|
|
Patch submitted to ardour-dev. |
|
|
Applied Audun's patch and some extra code as the original patch didn't fix all the issues reported in this report. |
|
|
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();
|
|
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;
|
|
|
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. |
|
|
The patch was kindly committed to svn by paul :) I guess it's time to resolve again.. |
|
|
Resolving issue as requested quite a while ago as the issue was fixed;) |
|
|
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. |
| 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 |