Functional: marker undo now works without deadlocks; overlapping skip

ranges work as agreed with Igor.

Implementation: change semantics of Location signals, change Locations
API, fix Session to handle signals correctly, clean up use of
Locations::apply() to avoid deadlock

gtk2_ardour/editor.cc

@@ -1409,10 +1409,9 @@ Editor::set_session (Session *t)
 	_session->locations()->added.connect (_session_connections, invalidator (*this), boost::bind (&Editor::add_new_location, this, _1), gui_context());
 	_session->locations()->removed.connect (_session_connections, invalidator (*this), boost::bind (&Editor::location_gone, this, _1), gui_context());
 	_session->locations()->changed.connect (_session_connections, invalidator (*this), boost::bind (&Editor::refresh_location_display, this), gui_context());
-	_session->locations()->StateChanged.connect (_session_connections, invalidator (*this), boost::bind (&Editor::refresh_location_display, this), gui_context());
 	_session->history().Changed.connect (_session_connections, invalidator (*this), boost::bind (&Editor::history_changed, this), gui_context());
-    _session->RecordStateChanged.connect (_session_connections, MISSING_INVALIDATOR, boost::bind (&Editor::start_lock_event_timing, this), gui_context());
-    _session->RecordStateChanged.connect (_session_connections, invalidator (*this), boost::bind (&Editor::start_session_auto_save_event_timing, this), gui_context());
+        _session->RecordStateChanged.connect (_session_connections, MISSING_INVALIDATOR, boost::bind (&Editor::start_lock_event_timing, this), gui_context());
+        _session->RecordStateChanged.connect (_session_connections, invalidator (*this), boost::bind (&Editor::start_session_auto_save_event_timing, this), gui_context());
     
 	playhead_cursor->show ();
 

gtk2_ardour/editor.h

@@ -569,7 +569,7 @@ class Editor : public PublicEditor, public PBD::ScopedConnectionList, public ARD
 	void location_changed (ARDOUR::Location *);
 	void location_flags_changed (ARDOUR::Location *);
 	void refresh_location_display ();
-	void refresh_location_display_internal (ARDOUR::Locations::LocationList&);
+	void refresh_location_display_internal (const ARDOUR::Locations::LocationList&);
 	void add_new_location (ARDOUR::Location *);
 	ArdourCanvas::Container* add_new_location_internal (ARDOUR::Location *);
 	void location_gone (ARDOUR::Location *);

gtk2_ardour/editor_markers.cc

@@ -294,7 +294,7 @@ Editor::find_location_from_marker (Marker *marker, bool& is_start) const
 }
 
 void
