port changes to ARDOUR::Location and ARDOUR::Locations APIs from Tracks to Ardour.

Fixes deadlocks caused by mutex on Locations list, and clarifies the purposes and uses of the class-level and
object-level change-related signals.

7 files changed, 287 insertions, 109 deletions

diff --git a/libs/ardour/ardour/location.h b/libs/ardour/ardour/location.h
index 754ebb8075..f809dbf280 100644
--- a/libs/ardour/ardour/location.h
+++ b/libs/ardour/ardour/location.h
@@ -53,6 +53,7 @@ class LIBARDOUR_API Location : public SessionHandleRef, public PBD::StatefulDest
 		IsRangeMarker = 0x20,
 		IsSessionRange = 0x40,
 		IsSkip = 0x80,
+		IsSkipping = 0x100, /* skipping is active (or not) */
 	};
 
 	Location (Session &);
@@ -78,7 +79,7 @@ class LIBARDOUR_API Location : public SessionHandleRef, public PBD::StatefulDest
 	int move_to (framepos_t pos);
 
 	const std::string& name() const { return _name; }
-	void set_name (const std::string &str) { _name = str; name_changed(this); }
+	void set_name (const std::string &str);
 
 	void set_auto_punch (bool yn, void *src);
 	void set_auto_loop (bool yn, void *src);
@@ -86,6 +87,7 @@ class LIBARDOUR_API Location : public SessionHandleRef, public PBD::StatefulDest
 	void set_cd (bool yn, void *src);
 	void set_is_range_marker (bool yn, void* src);
         void set_skip (bool yn);
+        void set_skipping (bool yn);
 
 	bool is_auto_punch () const { return _flags & IsAutoPunch; }
 	bool is_auto_loop () const { return _flags & IsAutoLoop; }
@@ -95,6 +97,7 @@ class LIBARDOUR_API Location : public SessionHandleRef, public PBD::StatefulDest
 	bool is_session_range () const { return _flags & IsSessionRange; }
 	bool is_range_marker() const { return _flags & IsRangeMarker; }
 	bool is_skip() const { return _flags & IsSkip; }
+	bool is_skipping() const { return (_flags & IsSkip) && (_flags & IsSkipping); }
 	bool matches (Flags f) const { return _flags & f; }
 
 	Flags flags () const { return _flags; }
@@ -197,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 */
-	PBD::Signal1<void,Location*> added;
-	PBD::Signal1<void,Location*> removed;
-	PBD::Signal1<void,const PBD::PropertyChange&>    StateChanged;
 
-	template<class T> void apply (T& obj, void (T::*method)(LocationList&)) {
-		Glib::Threads::Mutex::Lock lm (lock);
-		(obj.*method)(locations);
-	}
+        /* 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.
+        */
 
-	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);
+	PBD::Signal1<void,Location*> added;
+	PBD::Signal1<void,Location*> removed;
+	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)(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:
diff --git a/libs/ardour/ardour/midi_scene_changer.h b/libs/ardour/ardour/midi_scene_changer.h
index e2c62a2656..a87ea17da4 100644
--- a/libs/ardour/ardour/midi_scene_changer.h
+++ b/libs/ardour/ardour/midi_scene_changer.h
@@ -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;
 };
diff --git a/libs/ardour/ardour/session.h b/libs/ardour/ardour/session.h
index 2c0ac05bda..61f57a6cc6 100644
--- a/libs/ardour/ardour/session.h
+++ b/libs/ardour/ardour/session.h
@@ -1169,10 +1169,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 *);
@@ -1634,7 +1639,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);
diff --git a/libs/ardour/location.cc b/libs/ardour/location.cc
index df65c5b341..09e5001eea 100644
--- a/libs/ardour/location.cc
+++ b/libs/ardour/location.cc
@@ -161,6 +161,18 @@ Location::operator= (const Location& other)
 	return this;
 }
 
+/** Set location name
+ */
+
+void
+Location::set_name (const std::string& str)
+{ 
+        _name = str; 
+
+        name_changed (this); /* EMIT SIGNAL */
+        NameChanged  (); /* EMIT SIGNAL */
+}
+
 /** Set start position.
  *  @param s New start.
  *  @param force true to force setting, even if the given new start is after the current end.
@@ -282,6 +294,7 @@ Location::set_end (framepos_t e, bool force, bool allow_bbt_recompute)
 		if (allow_bbt_recompute) {
 			recompute_bbt_from_frames ();
 		}
+
 		end_changed(this); /* EMIT SIGNAL */
 		EndChanged(); /* EMIT SIGNAL */
 
@@ -296,22 +309,92 @@ Location::set_end (framepos_t e, bool force, bool allow_bbt_recompute)
 }
 
 int
