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 |