-Editor::refresh_location_display_internal (Locations::LocationList& locations)
+Editor::refresh_location_display_internal (const Locations::LocationList& locations)
 {
 	/* invalidate all */
 
@@ -304,7 +304,7 @@ Editor::refresh_location_display_internal (Locations::LocationList& locations)
 
 	/* add new ones */
 
-	for (Locations::LocationList::iterator i = locations.begin(); i != locations.end(); ++i) {
+	for (Locations::LocationList::const_iterator i = locations.begin(); i != locations.end(); ++i) {
 
 		LocationMarkerMap::iterator x;
 

gtk2_ardour/location_ui.cc

@@ -959,7 +959,7 @@ LocationUI::location_removed (Location* location)
 }
 
 void
-LocationUI::map_locations (Locations::LocationList& locations)
+LocationUI::map_locations (const Locations::LocationList& locations)
 {
 	Locations::LocationList::iterator i;
 	gint n;
@@ -968,9 +968,8 @@ LocationUI::map_locations (Locations::LocationList& locations)
 	LocationSortByStart cmp;
 
 	temp.sort (cmp);
-	locations = temp;
 
-	for (n = 0, i = locations.begin(); i != locations.end(); ++n, ++i) {
+	for (n = 0, i = temp.begin(); i != temp.end(); ++n, ++i) {
 
 		Location* location = *i;
 
@@ -1074,10 +1073,10 @@ LocationUI::set_session(ARDOUR::Session* s)
 	SessionHandlePtr::set_session (s);
 
 	if (_session) {
-		_session->locations()->changed.connect (_session_connections, invalidator (*this), boost::bind (&LocationUI::locations_changed, this, _1), gui_context());
-		_session->locations()->StateChanged.connect (_session_connections, invalidator (*this), boost::bind (&LocationUI::refresh_location_list, this), gui_context());
 		_session->locations()->added.connect (_session_connections, invalidator (*this), boost::bind (&LocationUI::location_added, this, _1), gui_context());
 		_session->locations()->removed.connect (_session_connections, invalidator (*this), boost::bind (&LocationUI::location_removed, this, _1), gui_context());
+		_session->locations()->changed.connect (_session_connections, invalidator (*this), boost::bind (&LocationUI::refresh_location_list, this), gui_context());
+
 		_clock_group->set_clock_mode (clock_mode_from_session_instant_xml ());
 	}
 
@@ -1087,17 +1086,6 @@ LocationUI::set_session(ARDOUR::Session* s)
 	refresh_location_list ();
 }
 
-void
-LocationUI::locations_changed (Locations::Change c)
-{
-	/* removal is signalled by both a removed and a changed signal emission from Locations,
-	   so we don't need to refresh the list on a removal
-	*/
-	if (c != Locations::REMOVAL) {
-		refresh_location_list ();
-	}
-}
-
 void
 LocationUI::session_going_away()
 {

gtk2_ardour/location_ui.h

@@ -202,8 +202,8 @@ class LocationUI : public Gtk::HBox, public ARDOUR::SessionHandlePtr
 
 	void location_removed (ARDOUR::Location *);
 	void location_added (ARDOUR::Location *);
-	void locations_changed (ARDOUR::Locations::Change);
-	void map_locations (ARDOUR::Locations::LocationList&);
+	void locations_changed ();
+	void map_locations (const ARDOUR::Locations::LocationList&);
 
         ClockGroup* _clock_group;
 	AudioClock::Mode clock_mode_from_session_instant_xml () const;

libs/ardour/ardour/location.h

@@ -200,28 +200,26 @@ class LIBARDOUR_API Locations : public SessionHandleRef, public PBD::StatefulDes
 
 	void find_all_between (framepos_t start, framepos_t, LocationList&, Location::Flags);
 
-	enum Change {
-		ADDITION, ///< a location was added, but nothing else changed
-		REMOVAL, ///< a location was removed, but nothing else changed
-		OTHER ///< something more complicated happened
-	};
-
 	PBD::Signal1<void,Location*> current_changed;
-	/** something changed about the location list; the parameter gives some idea as to what */
-	PBD::Signal1<void,Change>    changed;
-	/** a location has been added to the end of the list */
+
+        /* Objects that care about individual addition and removal of Locations should connect to added/removed.
+           If an object additionally cares about potential mass clearance of Locations, they should connect to changed.
+        */
+
 	PBD::Signal1<void,Location*> added;
 	PBD::Signal1<void,Location*> removed;
-	PBD::Signal1<void,const PBD::PropertyChange&>    StateChanged;
+	PBD::Signal0<void> changed; /* emitted when any action that could have added/removed more than 1 location actually removed 1 or more */
 
-	template<class T> void apply (T& obj, void (T::*method)(LocationList&)) {
-		Glib::Threads::Mutex::Lock lm (lock);
-		(obj.*method)(locations);
-	}
-
-	template<class T1, class T2> void apply (T1& obj, void (T1::*method)(LocationList&, T2& arg), T2& arg) {
-		Glib::Threads::Mutex::Lock lm (lock);
-		(obj.*method)(locations, arg);
+	template<class T> void apply (T& obj, void (T::*method)(const LocationList&)) const {
+                /* We don't want to hold the lock while the given method runs, so take a copy
+                   of the list and pass that instead.
+                */
+                Locations::LocationList copy;
+                {
+                        Glib::Threads::Mutex::Lock lm (lock);
+                        copy = locations;
+                }
+		(obj.*method)(copy);
 	}
 
   private:

libs/ardour/ardour/midi_scene_changer.h

@@ -57,7 +57,7 @@ class MIDISceneChanger : public SceneChanger
 	int last_delivered_program;
 	int last_delivered_bank;
 
-	void gather ();
+	void gather (const Locations::LocationList&);
 	bool recording () const;
 	void jump_to (int bank, int program);
 	void rt_deliver (MidiBuffer&, framepos_t, boost::shared_ptr<MIDISceneChange>);
@@ -65,7 +65,7 @@ class MIDISceneChanger : public SceneChanger
 
 	void bank_change_input (MIDI::Parser&, unsigned short, int channel);
 	void program_change_input (MIDI::Parser&, MIDI::byte, int channel);
-	void locations_changed (Locations::Change);
+	void locations_changed ();
 
 	PBD::ScopedConnectionList incoming_connections;
 };

libs/ardour/ardour/session.h

@@ -1187,10 +1187,15 @@ class LIBARDOUR_API Session : public PBD::StatefulDestructible, public PBD::Scop
 	void reset_rf_scale (framecnt_t frames_moved);
 
 	Locations*       _locations;
-	void              locations_changed ();
-	void              locations_added (Location*);
-	void              handle_locations_changed (Locations::LocationList&);
-	void              sync_locations_to_skips (Locations::LocationList&);
+	void              location_added (Location*);
+	void              location_removed (Location*);
+        void              locations_changed ();
+        void             _locations_changed (const Locations::LocationList&);
+
+        void              update_skips (Location*, bool consolidate);
+        Locations::LocationList consolidate_skips (Location*);
+	void              sync_locations_to_skips (const Locations::LocationList&);
+        PBD::ScopedConnectionList skip_connections;
 
 	PBD::ScopedConnectionList punch_connections;
 	void             auto_punch_start_changed (Location *);
@@ -1645,7 +1650,7 @@ class LIBARDOUR_API Session : public PBD::StatefulDestructible, public PBD::Scop
 	/** temporary list of Diskstreams used only during load of 2.X sessions */
 	std::list<boost::shared_ptr<Diskstream> > _diskstreams_2X;
 
-	void add_session_range_location (framepos_t, framepos_t);
+	void set_session_range_location (framepos_t, framepos_t);
 
 	void setup_midi_machine_control ();
 
@@ -1655,7 +1660,7 @@ class LIBARDOUR_API Session : public PBD::StatefulDestructible, public PBD::Scop
 	/** true if timecode transmission by the transport is suspended, otherwise false */
 	mutable gint _suspend_timecode_transmission;
 
-	void update_locations_after_tempo_map_change (Locations::LocationList &);
+	void update_locations_after_tempo_map_change (const Locations::LocationList &);
 
 	void start_time_changed (framepos_t);
 	void end_time_changed (framepos_t);

libs/ardour/location.cc

@@ -749,11 +749,6 @@ Locations::Locations (Session& s)
 	: SessionHandleRef (s)
 {
 	current_location = 0;
-
-	Location::changed.connect_same_thread (*this, boost::bind (&Locations::location_changed, this, _1));
-	Location::start_changed.connect_same_thread (*this, boost::bind (&Locations::location_changed, this, _1));
-	Location::end_changed.connect_same_thread (*this, boost::bind (&Locations::location_changed, this, _1));
-	Location::flags_changed.connect_same_thread (*this, boost::bind (&Locations::location_changed, this, _1));
 }
 
 Locations::~Locations ()
@@ -852,7 +847,7 @@ Locations::clear ()
 		current_location = 0;
 	}
 
-	changed (OTHER); /* EMIT SIGNAL */
+	changed (); /* EMIT SIGNAL */
 	current_changed (0); /* EMIT SIGNAL */
 }
 
@@ -875,8 +870,8 @@ Locations::clear_markers ()
 			i = tmp;
 		}
 	}
-
-	changed (OTHER); /* EMIT SIGNAL */
+        
+	changed (); /* EMIT SIGNAL */
 }
 
 void
