fix logic for removing an AutomationControl from a ControlGroup

libs/ardour/ardour/automation_control.h

@@ -33,10 +33,12 @@
 #include "evoral/types.hpp"
 #include "evoral/Control.hpp"
 
-#include "ardour/libardour_visibility.h"
 #include "ardour/automation_list.h"
+#include "ardour/control_group_member.h"
 #include "ardour/parameter_descriptor.h"
 
+#include "ardour/libardour_visibility.h"
+
 namespace ARDOUR {
 
 class Session;
@@ -49,6 +51,7 @@ class LIBARDOUR_API AutomationControl
 	: public PBD::Controllable
 	, public Evoral::Control
 	, public boost::enable_shared_from_this<AutomationControl>
+	, public ControlGroupMember
 {
     public:
 	AutomationControl(ARDOUR::Session&,
@@ -116,8 +119,6 @@ class LIBARDOUR_API AutomationControl
 	const ARDOUR::Session& session() const { return _session; }
 	void commit_transaction (bool did_write);
 
-	void set_group (boost::shared_ptr<ControlGroup>);
-
   protected:
 	ARDOUR::Session& _session;
 	boost::shared_ptr<ControlGroup> _group;
@@ -132,6 +133,14 @@ class LIBARDOUR_API AutomationControl
 	*/
 
 	virtual void actually_set_value (double value, PBD::Controllable::GroupControlDisposition);
+
+  private:
+	/* I am unclear on why we have to make ControlGroup a friend in order
+	   to get access to the ::set_group() method when it is already
+	   declared to be a friend in ControlGroupMember. Oh well.
+	*/
+	friend class ControlGroup;
+	void set_group (boost::shared_ptr<ControlGroup>);
 };
 
 class SlavableAutomationControl : public AutomationControl

libs/ardour/ardour/control_group_member.h

@@ -0,0 +1,40 @@
+/*
+  Copyright (C) 2016 Paul Davis
+
+  This program is free software; you can redistribute it and/or modify it
+  under the terms of the GNU General Public License as published by the Free
+  Software Foundation; either version 2 of the License, or (at your option)
+  any later version.
+
+  This program is distributed in the hope that it will be useful, but WITHOUT
+  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+  FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+  for more details.
+
+  You should have received a copy of the GNU General Public License along
+  with this program; if not, write to the Free Software Foundation, Inc.,
+  675 Mass Ave, Cambridge, MA 02139, USA.
+*/
+
+#ifndef __libardour_control_group_member_h__
+#define __libardour_control_group_member_h__
+
+namespace ARDOUR {
+
+class ControlGroup;
+
+class LIBARDOUR_API ControlGroupMember
+{
+  public:
+	virtual ~ControlGroupMember () {};
+  private:
+	friend class ControlGroup;
+	/* Only a ControlGroup can call this; all membership changes must be
+	   mediated by the ControlGroup, not by operating on the member.
+	*/
+	virtual void set_group (boost::shared_ptr<ControlGroup>) = 0;
+};
+
+} /* namespace */
+
+#endif /* __libardour_control_group_member_h__ */

libs/ardour/automation_control.cc

@@ -268,9 +268,11 @@ AutomationControl::interface_to_internal (double val) const
 void
 AutomationControl::set_group (boost::shared_ptr<ControlGroup> cg)
 {
-	if (_group) {
-		_group->remove_control (shared_from_this());
-	}
+	/* this method can only be called by a ControlGroup. We do not need
+	   to ensure consistency by calling ControlGroup::remove_control(),
+	   since we are guaranteed that the ControlGroup will take care of that
+	   for us.
+	*/
 
 	_group = cg;
 }

libs/ardour/control_group.cc

@@ -104,9 +104,19 @@ ControlGroup::control_going_away (boost::weak_ptr<AutomationControl> wac)
 int
 ControlGroup::remove_control (boost::shared_ptr<AutomationControl> ac)
 {
-	Glib::Threads::RWLock::WriterLock lm (controls_lock);
+	int erased;
+
+	{
+		Glib::Threads::RWLock::WriterLock lm (controls_lock);
+		erased = _controls.erase (ac->id());
+	}
+
+	if (erased) {
+		ac->set_group (boost::shared_ptr<ControlGroup>());
+	}
+
 	/* return zero if erased, non-zero otherwise */
-	return !(_controls.erase (ac->id()) > 0);
+	return !erased;
 }
 
 int