Skip to content

Add Undo/Redo History #7821

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/JournallingObject.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ class LMMS_EXPORT JournallingObject : public SerializingObject
}
}

void addJournalCheckPoint();
void addJournalCheckPoint(QString reason = "Unknown");

QDomElement saveState( QDomDocument & _doc,
QDomElement & _parent ) override;
Expand Down
29 changes: 20 additions & 9 deletions include/ProjectJournal.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

#include <QHash>
#include <QStack>
#include <QObject>

#include "LmmsTypes.h"
#include "DataFile.h"
Expand All @@ -40,8 +41,9 @@ class JournallingObject;


//! @warning many parts of this class may be rewritten soon
class ProjectJournal
class ProjectJournal : public QObject
{
Q_OBJECT
public:
static const int MAX_UNDO_STATES;

Expand All @@ -54,7 +56,7 @@ class ProjectJournal
bool canUndo() const;
bool canRedo() const;

void addJournalCheckPoint( JournallingObject *jo );
void addJournalCheckPoint(JournallingObject *jo, QString reason);

bool isJournalling() const
{
Expand Down Expand Up @@ -98,22 +100,31 @@ class ProjectJournal
return nullptr;
}


private:
using JoIdMap = QHash<jo_id_t, JournallingObject*>;

struct CheckPoint
{
CheckPoint( jo_id_t initID = 0, const DataFile& initData = DataFile( DataFile::Type::JournalData ) ) :
joID( initID ),
data( initData )
CheckPoint(jo_id_t initID = 0, const DataFile& initData = DataFile(DataFile::Type::JournalData), QString description = "") :
joID(initID),
data(initData),
description(description)
{
}
jo_id_t joID;
DataFile data;
QString description;
} ;
using CheckPointStack = QStack<CheckPoint>;

CheckPointStack& undoCheckPoints() { return m_undoCheckPoints; }
CheckPointStack& redoCheckPoints() { return m_redoCheckPoints; }

signals:
void undoTriggered();
void redoTriggered();
void checkPointAdded();

private:
using JoIdMap = QHash<jo_id_t, JournallingObject*>;

JoIdMap m_joIDs;

CheckPointStack m_undoCheckPoints;
Expand Down
84 changes: 84 additions & 0 deletions include/UndoRedoMenu.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
* UndoRedoMenu.h
*
* Copyright (c) 2004-2022 Tobias Doerffel <tobydox/at/users.sourceforge.net>
*
* This file is part of LMMS - https://lmms.io
*
* 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 (see COPYING); if not, write to the
* Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
* Boston, MA 02110-1301 USA.
*
*/

#ifndef LMMS_GUI_UNDO_REDO_MENU_H
#define LMMS_GUI_UNDO_REDO_MENU_H

#include <QMenu>
#include <QTreeWidget>
#include <QTextEdit>

#include "SideBarWidget.h"
#include "ProjectJournal.h"

namespace lmms::gui
{

class UndoRedoTreeWidget;

class UndoRedoMenu : public SideBarWidget
{
Q_OBJECT
public:
UndoRedoMenu(QWidget *parent = nullptr);
private slots:
void reloadTree();
private:
UndoRedoTreeWidget* m_undoRedoTree;
};

class UndoRedoTreeWidget : public QTreeWidget
{
Q_OBJECT
public:
UndoRedoTreeWidget(QWidget * parent);
~UndoRedoTreeWidget() override = default;
protected:
void contextMenuEvent(QContextMenuEvent * e) override;
private:
void applyUndoRedoEntry(QTreeWidgetItem * item, int column);
};

class UndoRedoTreeWidgetItem : public QTreeWidgetItem
{
public:
UndoRedoTreeWidgetItem(QTreeWidget* parent, ProjectJournal::CheckPoint* cp, int index, bool isUndo);
private:
int m_index;
bool m_isUndo;
ProjectJournal::CheckPoint* m_checkpoint;
friend class UndoRedoTreeWidget;
};

class UndoRedoMenuDetailsWindow : public QTextEdit
{
Q_OBJECT
public:
UndoRedoMenuDetailsWindow(QString text);
};


} // namespace lmms::gui

#endif // LMMS_GUI_UNDO_REDO_MENU_H
7 changes: 3 additions & 4 deletions src/core/AutomatableModel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -296,14 +296,13 @@ void AutomatableModel::setValue( const float value )
{
m_oldValue = m_value;
++m_setValueDepth;
const float old_val = m_value;

m_value = fittedValue( value );
if( old_val != m_value )
if( m_value != value )
{
// add changes to history so user can undo it
addJournalCheckPoint();
addJournalCheckPoint(tr("Set %1 to %2").arg(fullDisplayName(), QString::number(value)));

m_value = fittedValue( value );
// notify linked models
for (const auto& linkedModel : m_linkedModels)
{
Expand Down
6 changes: 3 additions & 3 deletions src/core/JournallingObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,11 @@ JournallingObject::~JournallingObject()



void JournallingObject::addJournalCheckPoint()
void JournallingObject::addJournalCheckPoint(QString reason)
{
if( isJournalling() )
{
Engine::projectJournal()->addJournalCheckPoint( this );
Engine::projectJournal()->addJournalCheckPoint(this, reason);
}
}

Expand Down Expand Up @@ -97,7 +97,7 @@ void JournallingObject::restoreState( const QDomElement & _this )
QDomNode node = _this.firstChild();
while( !node.isNull() )
{
if( node.isElement() && node.nodeName() == "journal" )
if(node.isElement() && node.nodeName() == "journallingObject")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I do not like the feature necessarily, since you fixed a bug I should give actionable feedback. I would try using constexpr auto and storing "journalingObject" in some kind of constant string expression, something like constexpr auto joAttribute = "journalingObject". This would pretty much prevent the bug from happening again. All that's left would be using that attribute where necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can if you want, although to be fair that string is only used twice, both times in JournallingObject.cpp

Copy link
Contributor

@sakertooth sakertooth Apr 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can if you want, although to be fair that string is only used twice, both times in JournallingObject.cpp

This disregards the fact that the code can change at any point. Looking at code as a fixed entity at a fixed point in time is not a good idea.

It's also just good practice. You'd be surprised at how small changes like this can add up and help. You generally do not want to use magic string literals.. case in point, bugs like this happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I have added a constexpr string for the journallingObject node name in the most recent commit.

{
const jo_id_t new_id = node.toElement().attribute( "id" ).toInt();
if( new_id )
Expand Down
14 changes: 10 additions & 4 deletions src/core/ProjectJournal.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

#include <cstdlib>
#include <QDomElement>
#include <QDebug>

#include "ProjectJournal.h"
#include "Engine.h"
Expand Down Expand Up @@ -62,7 +63,7 @@ void ProjectJournal::undo()
{
DataFile curState( DataFile::Type::JournalData );
jo->saveState( curState, curState.content() );
m_redoCheckPoints.push( CheckPoint( c.joID, curState ) );
m_redoCheckPoints.push(CheckPoint(c.joID, curState, c.description));

bool prev = isJournalling();
setJournalling( false );
Expand All @@ -75,8 +76,10 @@ void ProjectJournal::undo()
{
AutomationClip::resolveAllIDs();
}
emit undoTriggered();
break;
}
qWarning() << "Journalling object id" << c.joID << "for" << c.description << "is not valid! This may point to an unresolved issue somewhere in the undo system.";
}
}

Expand All @@ -93,15 +96,17 @@ void ProjectJournal::redo()
{
DataFile curState( DataFile::Type::JournalData );
jo->saveState( curState, curState.content() );
m_undoCheckPoints.push( CheckPoint( c.joID, curState ) );
m_undoCheckPoints.push(CheckPoint(c.joID, curState, c.description));

bool prev = isJournalling();
setJournalling( false );
jo->restoreState( c.data.content().firstChildElement() );
setJournalling( prev );
Engine::getSong()->setModified();
emit redoTriggered();
break;
}
qWarning() << "Journalling object id" << c.joID << "for" << c.description << "is not valid! This may point to an unresolved issue somewhere in the undo system.";
}
}

Expand All @@ -117,7 +122,7 @@ bool ProjectJournal::canRedo() const



void ProjectJournal::addJournalCheckPoint( JournallingObject *jo )
void ProjectJournal::addJournalCheckPoint(JournallingObject *jo, QString reason)
{
if( isJournalling() )
{
Expand All @@ -126,11 +131,12 @@ void ProjectJournal::addJournalCheckPoint( JournallingObject *jo )
DataFile dataFile( DataFile::Type::JournalData );
jo->saveState( dataFile, dataFile.content() );

m_undoCheckPoints.push( CheckPoint( jo->id(), dataFile ) );
m_undoCheckPoints.push(CheckPoint(jo->id(), dataFile, reason));
if( m_undoCheckPoints.size() > MAX_UNDO_STATES )
{
m_undoCheckPoints.remove( 0, m_undoCheckPoints.size() - MAX_UNDO_STATES );
}
emit checkPointAdded();
}
}

Expand Down
1 change: 1 addition & 0 deletions src/gui/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@ SET(LMMS_SRCS
gui/menus/MidiPortMenu.cpp
gui/menus/RecentProjectsMenu.cpp
gui/menus/TemplatesMenu.cpp
gui/menus/UndoRedoMenu.cpp

gui/modals/AboutDialog.cpp
gui/modals/ColorChooser.cpp
Expand Down
4 changes: 4 additions & 0 deletions src/gui/MainWindow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
#include "TimeLineWidget.h"
#include "ToolButton.h"
#include "ToolPlugin.h"
#include "UndoRedoMenu.h"
#include "VersionedSaveDialog.h"

#include "lmmsversion.h"
Expand Down Expand Up @@ -159,6 +160,9 @@ MainWindow::MainWindow() :
sideBar->appendTab(new FileBrowser(root_paths.join("*"), FileItem::defaultFilters(), title,
embed::getIconPixmap("computer").transformed(QTransform().rotate(90)), splitter, dirs_as_items));

emit initProgress(tr("Preparing undo/redo menu"));
sideBar->appendTab(new UndoRedoMenu(splitter));

m_workspace = new MovableQMdiArea(splitter);

// Load background
Expand Down
16 changes: 9 additions & 7 deletions src/gui/editors/PianoRoll.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1752,19 +1752,21 @@ void PianoRoll::mousePressEvent(QMouseEvent * me )
if (it == notes.rend())
{
is_new_note = true;
m_midiClip->addJournalCheckPoint();

// then set new note

// clear selection and select this new note
clearSelectedNotes();

// +32 to quanitize the note correctly when placing notes with
// the mouse. We do this here instead of in note.quantized
// because live notes should still be quantized at the half.
TimePos note_pos( pos_ticks - ( quantization() / 2 ) );
TimePos note_len( newNoteLen() );

m_midiClip->addJournalCheckPoint(tr("Add note"));

// then set new note

// clear selection and select this new note
clearSelectedNotes();


Note new_note( note_len, note_pos, key_num );
new_note.setSelected( true );
new_note.setPanning( m_lastNotePanning );
Expand Down Expand Up @@ -1846,7 +1848,7 @@ void PianoRoll::mousePressEvent(QMouseEvent * me )
m_currentNote->endPos() * m_ppb / TimePos::ticksPerBar() - RESIZE_AREA_WIDTH
&& m_currentNote->length() > 0 )
{
m_midiClip->addJournalCheckPoint();
m_midiClip->addJournalCheckPoint(tr("Resize note"));
// then resize the note
m_action = Action::ResizeNote;

Expand Down
Loading
Loading