@@ -914,7 +909,7 @@ Locations::clear_ranges ()
 		current_location = 0;
 	}
 
-	changed (OTHER); /* EMIT SIGNAL */
+        changed ();
 	current_changed (0); /* EMIT SIGNAL */
 }
 
@@ -975,21 +970,13 @@ Locations::remove (Location *loc)
 	if (was_removed) {
 
 		removed (loc); /* EMIT SIGNAL */
-
+                
 		if (was_current) {
 			 current_changed (0); /* EMIT SIGNAL */
 		}
-
-		changed (REMOVAL); /* EMIT_SIGNAL */
 	}
 }
 
-void
-Locations::location_changed (Location* /*loc*/)
-{
-	changed (OTHER); /* EMIT SIGNAL */
-}
-
 XMLNode&
 Locations::get_state ()
 {
@@ -1099,7 +1086,7 @@ Locations::set_state (const XMLNode& node, int version)
 		}
 	}
 
-	changed (OTHER); /* EMIT SIGNAL */
+	changed (); /* EMIT SIGNAL */
 
 	return 0;
 }

libs/ardour/midi_scene_changer.cc

@@ -42,8 +42,13 @@ MIDISceneChanger::MIDISceneChanger (Session& s)
 	, last_delivered_bank (-1)
 	  
 {
-	_session.locations()->changed.connect_same_thread (*this, boost::bind (&MIDISceneChanger::locations_changed, this, _1));
-	Location::scene_changed.connect_same_thread (*this, boost::bind (&MIDISceneChanger::gather, this));
+        /* catch any add/remove/clear etc. for all Locations */
+	_session.locations()->changed.connect_same_thread (*this, boost::bind (&MIDISceneChanger::locations_changed, this));
+	_session.locations()->added.connect_same_thread (*this, boost::bind (&MIDISceneChanger::locations_changed, this));
+	_session.locations()->removed.connect_same_thread (*this, boost::bind (&MIDISceneChanger::locations_changed, this));
+
+        /* catch class-based signal that notifies of us changes in the scene change state of any Location */
+	Location::scene_changed.connect_same_thread (*this, boost::bind (&MIDISceneChanger::locations_changed, this));
 }
 
 MIDISceneChanger::~MIDISceneChanger ()
