Batch undo

include/ClipView.h

@@ -199,8 +199,7 @@ protected slots:
 		Resize,
 		ResizeLeft,
 		Split,
-		CopySelection,
-		ToggleSelected
+		CopyOrToggleSelect, // depends on drag vs click
 	} ;
 
 	static TextFloat * s_textFloat;

include/ProjectJournal.h

@@ -56,6 +56,18 @@ class ProjectJournal
 
 	void addJournalCheckPoint( JournallingObject *jo );
 
+	//! Used similarly to std::lock_guard, see beginBatchAction()
+	class BatchActionGuard
+	{
+	public:
+		BatchActionGuard(ProjectJournal* j) : m_journal(j) {}
+		~BatchActionGuard() { m_journal->batchGuardDestroyed(); }
+	private:
+		ProjectJournal* m_journal;
+	};
+
+	BatchActionGuard beginBatchAction(bool append = false);
+
 	bool isJournalling() const
 	{
 		return m_journalling;
@@ -112,13 +124,19 @@ class ProjectJournal
 		jo_id_t joID;
 		DataFile data;
 	} ;
-	using CheckPointStack = QStack<CheckPoint>;
+	using CheckPointBatch = std::vector<CheckPoint>;
+	using CheckPointStack = std::vector<CheckPointBatch>;
+
+	bool isBatching() { return m_batchGuardCount > 0; }
+	void batchGuardDestroyed();
+	void restoreCheckPoint(ProjectJournal::CheckPointStack& restore, ProjectJournal::CheckPointStack& backup);
 
 	JoIdMap m_joIDs;
 
 	CheckPointStack m_undoCheckPoints;
 	CheckPointStack m_redoCheckPoints;
 
+	int m_batchGuardCount = 0;
 	bool m_journalling;
 
 } ;

src/core/AutomatableModel.cpp

