design changes to various SlavableAutomationControls to make it possible to get the logic right for boolean controls

libs/ardour/ardour/mute_control.h

@@ -64,9 +64,12 @@ class LIBARDOUR_API MuteControl : public SlavableAutomationControl
 	MuteMaster::MutePoint mute_points () const;
 
   protected:
-	void master_changed (bool, PBD::Controllable::GroupControlDisposition);
+	void master_changed (bool, PBD::Controllable::GroupControlDisposition, boost::shared_ptr<AutomationControl>);
 	void actually_set_value (double, PBD::Controllable::GroupControlDisposition group_override);
 
+	void pre_remove_master (boost::shared_ptr<AutomationControl>);
+	void post_add_master (boost::shared_ptr<AutomationControl>);
+
   private:
 	Muteable& _muteable;
 };

libs/ardour/ardour/mute_master.h

@@ -69,8 +69,7 @@ class LIBARDOUR_API MuteMaster : public SessionHandleRef, public PBD::Stateful
 	void set_soloed_by_others (bool yn) { _soloed_by_others = yn; }
 	void set_solo_ignore (bool yn) { _solo_ignore = yn; }
 
-	void mod_muted_by_others (int32_t delta);
-	int32_t muted_by_others () const { return _muted_by_others; }
+	void set_muted_by_others (bool);
 
 	PBD::Signal0<void> MutePointChanged;
 
@@ -84,7 +83,7 @@ class LIBARDOUR_API MuteMaster : public SessionHandleRef, public PBD::Stateful
 	bool      _soloed_by_self;
 	bool      _soloed_by_others;
 	bool      _solo_ignore;
-	int32_t   _muted_by_others;
+	bool      _muted_by_others;
 };
 
 } // namespace ARDOUR

libs/ardour/automation_control.cc

@@ -115,12 +115,14 @@ void
 AutomationControl::actually_set_value (double value, PBD::Controllable::GroupControlDisposition gcd)
 {
 	bool to_list = _list && boost::dynamic_pointer_cast<AutomationList>(_list)->automation_write();
+	const double old_value = Control::user_double ();
 
 	Control::set_double (value, _session.transport_frame(), to_list);
 
 	AutomationType at = (AutomationType) _parameter.type();
 
-	std::cerr << "++++ Changed (" << enum_2_string (at) << ", " << enum_2_string (gcd) << ") = " << value << " @ " << this << std::endl;
+	std::cerr << "++++ Changed (" << enum_2_string (at) << ", " << enum_2_string (gcd) << ") = " << value 
+	          << " (was " << old_value << ") @ " << this << std::endl;
 	Changed (true, gcd);
 }
 

libs/ardour/mute_control.cc

@@ -39,6 +39,40 @@ MuteControl::MuteControl (Session& session, std::string const & name, Muteable&
 	set_flags (Controllable::Flag (flags() | Controllable::RealTime));
 }
 