@@ -51,9 +56,9 @@ MIDISceneChanger::~MIDISceneChanger ()
 }
 
 void
-MIDISceneChanger::locations_changed (Locations::Change)
+MIDISceneChanger::locations_changed ()
 {
-	gather ();
+	_session.locations()->apply (*this, &MIDISceneChanger::gather);
 }
 
 /** Use the session's list of locations to collect all patch changes.
@@ -61,9 +66,8 @@ MIDISceneChanger::locations_changed (Locations::Change)
  * This is called whenever the locations change in anyway.
  */
 void
-MIDISceneChanger::gather ()
+MIDISceneChanger::gather (const Locations::LocationList& locations)
 {
-	const Locations::LocationList& locations (_session.locations()->list());
 	boost::shared_ptr<SceneChange> sc;
 
 	Glib::Threads::RWLock::WriterLock lm (scene_lock);

libs/ardour/session.cc

@@ -1378,73 +1378,165 @@ Session::set_auto_loop_location (Location* location)
 }
 
 void
-Session::locations_added (Location *)
+Session::update_skips (Location* loc, bool consolidate)
 {
-	_locations->apply (*this, &Session::sync_locations_to_skips);
+        Locations::LocationList skips;
+
+        if (consolidate) {
+
+                skips = consolidate_skips (loc);
+
+        } else {
+                Locations::LocationList all_locations = _locations->list ();
+                
+                for (Locations::LocationList::iterator l = all_locations.begin(); l != all_locations.end(); ++l) {
+                        if ((*l)->is_skip ()) {
+                                skips.push_back (*l);
+                        }
+                }
+        }
+
+        sync_locations_to_skips (skips);
+}
+
+Locations::LocationList
+Session::consolidate_skips (Location* loc)
+{
+        Locations::LocationList all_locations = _locations->list ();
+        Locations::LocationList skips;
+
+        for (Locations::LocationList::iterator l = all_locations.begin(); l != all_locations.end(); ) {
+
+                if (!(*l)->is_skip ()) {
+                        ++l;
+                        continue;
+                }
+
+                /* don't test against self */
+
+                if (*l == loc) {
+                        ++l;
+                        continue;
+                }
+                        
+                switch (Evoral::coverage ((*l)->start(), (*l)->end(), loc->start(), loc->end())) {
+                case Evoral::OverlapInternal:
+                case Evoral::OverlapExternal:
+                case Evoral::OverlapStart:
+                case Evoral::OverlapEnd:
+                        /* adjust new location to cover existing one */
+                        loc->set_start (min (loc->start(), (*l)->start()));
+                        loc->set_end (max (loc->end(), (*l)->end()));
+                        /* we don't need this one any more */
+                        _locations->remove (*l);
+                        /* the location has been deleted, so remove reference to it in our local list */
+                        l = all_locations.erase (l);
+                        break;
+
+                case Evoral::OverlapNone:
+                        skips.push_back (*l);
+                        ++l;
+                        break;
+                }
+        }
+
+        /* add the new one, which now covers the maximal appropriate range based on overlaps with existing skips */
+
+        skips.push_back (loc);
+
+        return skips;
+}
+
+void
+Session::sync_locations_to_skips (const Locations::LocationList& locations)
+{
+	clear_events (SessionEvent::Skip);
+
+	for (Locations::LocationList::const_iterator i = locations.begin(); i != locations.end(); ++i) {
+                
+		Location* location = *i;
+                
+		if (location->is_skipping()) {
+			SessionEvent* ev = new SessionEvent (SessionEvent::Skip, SessionEvent::Add, location->start(), location->end(), 1.0);
+			queue_event (ev);
+		}
+	}
+}
+
+void
+Session::location_added (Location *location)
+{
+        if (location->is_auto_punch()) {
+                set_auto_punch_location (location);
+        }
+
+        if (location->is_auto_loop()) {
+                set_auto_loop_location (location);
+        }
+        
+        if (location->is_session_range()) {
+                /* no need for any signal handling or event setting with the session range,
+                   because we keep a direct reference to it and use its start/end directly.
+                */
+                _session_range_location = location;
+        }
+
+        if (location->is_skip()) {
+                /* listen for per-location signals that require us to update skip-locate events */
+
+                location->StartChanged.connect_same_thread (skip_connections, boost::bind (&Session::update_skips, this, location, true));
+                location->EndChanged.connect_same_thread (skip_connections, boost::bind (&Session::update_skips, this, location, true));
+                location->Changed.connect_same_thread (skip_connections, boost::bind (&Session::update_skips, this, location, true));
+                location->FlagsChanged.connect_same_thread (skip_connections, boost::bind (&Session::update_skips, this, location, false));
+
+                update_skips (location, true);
+        }
+
+	set_dirty ();
+}
+
+void
+Session::location_removed (Location *location)
+{
+        if (location->is_auto_loop()) {
+                set_auto_loop_location (0);
+        }
+        
+        if (location->is_auto_punch()) {
+                set_auto_punch_location (0);
+        }
+
+        if (location->is_session_range()) {
+                /* this is never supposed to happen */
+                error << _("programming error: session range removed!") << endl;
+        }
+
+        if (location->is_skip()) {
+                
+                update_skips (location, false);
+        }
+
 	set_dirty ();
 }
 
 void
 Session::locations_changed ()
 {
-	_locations->apply (*this, &Session::handle_locations_changed);
+        _locations->apply (*this, &Session::_locations_changed);
 }
 
 void