@@ -296,14 +296,15 @@ void AutomatableModel::setValue( const float value )
 {
 	m_oldValue = m_value;
 	++m_setValueDepth;
-	const float old_val = m_value;
+	float newValue = fittedValue(value);
 
-	m_value = fittedValue( value );
-	if( old_val != m_value )
+	if (m_oldValue != newValue)
 	{
 		// add changes to history so user can undo it
 		addJournalCheckPoint();
 
+		m_value = newValue;
+
 		// notify linked models
 		for( AutoModelVector::Iterator it = m_linkedModels.begin(); it != m_linkedModels.end(); ++it )
 		{

src/core/AutomationClip.cpp

@@ -842,6 +842,10 @@ void AutomationClip::loadSettings( const QDomElement & _this )
 		useCustomClipColor( true );
 		setColor( _this.attribute( "color" ) );
 	}
+	else
+	{
+		useCustomClipColor(false);
+	}
 
 	int len = _this.attribute( "len" ).toInt();
 	if( len <= 0 )

src/core/Clip.cpp

@@ -60,6 +60,8 @@ Clip::Clip( Track * track ) :
 	movePosition( 0 );
 	changeLength( 0 );
 	setJournalling( true );
+
+	connect(&m_mutedModel, &Model::dataChanged, this, &Model::dataChanged);
 }
 
 

src/core/JournallingObject.cpp

@@ -73,7 +73,7 @@ QDomElement JournallingObject::saveState( QDomDocument & _doc,
 	{
 		QDomElement _this = SerializingObject::saveState( _doc, _parent );
 
-		QDomElement journalNode = _doc.createElement( "journallingObject" );
+		QDomElement journalNode = _doc.createElement("journal");
 		journalNode.setAttribute( "id", id() );
 		journalNode.setAttribute( "metadata", true );
 		_this.appendChild( journalNode );

src/core/ProjectJournal.cpp

@@ -24,6 +24,7 @@
 
 #include <cstdlib>
 
+#include "debug.h"
 #include "ProjectJournal.h"
 #include "Engine.h"
 #include "JournallingObject.h"
@@ -51,84 +52,151 @@ ProjectJournal::ProjectJournal() :
 
 void ProjectJournal::undo()
 {
-	while( !m_undoCheckPoints.isEmpty() )
-	{
-		CheckPoint c = m_undoCheckPoints.pop();
-		JournallingObject *jo = m_joIDs[c.joID];
+	restoreCheckPoint(m_undoCheckPoints, m_redoCheckPoints);
+}
 
-		if( jo )
-		{
-			DataFile curState( DataFile::JournalData );
-			jo->saveState( curState, curState.content() );
-			m_redoCheckPoints.push( CheckPoint( c.joID, curState ) );
 
-			bool prev = isJournalling();
-			setJournalling( false );
-			jo->restoreState( c.data.content().firstChildElement() );
-			setJournalling( prev );
-			Engine::getSong()->setModified();
-			break;
-		}
-	}
+
+
+void ProjectJournal::redo()
+{
+	restoreCheckPoint(m_redoCheckPoints, m_undoCheckPoints);
 }
 
 
 
-void ProjectJournal::redo()
+
+/*! \brief Take a backup of the current state before restoring the most recent checkpoint
+ *
+ *  \param restoreStack pop a checkpoint from this stack and restore it
+ *  \param backupStack append a checkpoint of the current state to this stack
+ */
+void ProjectJournal::restoreCheckPoint(ProjectJournal::CheckPointStack& restoreStack,
+										ProjectJournal::CheckPointStack& backupStack)
 {
-	while( !m_redoCheckPoints.isEmpty() )
+	while (!restoreStack.empty())
 	{
-		CheckPoint c = m_redoCheckPoints.pop();
-		JournallingObject *jo = m_joIDs[c.joID];
+		CheckPointBatch backup;
 
-		if( jo )
+		// For every checkpoint (journaled object) in the last batch...
+		for (CheckPoint& restorePoint: restoreStack.back())
 		{
-			DataFile curState( DataFile::JournalData );
+			JournallingObject* jo = journallingObject(restorePoint.joID);
+			// If the object with this ID has been destroyed there's an implementation problem somewhere else
+			if (!jo)
+			{
+				fprintf(stderr, "Cannot undo/redo changes on a deleted object\n");
+				continue;
+			}
+
+			// Create a backup of the object's current state
+			DataFile curState(DataFile::JournalData);
 			jo->saveState( curState, curState.content() );
-			m_undoCheckPoints.push( CheckPoint( c.joID, curState ) );
+			backup.emplace_back(restorePoint.joID, curState);
 
+			// Restore object to its previous state
 			bool prev = isJournalling();
 			setJournalling( false );
-			jo->restoreState( c.data.content().firstChildElement() );
+			jo->restoreState(restorePoint.data.content().firstChildElement());
 			setJournalling( prev );
 			Engine::getSong()->setModified();
-			break;
 		}
+		restoreStack.pop_back();
+		backupStack.push_back(std::move(backup));
+		return;
 	}
 }
 
+
+
+
 bool ProjectJournal::canUndo() const
 {
-	return !m_undoCheckPoints.isEmpty();
+	return !m_undoCheckPoints.empty();
 }
 
+
+
+
 bool ProjectJournal::canRedo() const
 {
-	return !m_redoCheckPoints.isEmpty();
+	return !m_redoCheckPoints.empty();
 }
 
 
 
+
 void ProjectJournal::addJournalCheckPoint( JournallingObject *jo )
 {
 	if( isJournalling() )
 	{
 		m_redoCheckPoints.clear();
 
+		// If we're not batching checkpoints, begin on a new one
+		if (!isBatching() || m_undoCheckPoints.empty())
+		{
+			m_undoCheckPoints.emplace_back();
+		}
+		CheckPointBatch& batch = m_undoCheckPoints.back();
+
+		// If this object already has a checkpoint in the batch, skip it
+		for (const CheckPoint& checkpoint: batch)
+		{
+			if (checkpoint.joID == jo->id()) { return; }
+		}
+
+		// Create a checkpoint and save it to the batch
 		DataFile dataFile( DataFile::JournalData );
 		jo->saveState( dataFile, dataFile.content() );
+		batch.emplace_back(jo->id(), dataFile);
 
-		m_undoCheckPoints.push( CheckPoint( jo->id(), dataFile ) );
+		// Remove excessive checkpoints from the stack
 		if( m_undoCheckPoints.size() > MAX_UNDO_STATES )
 		{
-			m_undoCheckPoints.remove( 0, m_undoCheckPoints.size() - MAX_UNDO_STATES );
+			m_undoCheckPoints.erase(m_undoCheckPoints.begin(), m_undoCheckPoints.end() - MAX_UNDO_STATES);
 		}
 	}
 }
 
 
 
 
+/*! \brief Start grouping new checkpoints together in a batch
+ *
+ *  Returns a BatchActionGuard that must be kept for the whole scope of the batch action.
+ *  If batching is already ongoing, checkpoints will be appended to the existing batch.
+ *  Batching ends when all BatchActionGuards goes out of scope or are destroyed.
+ *
+ *  \param append - Don't begin on a new batch, append to the last one instead.
+ *                  This is used as a workaround when we want add more checkpoints to a
+ *                  checkpoint/batch that has already been created.
+ */
+ProjectJournal::BatchActionGuard ProjectJournal::beginBatchAction(bool append)
+{
+	if (!append && !isBatching()) { m_undoCheckPoints.emplace_back(); }
+
+	++m_batchGuardCount;
+
+	return BatchActionGuard(this);
+}
+
+
+
+
+/*! \brief Called by BatchActionGuard's destructor */
+void ProjectJournal::batchGuardDestroyed()
+{
+	--m_batchGuardCount;
+
+	// If the batch ends and no checkpoints were added, remove it from the undo vector
+	if (m_batchGuardCount == 0 && m_undoCheckPoints.back().empty())
+	{
+		m_undoCheckPoints.pop_back();
+	}
+}
+
+
+
 jo_id_t ProjectJournal::allocID( JournallingObject * _obj )
 {
 	jo_id_t id;

src/core/Song.cpp

@@ -779,10 +779,10 @@ void Song::stopExport()
 
 void Song::insertBar()
 {
+	auto batchAction = Engine::projectJournal()->beginBatchAction();
 	m_tracksMutex.lockForRead();
 	for (Track* track: tracks())
 	{
-		// FIXME journal batch of tracks instead of each track individually
 		if (track->numOfClips() > 0) { track->addJournalCheckPoint(); }
 		track->insertBar(m_playPos[Mode_PlaySong]);
 	}
@@ -794,10 +794,10 @@ void Song::insertBar()
 
 void Song::removeBar()
 {
+	auto batchAction = Engine::projectJournal()->beginBatchAction();
 	m_tracksMutex.lockForRead();
 	for (Track* track: tracks())
 	{
-		// FIXME journal batch of tracks instead of each track individually
 		if (track->numOfClips() > 0) { track->addJournalCheckPoint(); }
 		track->removeBar(m_playPos[Mode_PlaySong]);
 	}

src/core/Track.cpp

@@ -39,6 +39,7 @@
 #include "InstrumentTrack.h"
 #include "PatternStore.h"
 #include "PatternTrack.h"
+#include "ProjectJournal.h"
 #include "SampleTrack.h"
 #include "Song.h"
 
@@ -587,19 +588,23 @@ void Track::toggleSolo()
 	const TrackContainer::TrackList & tl = m_trackContainer->tracks();
 
 	bool soloBefore = false;
-	for( TrackContainer::TrackList::const_iterator it = tl.begin();
-							it != tl.end(); ++it )
+	for (Track* track: tl)
 	{
-		if( *it != this )
+		if (track != this && track->m_soloModel.value())
 		{
-			if( ( *it )->m_soloModel.value() )
-			{
-				soloBefore = true;
-				break;
-			}
+			// The toggled solo led will have created a journal checkpoint already (see AutomatableModel::setValue)
+			// Now we are implicitly changing another solo led, so we must save that to the same checkpoint.
+			auto batchAction = Engine::projectJournal()->beginBatchAction(/*append*/ true);
+			track->addJournalCheckPoint();
+			soloBefore = true;
+			break;
 		}
 	}
 
+	// No other changes needs to be journaled since they will happen automatically once solo is toggled
+	bool journalingBefore = Engine::projectJournal()->isJournalling();
+	Engine::projectJournal()->setJournalling(false);
+
 	const bool solo = m_soloModel.value();
 	// Should we use the new behavior of solo or the older/legacy one?
 	const bool soloLegacyBehavior = ConfigManager::inst()->value("app", "sololegacybehavior", "0").toInt();
@@ -638,6 +643,7 @@ void Track::toggleSolo()
 			}
 		}
 	}
+	Engine::projectJournal()->setJournalling(journalingBefore);
 }
 
 void Track::setColor(const QColor& c)
@@ -659,4 +665,4 @@ BoolModel *Track::getMutedModel()
 	return &m_mutedModel;
 }
 
-} // namespace lmms
+} // namespace lmms