+void
+MuteControl::post_add_master (boost::shared_ptr<AutomationControl> m)
+{
+	if (m->get_value()) {
+
+		/* boolean masters records are not updated until AFTER
+		 * ::post_add_master() is called, so we can use them to check
+		 * on whether any master was already enabled before the new
+		 * one was added.
+		 */
+
+		if (!muted_by_self() && !get_boolean_masters()) {
+			Changed (false, Controllable::NoGroup);
+		}
+	}
+}
+
+void
+MuteControl::pre_remove_master (boost::shared_ptr<AutomationControl> m)
+{
+	if (!m) {
+		/* null control ptr means we're removing all masters */
+		_muteable.mute_master()->set_muted_by_others (false);
+		/* Changed will be emitted in SlavableAutomationControl::clear_masters() */
+		return;
+	}
+
+	if (m->get_value()) {
+		if (!muted_by_self() && (muted_by_others() == 1)) {
+			Changed (false, Controllable::NoGroup);
+		}
+	}
+}
+
 void
 MuteControl::actually_set_value (double val, Controllable::GroupControlDisposition gcd)
 {
@@ -55,54 +89,34 @@ MuteControl::actually_set_value (double val, Controllable::GroupControlDispositi
 }
 
 void
-MuteControl::master_changed (bool self_change, Controllable::GroupControlDisposition gcd)
+MuteControl::master_changed (bool self_change, Controllable::GroupControlDisposition gcd, boost::shared_ptr<AutomationControl> m)
 {
-	double m = get_masters_value ();
-	const int32_t old_muted_by_others = _muteable.mute_master()->muted_by_others ();
-	std::cerr << "master " << (self_change ? " self " : " not-self") << " changed to " << m << " old others = " << old_muted_by_others << std::endl;
-
-	_muteable.mute_master()->mod_muted_by_others (m ? 1 : -1);
+	bool send_signal = false;
+	const double changed_master_value = m->get_value();
+	boost::shared_ptr<MuteControl> mc = boost::dynamic_pointer_cast<MuteControl> (m);
 
 	if (m) {
-		/* master(s) are now muted. If we are self-muted, this
-		   doesn't change our status. If we are not self-muted,
-		   then it changes our status if either:
+		cerr << "master changed, self ? " << self_change << " self muted = "
+		     << mc->muted_by_self() << " others " << mc->muted_by_others()
+		     << endl;
+	}
 
-		   - the master had its own self-muted status changed OR
-		   - the total number of masters that are muted used to be zero
-		*/
-
-		if (!muted_by_self()) {
-			if (self_change || old_muted_by_others == 0) {
-				/* note false as the first argument - our own
-				   value was not changed
-				*/
-				Changed (false, gcd);
-			} else {
-				cerr << " no Change signal\n";
-			}
-		} else {
-			cerr << "muted by self, not relevant\n";
+	if (changed_master_value) {
+		/* this master is now enabled */
+		if (!muted_by_self() && get_boolean_masters() == 0) {
+			send_signal = true;
 		}
 	} else {
-		/* no master(s) are now muted. If we are self-muted, this
-		   doesn't change our status. If we are not self-muted,
-		   then it changes our status if either:
-
-		   - the master had its own self-muted status changed OR
-		   - the total number of masters that are muted used to be non-zero
-		*/
-
-		if (!muted_by_self()) {
-			if (self_change || old_muted_by_others != 0) {
-				Changed (false, gcd);
-			} else {
-				cerr << " No change signal\n";
-			}
-		} else {
-			cerr << "muted by self, not relevant\n";
+		if (!muted_by_self() && get_boolean_masters() == 1) {
+			send_signal = true;
 		}
 	}
+
+	update_boolean_masters_records (m);
+
+	if (send_signal) {
+		Changed (false, Controllable::NoGroup);
+	}
 }
 
 double
@@ -141,7 +155,7 @@ MuteControl::mute_points () const
 bool
 MuteControl::muted () const
 {
-	return _muteable.mute_master()->muted_by_self() || _muteable.mute_master()->muted_by_others();
+	return muted_by_self() || muted_by_others();
 }
 
 bool
@@ -153,5 +167,5 @@ MuteControl::muted_by_self () const
 bool
 MuteControl::muted_by_others () const
 {
-	return _muteable.mute_master()->muted_by_others ();
+	return get_masters_value ();
 }

libs/ardour/mute_master.cc

@@ -170,8 +170,8 @@ MuteMaster::muted_by_others_at (MutePoint mp) const
 }
 
 void
-MuteMaster::mod_muted_by_others (int32_t delta)
+MuteMaster::set_muted_by_others (bool yn)
 {
-	_muted_by_others = max (0, _muted_by_others + delta);
-	std::cerr << this << " mod others by " << delta << " to get " << _muted_by_others << endl;
+	_muted_by_others = yn;
+	std::cerr << this << " set muted by others to " << yn << std::endl;
 }

libs/ardour/slavable_automation_control.cc

@@ -119,13 +119,11 @@ SlavableAutomationControl::actually_set_value (double val, Controllable::GroupCo
 void
 SlavableAutomationControl::add_master (boost::shared_ptr<AutomationControl> m)
 {
-	double current_value;
-	double new_value;
 	std::pair<Masters::iterator,bool> res;
 
 	{
 		Glib::Threads::RWLock::WriterLock lm (master_lock);
-		current_value = get_value_locked ();
+		const double current_value = get_value_locked ();
 
 		/* ratio will be recomputed below */
 
@@ -133,9 +131,7 @@ SlavableAutomationControl::add_master (boost::shared_ptr<AutomationControl> m)
 
 		if (res.second) {
 
-			if (_desc.toggled) {
-				recompute_masters_ratios (current_value);
-			}
+			recompute_masters_ratios (current_value);
 
 			/* note that we bind @param m as a weak_ptr<AutomationControl>, thus
 			   avoiding holding a reference to the control in the binding
@@ -152,11 +148,9 @@ SlavableAutomationControl::add_master (boost::shared_ptr<AutomationControl> m)
 			   because the change came from the master.
 			*/
 
-			m->Changed.connect_same_thread (res.first->second.connection, boost::bind (&SlavableAutomationControl::master_changed, this, _1, _2));
+			m->Changed.connect_same_thread (res.first->second.connection, boost::bind (&SlavableAutomationControl::master_changed, this, _1, _2, m));
 			cerr << this << enum_2_string ((AutomationType) _parameter.type()) << " now listening to Changed from " << m << endl;
 		}
-
-		new_value = get_value_locked ();
 	}
 
 	if (res.second) {
@@ -164,30 +158,66 @@ SlavableAutomationControl::add_master (boost::shared_ptr<AutomationControl> m)
 		MasterStatusChange (); /* EMIT SIGNAL */
 	}
 
-	if (new_value != current_value) {
-		/* need to do this without a writable() check in case
-		 * the master is removed while this control is doing
-		 * automation playback.
-		 */
-		 actually_set_value (new_value, Controllable::NoGroup);
+	post_add_master (m);
+
+	update_boolean_masters_records (m);
+}
+
+bool
+SlavableAutomationControl::get_boolean_masters () const
+{
+	if (!_desc.toggled) {
+		return false;
 	}
 
+	Glib::Threads::RWLock::ReaderLock lm (master_lock);
+	for (Masters::const_iterator mr = _masters.begin(); mr != _masters.end(); ++mr) {
+		if (mr->second.yn()) {
+			return true;
+		}
+	}
 }
 
 void
-SlavableAutomationControl::master_changed (bool /*from_self*/, GroupControlDisposition gcd)
+SlavableAutomationControl::update_boolean_masters_records (boost::shared_ptr<AutomationControl> m)
 {
-	/* our value has (likely) changed, but not because we were
-	 * modified. Just the master.
-	 */
+	if (_desc.toggled) {
+		/* We may modify a MasterRecord, but we not modify the master
+		 * map, so we use a ReaderLock
+		 */
+		Glib::Threads::RWLock::ReaderLock lm (master_lock);
+		Masters::iterator mi = _masters.find (m->id());
+		if (mi != _masters.end()) {
+			/* update MasterRecord to show whether the master is
+			   on/off. We need to store this because the master
+			   may change (in the sense of emitting Changed())
+			   several times without actually changing the result
+			   of ::get_value(). This is a feature of
+			   AutomationControls (or even just Controllables,
+			   really) which have more than a simple scalar
+			   value. For example, the master may be a mute control
+			   which can be muted_by_self() and/or
+			   muted_by_others(). When either of those two
+			   conditions changes, Changed() will be emitted, even
+			   though ::get_value() will return the same value each
+			   time (1.0 if either are true, 0.0 if neither is).
 
-	/* propagate master state into our own control so that if we stop
-	 * being slaved, our value doesn't change, and propagate to any
-	 * group this control is part of.
-	 */
+			   This provides a way for derived types to check
+			   the last known state of a Master when the Master
+			   changes. We update it after calling
+			   ::master_changed() (though derived types must do
+			   this themselves).
+			*/
+			mi->second.set_yn (m->get_value());
+		}
+	}
+}
 
-	cerr << this << ' ' << enum_2_string ((AutomationType) _parameter.type()) << " pass along " << get_masters_value() << " from master to group\n";
-	actually_set_value (get_masters_value(), Controllable::UseGroup);
+void
+SlavableAutomationControl::master_changed (bool /*from_self*/, GroupControlDisposition gcd, boost::shared_ptr<AutomationControl> m)
+{
+	update_boolean_masters_records (m);
+	Changed (false, Controllable::NoGroup); /* EMIT SIGNAL */
 }
 
 void
@@ -207,6 +237,8 @@ SlavableAutomationControl::remove_master (boost::shared_ptr<AutomationControl> m
 	bool masters_left;
 	Masters::size_type erased = 0;
 
+	pre_remove_master (m);
+
 	{
 		Glib::Threads::RWLock::WriterLock lm (master_lock);
 		current_value = get_value_locked ();
@@ -230,6 +262,10 @@ SlavableAutomationControl::remove_master (boost::shared_ptr<AutomationControl> m
 			actually_set_value (current_value, Controllable::UseGroup);
 		}
 	}
+
+	/* no need to update boolean masters records, since the MR will have
+	 * been removed already.
+	 */
 }
 
 void
@@ -239,6 +275,9 @@ SlavableAutomationControl::clear_masters ()
 	double new_value;
 	bool had_masters = false;
 
+	/* null ptr means "all masters */
+	pre_remove_master (boost::shared_ptr<AutomationControl>());
+
 	{
 		Glib::Threads::RWLock::WriterLock lm (master_lock);
 		current_value = get_value_locked ();
@@ -254,9 +293,12 @@ SlavableAutomationControl::clear_masters ()
 	}
 
 	if (new_value != current_value) {
-		Changed (false, Controllable::NoGroup);
+		actually_set_value (current_value, Controllable::UseGroup);
 	}
 
+	/* no need to update boolean masters records, since all MRs will have
+	 * been removed already.
+	 */
 }
 
 bool

libs/ardour/solo_isolate_control.cc

@@ -42,7 +42,7 @@ SoloIsolateControl::SoloIsolateControl (Session& session, std::string const & na
 }
 
 void
-SoloIsolateControl::master_changed (bool from_self, PBD::Controllable::GroupControlDisposition gcd)
+SoloIsolateControl::master_changed (bool from_self, PBD::Controllable::GroupControlDisposition gcd, boost::shared_ptr<AutomationControl>)
 {
 	if (!_soloable.can_solo()) {
 		return;