-Session::handle_locations_changed (Locations::LocationList& locations)
+Session::_locations_changed (const Locations::LocationList& locations)
 {
-	Locations::LocationList::iterator i;
-	Location* location;
-	bool set_loop = false;
-	bool set_punch = false;
+        /* There was some mass-change in the Locations object. 
 
-	for (i = locations.begin(); i != locations.end(); ++i) {
+           We might be re-adding a location here but it doesn't actually matter
+           for all the locations that the Session takes an interest in.
+        */
 
-		location =* i;
-
-		if (location->is_auto_punch()) {
-			set_auto_punch_location (location);
-			set_punch = true;
-		}
-		if (location->is_auto_loop()) {
-			set_auto_loop_location (location);
-			set_loop = true;
-		}
-
-		if (location->is_session_range()) {
-			_session_range_location = location;
-		}
-	}
-
-	sync_locations_to_skips (locations);
-
-	if (!set_loop) {
-		set_auto_loop_location (0);
-	}
-	if (!set_punch) {
-		set_auto_punch_location (0);
-	}
-
-	set_dirty();
-}
-
-void
-Session::sync_locations_to_skips (Locations::LocationList& locations)
-{
-	Locations::LocationList::iterator i;
-	Location* location;
-
-	clear_events (SessionEvent::Skip);
-
-	for (i = locations.begin(); i != locations.end(); ++i) {
-
-		location = *i;
-
-		if (location->is_skipping()) {
-			SessionEvent* ev = new SessionEvent (SessionEvent::Skip, SessionEvent::Add, location->start(), location->end(), 1.0);
-			queue_event (ev);
-		}
-	}
+	for (Locations::LocationList::const_iterator i = locations.begin(); i != locations.end(); ++i) {
+                location_added (*i);
+        }
 }
 
 void