-Location::set (framepos_t start, framepos_t end, bool allow_bbt_recompute)
+Location::set (framepos_t s, framepos_t e, bool allow_bbt_recompute)
 {
-	if (start < 0 || end < 0) {
+	if (s < 0 || e < 0) {
 		return -1;
 	}
 
 	/* check validity */
-	if (((is_auto_punch() || is_auto_loop()) && start >= end) || (!is_mark() && start > end)) {
+	if (((is_auto_punch() || is_auto_loop()) && s >= e) || (!is_mark() && s > e)) {
 		return -1;
 	}
 
-	/* now we know these values are ok, so force-set them */
-	int const s = set_start (start, true, allow_bbt_recompute);
-	int const e = set_end (end, true, allow_bbt_recompute);
+        bool start_change = false;
+        bool end_change = false;
+
+	if (is_mark()) {
+
+		if (_start != s) {
+			_start = s;
+			_end = s;
+
+			if (allow_bbt_recompute) {
+				recompute_bbt_from_frames ();
+			}
+
+                        start_change = true;
+                        end_change = true;
+		}
+
+		assert (_start >= 0);
+		assert (_end >= 0);
+
+	} else {
+                
+                if (s != _start) {
+
+                        framepos_t const old = _start;
+                        _start = s;
+
+                        if (allow_bbt_recompute) {
+                                recompute_bbt_from_frames ();
+                        }
+
+                        start_change = true;
+                        
+                        if (is_session_range ()) {
+                                Session::StartTimeChanged (old); /* EMIT SIGNAL */
+                                AudioFileSource::set_header_position_offset (s);
+                        }
+                }
+                        
+                 
+                if (e != _end) {
+                        
+                        framepos_t const old = _end;
+                        _end = e;
+
+                        if (allow_bbt_recompute) {
+                                recompute_bbt_from_frames ();
+                        }
+                        
+                        end_change = true;
+
+                        if (is_session_range()) {
+                                Session::EndTimeChanged (old); /* EMIT SIGNAL */
+                        }
+                }
+
+                assert (_end >= 0);
+        }
+
+        if (start_change) {
+                start_changed(this); /* EMIT SIGNAL */
+                StartChanged(); /* EMIT SIGNAL */
+        }
+
+        if (end_change) {
+                end_changed(this); /* EMIT SIGNAL */
+                EndChanged(); /* EMIT SIGNAL */
+        }
 
-	return (s == 0 && e == 0) ? 0 : -1;
+        if (start_change && end_change) {
+                changed (this);
+                Changed ();
+        }
+
+        return 0;
 }
 
 int
@@ -387,6 +470,17 @@ Location::set_skip (bool yn)
 }
 
 void
+Location::set_skipping (bool yn)
+{
+        if (is_range_marker() && is_skip() && length() > 0) {
+                if (set_flag_internal (yn, IsSkipping)) {
+                        flags_changed (this);
+                        FlagsChanged ();
+                }
+        }
+}
+
+void
 Location::set_auto_punch (bool yn, void*)
 {
 	if (is_mark() || _start == _end) {
@@ -655,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 ()
@@ -758,7 +847,7 @@ Locations::clear ()
 		current_location = 0;
 	}
 
-	changed (OTHER); /* EMIT SIGNAL */
+	changed (); /* EMIT SIGNAL */
 	current_changed (0); /* EMIT SIGNAL */
 }
 
@@ -781,8 +870,8 @@ Locations::clear_markers ()
 			i = tmp;
 		}
 	}
-
-	changed (OTHER); /* EMIT SIGNAL */
+        
+	changed (); /* EMIT SIGNAL */
 }
 
 void
@@ -820,7 +909,7 @@ Locations::clear_ranges ()
 		current_location = 0;
 	}
 
-	changed (OTHER); /* EMIT SIGNAL */
+        changed ();
 	current_changed (0); /* EMIT SIGNAL */
 }
 
@@ -881,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 ()
 {
@@ -1005,7 +1086,7 @@ Locations::set_state (const XMLNode& node, int version)
 		}
 	}
 
-	changed (OTHER); /* EMIT SIGNAL */
+	changed (); /* EMIT SIGNAL */
 
 	return 0;
 }
diff --git a/libs/ardour/midi_scene_changer.cc b/libs/ardour/midi_scene_changer.cc
index 88a9e8db35..57fca52e53 100644
--- a/libs/ardour/midi_scene_changer.cc
+++ b/libs/ardour/midi_scene_changer.cc
@@ -42,7 +42,7 @@ MIDISceneChanger::MIDISceneChanger (Session& s)
 	, last_delivered_bank (-1)
 	  
 {
-	_session.locations()->changed.connect_same_thread (*this, boost::bind (&MIDISceneChanger::locations_changed, this, _1));
+	_session.locations()->changed.connect_same_thread (*this, boost::bind (&MIDISceneChanger::locations_changed, this));
 	Location::scene_changed.connect_same_thread (*this, boost::bind (&MIDISceneChanger::gather, this));
 }
 
@@ -51,7 +51,7 @@ MIDISceneChanger::~MIDISceneChanger ()
 }
 
 void
-MIDISceneChanger::locations_changed (Locations::Change)
+MIDISceneChanger::locations_changed ()
 {
 	gather ();
 }
