From 88eeebcb8b89ccc2ff30ad6a501e875c17bdac72 Mon Sep 17 00:00:00 2001 From: Paul Licameli Date: Thu, 11 Feb 2016 11:03:12 -0500 Subject: [PATCH] Undoing and redoing of Tags... Use reference counting (in std::shared_ptr) to avoid replicating Tags with each Undo state. --- src/Menus.cpp | 20 ++++++++++-- src/Menus.h | 1 + src/Project.cpp | 65 +++++++++++++++++++++++++++++-------- src/Project.h | 7 ++-- src/Tags.cpp | 5 +++ src/Tags.h | 2 ++ src/UndoManager.cpp | 20 +++++++++--- src/UndoManager.h | 11 +++++-- src/export/Export.cpp | 2 +- src/export/ExportFFmpeg.cpp | 3 +- 10 files changed, 109 insertions(+), 27 deletions(-) diff --git a/src/Menus.cpp b/src/Menus.cpp index 9ffae1371..cddf17466 100644 --- a/src/Menus.cpp +++ b/src/Menus.cpp @@ -5582,8 +5582,24 @@ void AudacityProject::OnImportRaw() void AudacityProject::OnEditMetadata() { - if (mTags->ShowEditDialog(this, _("Edit Metadata Tags"), true)) - PushState(_("Edit Metadata Tags"), _("Metadata Tags")); + (void)DoEditMetadata(_("Edit Metadata Tags"), _("Metadata Tags"), true); +} + +bool AudacityProject::DoEditMetadata +(const wxString &title, const wxString &shortUndoDescription, bool force) +{ + // Back up my tags + auto newTags = mTags->Duplicate(); + + if (newTags->ShowEditDialog(this, title, force)) { + // Commit the change to project state only now. + mTags = newTags; + PushState(title, shortUndoDescription); + + return true; + } + + return false; } void AudacityProject::HandleMixAndRender(bool toNewTrack) diff --git a/src/Menus.h b/src/Menus.h index 179d61a9e..f6ead01bd 100644 --- a/src/Menus.h +++ b/src/Menus.h @@ -320,6 +320,7 @@ void OnImportMIDI(); void OnImportRaw(); void OnEditMetadata(); +bool DoEditMetadata(const wxString &title, const wxString &shortUndoDescription, bool force); void OnMixAndRender(); void OnMixAndRenderToNewTrack(); diff --git a/src/Project.cpp b/src/Project.cpp index cb40f365f..b44ddcddf 100644 --- a/src/Project.cpp +++ b/src/Project.cpp @@ -1026,6 +1026,9 @@ AudacityProject::AudacityProject(wxWindow * parent, wxWindowID id, // MM: Give track panel the focus to ensure keyboard commands work mTrackPanel->SetFocus(); + // Create tags object + mTags = std::make_shared(); + InitialState(); FixScrollbars(); mRuler->SetLeftOffset(mTrackPanel->GetLeftOffset()); // bevel on AdornedRuler @@ -1051,9 +1054,6 @@ AudacityProject::AudacityProject(wxWindow * parent, wxWindowID id, #endif mIconized = false; - // Create tags object - mTags = new Tags(); - mTrackFactory = new TrackFactory(mDirManager, &mViewInfo); int widths[] = {0, GetControlToolBar()->WidthForStatusBar(mStatusBar), -1, 150}; @@ -1250,9 +1250,9 @@ bool AudacityProject::IsAudioActive() const gAudioIO->IsStreamActive(GetAudioIOToken()); } -Tags *AudacityProject::GetTags() +const Tags *AudacityProject::GetTags() { - return mTags; + return mTags.get(); } wxString AudacityProject::GetName() @@ -2320,8 +2320,7 @@ void AudacityProject::OnCloseWindow(wxCloseEvent & event) delete mTrackFactory; mTrackFactory = NULL; - delete mTags; - mTags = NULL; + mTags.reset(); delete mImportXMLTagHandler; mImportXMLTagHandler = NULL; @@ -3182,7 +3181,7 @@ bool AudacityProject::HandleXMLTag(const wxChar *tag, const wxChar **attrs) XMLTagHandler *AudacityProject::HandleXMLChild(const wxChar *tag) { if (!wxStrcmp(tag, wxT("tags"))) { - return mTags; + return mTags.get(); } if (!wxStrcmp(tag, wxT("wavetrack"))) { @@ -3793,12 +3792,40 @@ bool AudacityProject::Import(const wxString &fileName, WaveTrackArray* pTrackArr { Track **newTracks; int numTracks; - wxString errorMessage=wxT(""); + wxString errorMessage = wxEmptyString; + + // Backup Tags, before the import. Be prepared to roll back changes. + struct TempTags { + TempTags(std::shared_ptr & pTags_) + : pTags(pTags_) + { + oldTags = pTags; + if (oldTags) + pTags = oldTags->Duplicate(); + } + + ~TempTags() + { + if (oldTags) { + // roll back + pTags = oldTags; + } + } + + void Commit() + { + oldTags.reset(); + } + + std::shared_ptr & pTags; + std::shared_ptr oldTags; + }; + TempTags tempTags(mTags); numTracks = Importer::Get().Import(fileName, mTrackFactory, &newTracks, - mTags, + mTags.get(), errorMessage); if (!errorMessage.IsEmpty()) { @@ -3814,9 +3841,17 @@ bool AudacityProject::Import(const wxString &fileName, WaveTrackArray* pTrackArr wxGetApp().AddFileToHistory(fileName); + // no more errors + tempTags.Commit(); + // for LOF ("list of files") files, do not import the file as if it // were an audio file itself if (fileName.AfterLast('.').IsSameAs(wxT("lof"), false)) { + // PRL: don't redundantly do the steps below, because we already + // did it in case of LOF, because of some weird recursion back to this + // same function. I think this should be untangled. + + // So Undo history push is not bypassed, despite appearances. return false; } @@ -3830,6 +3865,7 @@ bool AudacityProject::Import(const wxString &fileName, WaveTrackArray* pTrackArr } } + // PRL: Undo history is incremented inside this: AddImportedTracks(fileName, newTracks, numTracks); int mode = gPrefs->Read(wxT("/AudioFiles/NormalizeOnLoad"), 0L); @@ -3984,7 +4020,7 @@ void AudacityProject::InitialState() GetUndoManager()->ClearStates(); - GetUndoManager()->PushState(mTracks, mViewInfo.selectedRegion, + GetUndoManager()->PushState(mTracks, mViewInfo.selectedRegion, mTags, _("Created new project"), wxT("")); GetUndoManager()->StateSaved(); @@ -4008,7 +4044,7 @@ void AudacityProject::PushState(const wxString &desc, const wxString &shortDesc, UndoPush flags ) { - GetUndoManager()->PushState(mTracks, mViewInfo.selectedRegion, + GetUndoManager()->PushState(mTracks, mViewInfo.selectedRegion, mTags, desc, shortDesc, flags); mDirty = true; @@ -4044,7 +4080,7 @@ void AudacityProject::RollbackState() void AudacityProject::ModifyState(bool bWantsAutoSave) { - GetUndoManager()->ModifyState(mTracks, mViewInfo.selectedRegion); + GetUndoManager()->ModifyState(mTracks, mViewInfo.selectedRegion, mTags); if (bWantsAutoSave) AutoSave(); } @@ -4054,6 +4090,9 @@ void AudacityProject::ModifyState(bool bWantsAutoSave) // Need to keep it and its tracks "t" available for Undo/Redo/SetStateTo. void AudacityProject::PopState(const UndoState &state) { + // Restore tags + mTags = state.tags; + TrackList *const tracks = state.tracks.get(); mTracks->Clear(true); diff --git a/src/Project.h b/src/Project.h index 3dae3b8b4..824a83234 100644 --- a/src/Project.h +++ b/src/Project.h @@ -185,7 +185,7 @@ class AUDACITY_DLL_API AudacityProject: public wxFrame, DirManager *GetDirManager(); TrackFactory *GetTrackFactory(); AdornedRulerPanel *GetRulerPanel(); - Tags *GetTags(); + const Tags *GetTags(); int GetAudioIOToken() const; bool IsAudioActive() const; void SetAudioIOToken(int token); @@ -526,7 +526,10 @@ public: wxMenu *mRecentFilesMenu; // Tags (artist name, song properties, MP3 ID3 info, etc.) - Tags *mTags; + // The structure may be shared with undo history entries + // To keep undo working correctly, always replace this with a new duplicate + // BEFORE doing any editing of it! + std::shared_ptr mTags; // List of tracks and display info TrackList *mTracks; diff --git a/src/Tags.cpp b/src/Tags.cpp index 372c51769..62ecc0a12 100644 --- a/src/Tags.cpp +++ b/src/Tags.cpp @@ -233,6 +233,11 @@ Tags::~Tags() { } +std::shared_ptr Tags::Duplicate() const +{ + return std::make_shared(*this); +} + Tags & Tags::operator=(const Tags & src) { mEditTitle = src.mEditTitle; diff --git a/src/Tags.h b/src/Tags.h index bb7521702..c666c75bd 100644 --- a/src/Tags.h +++ b/src/Tags.h @@ -77,6 +77,8 @@ class AUDACITY_DLL_API Tags: public XMLTagHandler { Tags(); // constructor virtual ~Tags(); + std::shared_ptr Duplicate() const; + Tags & operator= (const Tags & src ); bool ShowEditDialog(wxWindow *parent, const wxString &title, bool force = false); diff --git a/src/UndoManager.cpp b/src/UndoManager.cpp index 969e8033c..7f9f205c5 100644 --- a/src/UndoManager.cpp +++ b/src/UndoManager.cpp @@ -31,6 +31,7 @@ UndoManager #include "WaveTrack.h" // temp #include "NoteTrack.h" // for Sonify* function declarations #include "Diags.h" +#include "Tags.h" #include "UndoManager.h" @@ -41,8 +42,9 @@ struct UndoStackElem { UndoStackElem(std::unique_ptr &&tracks_, const wxString &description_, const wxString &shortDescription_, - const SelectedRegion &selectedRegion_) - : state(std::move(tracks_), selectedRegion_) + const SelectedRegion &selectedRegion_, + const std::shared_ptr &tags_) + : state(std::move(tracks_), tags_, selectedRegion_) , description(description_) , shortDescription(shortDescription_) { @@ -196,7 +198,8 @@ bool UndoManager::RedoAvailable() } void UndoManager::ModifyState(const TrackList * l, - const SelectedRegion &selectedRegion) + const SelectedRegion &selectedRegion, + const std::shared_ptr &tags) { if (current == wxNOT_FOUND) { return; @@ -217,12 +220,15 @@ void UndoManager::ModifyState(const TrackList * l, // Replace stack[current]->state.tracks = std::move(tracksCopy); + stack[current]->state.tags = tags; + stack[current]->state.selectedRegion = selectedRegion; SonifyEndModifyState(); } void UndoManager::PushState(const TrackList * l, const SelectedRegion &selectedRegion, + const std::shared_ptr &tags, const wxString &longDescription, const wxString &shortDescription, UndoPush flags) @@ -233,7 +239,7 @@ void UndoManager::PushState(const TrackList * l, if (((flags & UndoPush::CONSOLIDATE) != UndoPush::MINIMAL) && lastAction == longDescription && consolidationCount < 2) { consolidationCount++; - ModifyState(l, selectedRegion); + ModifyState(l, selectedRegion, tags); // MB: If the "saved" state was modified by ModifyState, reset // it so that UnsavedChanges returns true. if (current == saved) { @@ -257,10 +263,14 @@ void UndoManager::PushState(const TrackList * l, t = iter.Next(); } + // Assume tags was duplicted before any changes. + // Just save a new shared_ptr to it. stack.emplace_back( std::make_unique - (std::move(tracksCopy), longDescription, shortDescription, selectedRegion) + (std::move(tracksCopy), + longDescription, shortDescription, selectedRegion, tags) ); + current++; if (saved >= current) { diff --git a/src/UndoManager.h b/src/UndoManager.h index 9101a49d7..40f92acde 100644 --- a/src/UndoManager.h +++ b/src/UndoManager.h @@ -54,16 +54,20 @@ #include "ondemand/ODTaskThread.h" #include "SelectedRegion.h" +class Tags; class Track; class TrackList; struct UndoStackElem; struct UndoState { - UndoState(std::unique_ptr &&tracks_, const SelectedRegion &selectedRegion_) - : tracks(std::move(tracks_)), selectedRegion(selectedRegion_) + UndoState(std::unique_ptr &&tracks_, + const std::shared_ptr &tags_, + const SelectedRegion &selectedRegion_) + : tracks(std::move(tracks_)), tags(tags_), selectedRegion(selectedRegion_) {} std::unique_ptr tracks; + std::shared_ptr tags; SelectedRegion selectedRegion; // by value }; @@ -91,10 +95,11 @@ class AUDACITY_DLL_API UndoManager { void PushState(const TrackList * l, const SelectedRegion &selectedRegion, + const std::shared_ptr &tags, const wxString &longDescription, const wxString &shortDescription, UndoPush flags = UndoPush::AUTOSAVE); void ModifyState(const TrackList * l, - const SelectedRegion &selectedRegion); + const SelectedRegion &selectedRegion, const std::shared_ptr &tags); void ClearStates(); void RemoveStates(int num); // removes the 'num' oldest states void RemoveStateAt(int n); // removes the n'th state (1 is oldest) diff --git a/src/export/Export.cpp b/src/export/Export.cpp index b19ea8b16..e5e6ac06a 100644 --- a/src/export/Export.cpp +++ b/src/export/Export.cpp @@ -364,7 +364,7 @@ bool Exporter::Process(AudacityProject *project, bool selectedOnly, double t0, d // Let user edit MetaData if (mPlugins[mFormat]->GetCanMetaData(mSubFormat)) { - if (!(project->GetTags()->ShowEditDialog(project, _("Edit Metadata Tags"), mProject->GetShowId3Dialog()))) { + if (!(project->DoEditMetadata(_("Edit Metadata Tags for Export"), _("Exported Tags"), mProject->GetShowId3Dialog()))) { return false; } } diff --git a/src/export/ExportFFmpeg.cpp b/src/export/ExportFFmpeg.cpp index b74a77f13..f5aa266ad 100644 --- a/src/export/ExportFFmpeg.cpp +++ b/src/export/ExportFFmpeg.cpp @@ -312,7 +312,8 @@ bool ExportFFmpeg::Init(const char *shortname, AudacityProject *project, const T if (!InitCodecs(project)) return false; - if (metadata == NULL) metadata = project->GetTags(); + if (metadata == NULL) + metadata = project->GetTags(); // Add metadata BEFORE writing the header. // At the moment that works with ffmpeg-git and ffmpeg-0.5 for MP4.