@@ -3719,7 +3811,7 @@ Session::maybe_update_session_range (framepos_t a, framepos_t b)
 
 	if (_session_range_location == 0) {
 
-		add_session_range_location (a, b);
+		set_session_range_location (a, b);
 
 	} else {
 
@@ -4609,9 +4701,9 @@ Session::tempo_map_changed (const PropertyChange&)
 }
 
 void
-Session::update_locations_after_tempo_map_change (Locations::LocationList& loc)
+Session::update_locations_after_tempo_map_change (const Locations::LocationList& loc)
 {
-	for (Locations::LocationList::iterator i = loc.begin(); i != loc.end(); ++i) {
+	for (Locations::LocationList::const_iterator i = loc.begin(); i != loc.end(); ++i) {
 		(*i)->recompute_frames_from_bbt ();
 	}
 }
@@ -5327,10 +5419,18 @@ Session::current_end_frame () const
 }
 
 void
-Session::add_session_range_location (framepos_t start, framepos_t end)
+Session::set_session_range_location (framepos_t start, framepos_t end)
 {
-	_session_range_location = new Location (*this, start, end, _("session"), Location::IsSessionRange);
-	_locations->add (_session_range_location);
+        if (_session_range_location == 0) {
+
+                /* some what weird that we keep our own reference to this location,
+                   but it makes lookup O(1) regardless of the number of markers
+                */
+                _session_range_location = new Location (*this, start, end, _("session"), Location::IsSessionRange);
+                _locations->add (_session_range_location);
+        } else {
+                _session_range_location->set (start, end);
+        }
 }
 
 void

libs/ardour/session_state.cc

@@ -323,10 +323,9 @@ Session::post_engine_init ()
 		
 		initialize_latencies ();
 		
+		_locations->added.connect_same_thread (*this, boost::bind (&Session::location_added, this, _1));
+		_locations->removed.connect_same_thread (*this, boost::bind (&Session::location_removed, this, _1));
 		_locations->changed.connect_same_thread (*this, boost::bind (&Session::locations_changed, this));
-		_locations->added.connect_same_thread (*this, boost::bind (&Session::locations_added, this, _1));
-		
-		
 		
 	} catch (AudioEngine::PortRegistrationFailure& err) {
 		/* handle this one in a different way than all others, so that its clear what happened */
@@ -1262,7 +1261,7 @@ Session::set_state (const XMLNode& node, int version)
 		goto out;
 	}
 
-	locations_changed ();
+        locations_changed ();
 
 	if (_session_range_location) {
 		AudioFileSource::set_header_position_offset (_session_range_location->start());