diff --git a/libs/ardour/session.cc b/libs/ardour/session.cc
index 5575f42360..6807bed812 100644
--- a/libs/ardour/session.cc
+++ b/libs/ardour/session.cc
@@ -1235,9 +1235,9 @@ Session::set_auto_punch_location (Location* location)
 
 	punch_connections.drop_connections ();
 
-	location->start_changed.connect_same_thread (punch_connections, boost::bind (&Session::auto_punch_start_changed, this, _1));
-	location->end_changed.connect_same_thread (punch_connections, boost::bind (&Session::auto_punch_end_changed, this, _1));
-	location->changed.connect_same_thread (punch_connections, boost::bind (&Session::auto_punch_changed, this, _1));
+	location->StartChanged.connect_same_thread (punch_connections, boost::bind (&Session::auto_punch_start_changed, this, location));
+	location->EndChanged.connect_same_thread (punch_connections, boost::bind (&Session::auto_punch_end_changed, this, location));
+	location->Changed.connect_same_thread (punch_connections, boost::bind (&Session::auto_punch_changed, this, location));
 
 	location->set_auto_punch (true, this);
 
@@ -1277,9 +1277,9 @@ Session::set_auto_loop_location (Location* location)
 
 	loop_connections.drop_connections ();
 
-	location->start_changed.connect_same_thread (loop_connections, boost::bind (&Session::auto_loop_changed, this, _1));
-	location->end_changed.connect_same_thread (loop_connections, boost::bind (&Session::auto_loop_changed, this, _1));
-	location->changed.connect_same_thread (loop_connections, boost::bind (&Session::auto_loop_changed, this, _1));
+	location->StartChanged.connect_same_thread (loop_connections, boost::bind (&Session::auto_loop_changed, this, location));
+	location->EndChanged.connect_same_thread (loop_connections, boost::bind (&Session::auto_loop_changed, this, location));
+	location->Changed.connect_same_thread (loop_connections, boost::bind (&Session::auto_loop_changed, this, location));
 
 	location->set_auto_loop (true, this);
 
@@ -1293,73 +1293,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);
-	set_dirty ();
+        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);
 }
 
-void
-Session::locations_changed ()
+Locations::LocationList
+Session::consolidate_skips (Location* loc)
 {
-	_locations->apply (*this, &Session::handle_locations_changed);
+        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::handle_locations_changed (Locations::LocationList& locations)
+Session::sync_locations_to_skips (const Locations::LocationList& locations)
 {
-	Locations::LocationList::iterator i;
-	Location* location;
-	bool set_loop = false;
-	bool set_punch = false;
+	clear_events (SessionEvent::Skip);
 
-	for (i = locations.begin(); i != locations.end(); ++i) {
+	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);
+		}
+	}
+}
 
-		location =* i;
+void
+Session::location_added (Location *location)
+{
+        if (location->is_auto_punch()) {
+                set_auto_punch_location (location);
+        }
 
-		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_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_session_range()) {
-			_session_range_location = location;
-		}
-	}
+        if (location->is_skip()) {
+                /* listen for per-location signals that require us to update skip-locate events */
 
-	sync_locations_to_skips (locations);
+                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));
 
-	if (!set_loop) {
-		set_auto_loop_location (0);
-	}
-	if (!set_punch) {
-		set_auto_punch_location (0);
-	}
+                update_skips (location, true);
+        }
 
-	set_dirty();
+	set_dirty ();
 }
 
 void
-Session::sync_locations_to_skips (Locations::LocationList& locations)
+Session::location_removed (Location *location)
 {
-	Locations::LocationList::iterator i;
-	Location* location;
+        if (location->is_auto_loop()) {
+                set_auto_loop_location (0);
+        }
+        
+        if (location->is_auto_punch()) {
+                set_auto_punch_location (0);
+        }
 
-	clear_events (SessionEvent::Skip);
+        if (location->is_session_range()) {
+                /* this is never supposed to happen */
+                error << _("programming error: session range removed!") << endl;
+        }
 
-	for (i = locations.begin(); i != locations.end(); ++i) {
+        if (location->is_skip()) {
+                
+                update_skips (location, false);
+        }
 
-		location = *i;
+	set_dirty ();
+}
 
-		if (location->is_skip()) {
-			SessionEvent* ev = new SessionEvent (SessionEvent::Skip, SessionEvent::Add, location->start(), location->end(), 1.0);
-			queue_event (ev);
-		}
-	}
+void
+Session::locations_changed ()
+{
+        _locations->apply (*this, &Session::_locations_changed);
+}
+
+void
+Session::_locations_changed (const Locations::LocationList& locations)
+{
+        /* There was some mass-change in the Locations object. 
+
+           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.
+        */
+
+	for (Locations::LocationList::const_iterator i = locations.begin(); i != locations.end(); ++i) {
+                location_added (*i);
+        }
 }
 
 void
@@ -4025,9 +4117,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 ();
 	}
 }
diff --git a/libs/ardour/session_state.cc b/libs/ardour/session_state.cc
index 6560c6004b..539facc784 100644
--- a/libs/ardour/session_state.cc
+++ b/libs/ardour/session_state.cc
@@ -316,10 +316,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 */