From 8187a2e3a7f8182de400b557b733d007bf93a078 Mon Sep 17 00:00:00 2001 From: IanCaio Date: Sun, 4 Oct 2020 11:07:44 -0300 Subject: [PATCH 01/13] Initial Commit This branch aims to create a feature for merging Instrument Track's patterns in a single pattern. Current changes: - Adds a getPattern method to PatternView. - Adds a context menu action called "Merge". - Inside TrackContentObjectView::contextMenuEvent, we check if the selected TCOs can be merged and store the answer in a boolean called canMergeTCOs. The conditions to merge TCOs are: Having multiple TCOs selected; All TCOs belonging to a InstrumentTrack; All TCOs belonging to the same track. - If canMergeTCOs is true, add an action on the context menu for merging the selection. - Creates a method called TrackContentObjectView::mergeTCOs( tcovs ). That method first finds the earliest MidiTime from all selected TCOs and creates a pattern on that position. Then, notes are added to that pattern and repositioned depending on the relative position from the current TCOV being merged in relation to the new pattern. Some warnings were added to this method in case dynamic_casts failed, though they shouldn't. After all notes being added and positioned, newPattern->updateLength() and newPattern->dataChanged() are called. Known Bugs: - Some added notes don't play until you make a change (i.e.: move them) in the PianoRoll. To Do: - Remove QDebug include on Track.cpp. - Remove some debugging warnings. - Change the PixMap for the Merge Selection action on the context menu. - Fix bugs. --- include/Pattern.h | 1 + include/Track.h | 4 +- src/core/Track.cpp | 121 ++++++++++++++++++++++++++++++++++++++++- src/tracks/Pattern.cpp | 11 ++++ 4 files changed, 135 insertions(+), 2 deletions(-) diff --git a/include/Pattern.h b/include/Pattern.h index 5192da9faf8..1699f5d53f4 100644 --- a/include/Pattern.h +++ b/include/Pattern.h @@ -182,6 +182,7 @@ class PatternView : public TrackContentObjectView void setMutedNoteBorderColor(QColor const & color) { m_mutedNoteBorderColor = color; } public slots: + Pattern * getPattern(); void update() override; diff --git a/include/Track.h b/include/Track.h index 9362a838019..49adc48084d 100644 --- a/include/Track.h +++ b/include/Track.h @@ -263,6 +263,7 @@ class TrackContentObjectView : public selectableObject, public ModelView // some metadata to be written to the clipboard. static void remove( QVector tcovs ); static void toggleMute( QVector tcovs ); + static void mergeTCOs( QVector tcovs ); public slots: virtual bool close(); @@ -277,7 +278,8 @@ public slots: Cut, Copy, Paste, - Mute + Mute, + Merge }; virtual void constructContextMenu( QMenu * ) diff --git a/src/core/Track.cpp b/src/core/Track.cpp index c7f6d62b0b5..32c548f5a50 100644 --- a/src/core/Track.cpp +++ b/src/core/Track.cpp @@ -46,6 +46,7 @@ #include #include #include +#include #include "AutomationPattern.h" @@ -69,6 +70,7 @@ #include "SongEditor.h" #include "StringPairDrag.h" #include "TextFloat.h" +#include "Pattern.h" /*! The width of the resize grip in pixels @@ -1123,9 +1125,11 @@ void TrackContentObjectView::mouseReleaseEvent( QMouseEvent * me ) */ void TrackContentObjectView::contextMenuEvent( QContextMenuEvent * cme ) { + QVector selectedTCOs = getClickedTCOs(); + // Depending on whether we right-clicked a selection or an individual TCO we will have // different labels for the actions. - bool individualTCO = getClickedTCOs().size() <= 1; + bool individualTCO = selectedTCOs.size() <= 1; if( cme->modifiers() ) { @@ -1134,6 +1138,45 @@ void TrackContentObjectView::contextMenuEvent( QContextMenuEvent * cme ) QMenu contextMenu( this ); + // We can only merge InstrumentTrack's TCOs, so check if we only have those in the selection, + // and also if they all belong to the same track + bool canMergeTCOs = false; + if( !individualTCO ) + { + // We can only merge if we have more then one TCO, so first set to true + canMergeTCOs = true; + + // Variable to check if all TCOs belong to the same track + TrackView *previousOwnerTrackView = nullptr; + + // Then we check every selected TCO to see if all of them are InstrumentTrack's TCOs. + // If any isn't, we set canMergeTCOs to false and quit the loop. + for( auto tcov: selectedTCOs ) + { + TrackView *ownerTrackView = tcov->getTrackView(); + + // Set the previousOwnerTrackView to the first TrackView + if( !previousOwnerTrackView ) + { + previousOwnerTrackView = ownerTrackView; + } + + // Are all TCOs from the same track? + if( ownerTrackView != previousOwnerTrackView ) + { + canMergeTCOs = false; + break; + } + + // Is the TCO from an InstrumentTrack? + if( ! dynamic_cast(ownerTrackView) ) + { + canMergeTCOs = false; + break; + } + } + } + if( fixedTCOs() == false ) { contextMenu.addAction( @@ -1151,6 +1194,14 @@ void TrackContentObjectView::contextMenuEvent( QContextMenuEvent * cme ) ? tr("Cut") : tr("Cut selection"), [this](){ contextMenuAction( Cut ); } ); + + if( canMergeTCOs ) + { + contextMenu.addAction( + embed::getIconPixmap( "edit_cut" ), + tr("Merge Selection"), + [this](){ contextMenuAction( Merge ); } ); + } } contextMenu.addAction( @@ -1202,6 +1253,9 @@ void TrackContentObjectView::contextMenuAction( ContextMenuAction action ) case Mute: toggleMute( active ); break; + case Merge: + mergeTCOs( active ); + break; } } @@ -1320,6 +1374,71 @@ void TrackContentObjectView::toggleMute( QVector tcovs } } +void TrackContentObjectView::mergeTCOs( QVector tcovs ) +{ + qWarning("Merging!"); + // Get the track that we are merging TCOs in + InstrumentTrack *track = dynamic_cast(tcovs.at(0)->getTrackView()->getTrack()); + + if( !track ) + { + qWarning("Warning: Couldn't retrieve InstrumentTrack in mergeTCOs()"); + return; + } + + // Find the earliest position of all the selected TCOVs + MidiTime earliestPos = tcovs.at(0)->getTrackContentObject()->startPosition(); + MidiTime currentPos = earliestPos; + + for( auto tcov: tcovs ) + { + currentPos = tcov->getTrackContentObject()->startPosition(); + if( currentPos < earliestPos ) + { + earliestPos = currentPos; + } + } + + // Create a pattern where all notes will be added + Pattern * newPattern = dynamic_cast( track->createTCO( earliestPos ) ); + if( !newPattern ) + { + qWarning("Warning: Failed to convert TCO to Pattern on mergeTCOs"); + return; + } + + newPattern->movePosition( earliestPos ); // Kinda weird we need to call this even though we already give the Pos on the constructor right? + + // Add the notes and remove the TCOs that are being merged + for( auto tcov: tcovs ) + { + // Convert TCOV to PatternView + PatternView *pView = dynamic_cast( tcov ); + + if( !pView ) + { + qWarning("Warning: Non-pattern TCO on InstrumentTrack"); + continue; + } + + NoteVector currentTCONotes = pView->getPattern()->notes(); + MidiTime pViewPos = pView->getPattern()->startPosition(); + + for( Note *note: currentTCONotes ) + { + Note *newNote = newPattern->addNote( *note, false ); + MidiTime originalNotePos = newNote->pos(); + newNote->setPos( originalNotePos + ( pViewPos - earliestPos ) ); + } + + // No need to check for nullptr because we check while building the tcovs QVector + tcov->remove(); + } + + // Because we might have moved notes beyond the current pattern length + newPattern->updateLength(); + newPattern->dataChanged(); +} diff --git a/src/tracks/Pattern.cpp b/src/tracks/Pattern.cpp index e5de8b83b68..324dd12b5cd 100644 --- a/src/tracks/Pattern.cpp +++ b/src/tracks/Pattern.cpp @@ -613,6 +613,17 @@ PatternView::PatternView( Pattern* pattern, TrackView* parent ) : setStyle( QApplication::style() ); } + + + +Pattern * PatternView::getPattern() +{ + return m_pat; +} + + + + void PatternView::update() { ToolTip::add(this, m_pat->name()); From 21dff27a4a2021cfd8003154bacb050163ea6e71 Mon Sep 17 00:00:00 2001 From: IanCaio Date: Sun, 4 Oct 2020 17:07:59 -0300 Subject: [PATCH 02/13] Fix bug about notes not being played This commit fixes a previous bug where some notes wouldn't play after a merge operation happened. Apparently the root cause was Pattern->rearrangeAllNotes() not being called. It seems to be crucial that the notes are arranged in the right order for them to play properly. Removes QDebug header and includes Note.h. --- src/core/Track.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/core/Track.cpp b/src/core/Track.cpp index 32c548f5a50..6cd656a25b5 100644 --- a/src/core/Track.cpp +++ b/src/core/Track.cpp @@ -46,7 +46,6 @@ #include #include #include -#include #include "AutomationPattern.h" @@ -71,6 +70,7 @@ #include "StringPairDrag.h" #include "TextFloat.h" #include "Pattern.h" +#include "Note.h" /*! The width of the resize grip in pixels @@ -1376,7 +1376,6 @@ void TrackContentObjectView::toggleMute( QVector tcovs void TrackContentObjectView::mergeTCOs( QVector tcovs ) { - qWarning("Merging!"); // Get the track that we are merging TCOs in InstrumentTrack *track = dynamic_cast(tcovs.at(0)->getTrackView()->getTrack()); @@ -1435,9 +1434,13 @@ void TrackContentObjectView::mergeTCOs( QVector tcovs tcov->remove(); } - // Because we might have moved notes beyond the current pattern length + // Update length since we might have moved notes beyond the end of the pattern length newPattern->updateLength(); - newPattern->dataChanged(); + // Rearrange notes because we might have moved them + newPattern->rearrangeAllNotes(); + // Update engine + Engine::getSong()->setModified(); + gui->songEditor()->update(); } From ff76e0fab16481f2c34de58bf0922c94d9fe1a79 Mon Sep 17 00:00:00 2001 From: IanCaio Date: Sun, 4 Oct 2020 18:16:19 -0300 Subject: [PATCH 03/13] Reorganizes code to make it ready for review This commit reorganizes the code a little bit to make it better suited for review. The code that checked whether a selection can be merged was extracted to a separate method called TrackContentObjectView::canMergeSelection(). Two conditionals inside that code were merged into a single one, since they both resulted in the same action. Also updated a comment next to a line of code that is probably going to be removed after the PR #5699 is merged (it's currently necessary because of a bug that is fixed on that PR). --- include/Track.h | 3 ++ src/core/Track.cpp | 75 ++++++++++++++++++++++------------------------ 2 files changed, 39 insertions(+), 39 deletions(-) diff --git a/include/Track.h b/include/Track.h index 49adc48084d..ca05a3e2d3c 100644 --- a/include/Track.h +++ b/include/Track.h @@ -265,6 +265,9 @@ class TrackContentObjectView : public selectableObject, public ModelView static void toggleMute( QVector tcovs ); static void mergeTCOs( QVector tcovs ); + // Returns true if selection can be merged and false if not + static bool canMergeSelection( QVector tcovs ); + public slots: virtual bool close(); void cut(); diff --git a/src/core/Track.cpp b/src/core/Track.cpp index 6cd656a25b5..82fa6e60f42 100644 --- a/src/core/Track.cpp +++ b/src/core/Track.cpp @@ -1138,44 +1138,7 @@ void TrackContentObjectView::contextMenuEvent( QContextMenuEvent * cme ) QMenu contextMenu( this ); - // We can only merge InstrumentTrack's TCOs, so check if we only have those in the selection, - // and also if they all belong to the same track - bool canMergeTCOs = false; - if( !individualTCO ) - { - // We can only merge if we have more then one TCO, so first set to true - canMergeTCOs = true; - - // Variable to check if all TCOs belong to the same track - TrackView *previousOwnerTrackView = nullptr; - - // Then we check every selected TCO to see if all of them are InstrumentTrack's TCOs. - // If any isn't, we set canMergeTCOs to false and quit the loop. - for( auto tcov: selectedTCOs ) - { - TrackView *ownerTrackView = tcov->getTrackView(); - - // Set the previousOwnerTrackView to the first TrackView - if( !previousOwnerTrackView ) - { - previousOwnerTrackView = ownerTrackView; - } - - // Are all TCOs from the same track? - if( ownerTrackView != previousOwnerTrackView ) - { - canMergeTCOs = false; - break; - } - - // Is the TCO from an InstrumentTrack? - if( ! dynamic_cast(ownerTrackView) ) - { - canMergeTCOs = false; - break; - } - } - } + bool canMergeTCOs = individualTCO ? false : canMergeSelection( selectedTCOs ); if( fixedTCOs() == false ) { @@ -1374,6 +1337,40 @@ void TrackContentObjectView::toggleMute( QVector tcovs } } +bool TrackContentObjectView::canMergeSelection( QVector tcovs ) +{ + // We can only merge InstrumentTrack's TCOs, so check if we only have those in the selection, + // and also if they all belong to the same track + + // We start assuming we can merge and if any condition is broken we set it to false + bool canMerge = true; + + // Variable to check if all TCOs belong to the same track + TrackView *previousOwnerTrackView = nullptr; + + // Then we check every selected TCO to see if all of them are InstrumentTrack's TCOs. + // If any isn't, we set canMerge to false and quit the loop. + for( auto tcov: tcovs ) + { + TrackView *ownerTrackView = tcov->getTrackView(); + + // Set the previousOwnerTrackView to the first TrackView + if( !previousOwnerTrackView ) + { + previousOwnerTrackView = ownerTrackView; + } + + // If there are TCOs from different tracks or TCOs from tracks other than an InstrumentTrack, can't merge them + if( ownerTrackView != previousOwnerTrackView || !dynamic_cast(ownerTrackView) ) + { + canMerge = false; + break; + } + } + + return canMerge; +} + void TrackContentObjectView::mergeTCOs( QVector tcovs ) { // Get the track that we are merging TCOs in @@ -1406,7 +1403,7 @@ void TrackContentObjectView::mergeTCOs( QVector tcovs return; } - newPattern->movePosition( earliestPos ); // Kinda weird we need to call this even though we already give the Pos on the constructor right? + newPattern->movePosition( earliestPos ); // TODO: Won't be necessary once #5699 is merged! // Add the notes and remove the TCOs that are being merged for( auto tcov: tcovs ) From 97905b26480aa524eaf0d0d7e6b5b6e383020193 Mon Sep 17 00:00:00 2001 From: IanCaio Date: Sun, 4 Oct 2020 21:01:48 -0300 Subject: [PATCH 04/13] Updates the icon for the merge action --- data/themes/classic/edit_merge.png | Bin 0 -> 611 bytes data/themes/default/edit_merge.png | Bin 0 -> 611 bytes src/core/Track.cpp | 2 +- 3 files changed, 1 insertion(+), 1 deletion(-) create mode 100644 data/themes/classic/edit_merge.png create mode 100644 data/themes/default/edit_merge.png diff --git a/data/themes/classic/edit_merge.png b/data/themes/classic/edit_merge.png new file mode 100644 index 0000000000000000000000000000000000000000..3e8742b86efca57563e9b1036d8fa15621987864 GIT binary patch literal 611 zcmV-p0-XJcP)r;zp4O@dpG&1<5KK!5HxYL=gl* z<}rK&HyXqj@CkGwxN|8)$ig8JLx_rG69_0Sgc;*ZRrhg`PMDafjvq91RUf|k?(O0N zpx#rL)o5OS)c5K<4GQXA;58ty3B(M06qp7Qa4m$;9ZRVn)TG{QI=N;$b=p@=&_MkH zU`h{+@wqA8S5J>&TXpH5n+#P~0gQ3pl&qhh>cug9PuuOO$0F5G^RnE9xe2 zd4z2N6Y6Zeu7Iy0gx-OKb=fbC@mZa6XjKgw{`{_z`cy6UCj=;pP6%OOSx%uHs!Cd| zMMBuhR9OJmfk(ji0|`fZ_13D|?PuQ@v-eLuqi(C~O@`mp-^Td3HvOf}nbPfy>6%)i zmg?FVU_4u?8+ZXM09|zvC~Ia4 zm{d;!Q-rVroC2naaRyk&%LlMeiJy4FtTwBxWvxI1$YCT0FH+chGWzG zV+dT`>qQ8mKU#)51gifr8&#{}+Yo50P0Xk!fMFB-377;P9_FOp0xkd_LjyDmEU_~b xwt(#ndIUIEzu!6RyP^U={!X002ovPDHLkV1h*i{JQ`E literal 0 HcmV?d00001 diff --git a/data/themes/default/edit_merge.png b/data/themes/default/edit_merge.png new file mode 100644 index 0000000000000000000000000000000000000000..3e8742b86efca57563e9b1036d8fa15621987864 GIT binary patch literal 611 zcmV-p0-XJcP)r;zp4O@dpG&1<5KK!5HxYL=gl* z<}rK&HyXqj@CkGwxN|8)$ig8JLx_rG69_0Sgc;*ZRrhg`PMDafjvq91RUf|k?(O0N zpx#rL)o5OS)c5K<4GQXA;58ty3B(M06qp7Qa4m$;9ZRVn)TG{QI=N;$b=p@=&_MkH zU`h{+@wqA8S5J>&TXpH5n+#P~0gQ3pl&qhh>cug9PuuOO$0F5G^RnE9xe2 zd4z2N6Y6Zeu7Iy0gx-OKb=fbC@mZa6XjKgw{`{_z`cy6UCj=;pP6%OOSx%uHs!Cd| zMMBuhR9OJmfk(ji0|`fZ_13D|?PuQ@v-eLuqi(C~O@`mp-^Td3HvOf}nbPfy>6%)i zmg?FVU_4u?8+ZXM09|zvC~Ia4 zm{d;!Q-rVroC2naaRyk&%LlMeiJy4FtTwBxWvxI1$YCT0FH+chGWzG zV+dT`>qQ8mKU#)51gifr8&#{}+Yo50P0Xk!fMFB-377;P9_FOp0xkd_LjyDmEU_~b xwt(#ndIUIEzu!6RyP^U={!X002ovPDHLkV1h*i{JQ`E literal 0 HcmV?d00001 diff --git a/src/core/Track.cpp b/src/core/Track.cpp index 82fa6e60f42..99fd033afb9 100644 --- a/src/core/Track.cpp +++ b/src/core/Track.cpp @@ -1161,7 +1161,7 @@ void TrackContentObjectView::contextMenuEvent( QContextMenuEvent * cme ) if( canMergeTCOs ) { contextMenu.addAction( - embed::getIconPixmap( "edit_cut" ), + embed::getIconPixmap( "edit_merge" ), tr("Merge Selection"), [this](){ contextMenuAction( Merge ); } ); } From e43852952c94f0cdce1afc474bb9453b5fa6036e Mon Sep 17 00:00:00 2001 From: IanCaio Date: Sat, 10 Oct 2020 16:29:33 -0300 Subject: [PATCH 05/13] Fixes history system for the merge operation Fixes the history system for the merge operation by adding a journal checkpoint on the track where the merge is happening and disabling the journalling state of everything being handled during the operation, restoring the journalling state at the end. --- src/core/Track.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/core/Track.cpp b/src/core/Track.cpp index 3fa52c50000..6ea07a4818d 100644 --- a/src/core/Track.cpp +++ b/src/core/Track.cpp @@ -1324,6 +1324,10 @@ void TrackContentObjectView::mergeTCOs( QVector tcovs return; } + // For Undo/Redo + track->addJournalCheckPoint(); + track->saveJournallingState( false ); + // Find the earliest position of all the selected TCOVs MidiTime earliestPos = tcovs.at(0)->getTrackContentObject()->startPosition(); MidiTime currentPos = earliestPos; @@ -1345,6 +1349,7 @@ void TrackContentObjectView::mergeTCOs( QVector tcovs return; } + newPattern->saveJournallingState( false ); newPattern->movePosition( earliestPos ); // TODO: Won't be necessary once #5699 is merged! // Add the notes and remove the TCOs that are being merged @@ -1369,14 +1374,23 @@ void TrackContentObjectView::mergeTCOs( QVector tcovs newNote->setPos( originalNotePos + ( pViewPos - earliestPos ) ); } + // We disable the journalling system before removing, so the + // removal doesn't get added to the undo/redo history + tcov->getTrackContentObject()->saveJournallingState( false ); // No need to check for nullptr because we check while building the tcovs QVector tcov->remove(); + // TODO: Is it a good idea to restore the journalling state after remove() + // or is the next line completely unnecessary? + tcov->getTrackContentObject()->restoreJournallingState(); } // Update length since we might have moved notes beyond the end of the pattern length newPattern->updateLength(); // Rearrange notes because we might have moved them newPattern->rearrangeAllNotes(); + // Restore journalling states now that the operation is finished + track->restoreJournallingState(); + newPattern->restoreJournallingState(); // Update engine Engine::getSong()->setModified(); gui->songEditor()->update(); From 5312c45585a68043a9d06551b1292e00200ea56b Mon Sep 17 00:00:00 2001 From: Ian Caio Date: Fri, 27 Nov 2020 01:02:04 -0300 Subject: [PATCH 06/13] Removes method call not needed anymore After the bug fix from #5699, there's no need to call movePosition on a TCO that was created with a position on its constructor. The line that called movePosition was removed. Code style was also fixed. --- include/Track.h | 4 +-- src/core/Track.cpp | 61 ++++++++++++++++++++++++---------------------- 2 files changed, 34 insertions(+), 31 deletions(-) diff --git a/include/Track.h b/include/Track.h index 38ac5fc8d0b..c40eab3494c 100644 --- a/include/Track.h +++ b/include/Track.h @@ -289,10 +289,10 @@ class TrackContentObjectView : public selectableObject, public ModelView // some metadata to be written to the clipboard. static void remove( QVector tcovs ); static void toggleMute( QVector tcovs ); - static void mergeTCOs( QVector tcovs ); + static void mergeTCOs(QVector tcovs); // Returns true if selection can be merged and false if not - static bool canMergeSelection( QVector tcovs ); + static bool canMergeSelection(QVector tcovs); QColor getColorForDisplay( QColor ); diff --git a/src/core/Track.cpp b/src/core/Track.cpp index c5487702fbe..0357a1ae9f1 100644 --- a/src/core/Track.cpp +++ b/src/core/Track.cpp @@ -1167,7 +1167,7 @@ void TrackContentObjectView::contextMenuEvent( QContextMenuEvent * cme ) QMenu contextMenu( this ); - bool canMergeTCOs = individualTCO ? false : canMergeSelection( selectedTCOs ); + bool canMergeTCOs = individualTCO ? false : canMergeSelection(selectedTCOs); if( fixedTCOs() == false ) { @@ -1187,12 +1187,13 @@ void TrackContentObjectView::contextMenuEvent( QContextMenuEvent * cme ) : tr("Cut selection"), [this](){ contextMenuAction( Cut ); } ); - if( canMergeTCOs ) + if (canMergeTCOs) { contextMenu.addAction( - embed::getIconPixmap( "edit_merge" ), + embed::getIconPixmap("edit_merge"), tr("Merge Selection"), - [this](){ contextMenuAction( Merge ); } ); + [this](){ contextMenuAction(Merge); } + ); } } @@ -1253,7 +1254,7 @@ void TrackContentObjectView::contextMenuAction( ContextMenuAction action ) toggleMute( active ); break; case Merge: - mergeTCOs( active ); + mergeTCOs(active); break; } } @@ -1338,7 +1339,7 @@ void TrackContentObjectView::toggleMute( QVector tcovs } } -bool TrackContentObjectView::canMergeSelection( QVector tcovs ) +bool TrackContentObjectView::canMergeSelection(QVector tcovs) { // We can only merge InstrumentTrack's TCOs, so check if we only have those in the selection, // and also if they all belong to the same track @@ -1347,22 +1348,24 @@ bool TrackContentObjectView::canMergeSelection( QVectorgetTrackView(); + TrackView * ownerTrackView = tcov->getTrackView(); // Set the previousOwnerTrackView to the first TrackView - if( !previousOwnerTrackView ) + if (!previousOwnerTrackView) { previousOwnerTrackView = ownerTrackView; } - // If there are TCOs from different tracks or TCOs from tracks other than an InstrumentTrack, can't merge them - if( ownerTrackView != previousOwnerTrackView || !dynamic_cast(ownerTrackView) ) + // If there are TCOs from different tracks or TCOs from tracks + // other than an InstrumentTrack, can't merge them + if (ownerTrackView != previousOwnerTrackView + || !dynamic_cast(ownerTrackView)) { canMerge = false; break; @@ -1372,12 +1375,13 @@ bool TrackContentObjectView::canMergeSelection( QVector tcovs ) +void TrackContentObjectView::mergeTCOs(QVector tcovs) { // Get the track that we are merging TCOs in - InstrumentTrack *track = dynamic_cast(tcovs.at(0)->getTrackView()->getTrack()); + InstrumentTrack * track = + dynamic_cast(tcovs.at(0)->getTrackView()->getTrack()); - if( !track ) + if (!track) { qWarning("Warning: Couldn't retrieve InstrumentTrack in mergeTCOs()"); return; @@ -1385,39 +1389,38 @@ void TrackContentObjectView::mergeTCOs( QVector tcovs // For Undo/Redo track->addJournalCheckPoint(); - track->saveJournallingState( false ); + track->saveJournallingState(false); // Find the earliest position of all the selected TCOVs MidiTime earliestPos = tcovs.at(0)->getTrackContentObject()->startPosition(); MidiTime currentPos = earliestPos; - for( auto tcov: tcovs ) + for (auto tcov: tcovs) { currentPos = tcov->getTrackContentObject()->startPosition(); - if( currentPos < earliestPos ) + if (currentPos < earliestPos) { earliestPos = currentPos; } } // Create a pattern where all notes will be added - Pattern * newPattern = dynamic_cast( track->createTCO( earliestPos ) ); - if( !newPattern ) + Pattern * newPattern = dynamic_cast(track->createTCO(earliestPos)); + if (!newPattern) { qWarning("Warning: Failed to convert TCO to Pattern on mergeTCOs"); return; } - newPattern->saveJournallingState( false ); - newPattern->movePosition( earliestPos ); // TODO: Won't be necessary once #5699 is merged! + newPattern->saveJournallingState(false); // Add the notes and remove the TCOs that are being merged - for( auto tcov: tcovs ) + for (auto tcov: tcovs) { // Convert TCOV to PatternView - PatternView *pView = dynamic_cast( tcov ); + PatternView * pView = dynamic_cast(tcov); - if( !pView ) + if (!pView) { qWarning("Warning: Non-pattern TCO on InstrumentTrack"); continue; @@ -1426,16 +1429,16 @@ void TrackContentObjectView::mergeTCOs( QVector tcovs NoteVector currentTCONotes = pView->getPattern()->notes(); MidiTime pViewPos = pView->getPattern()->startPosition(); - for( Note *note: currentTCONotes ) + for (Note * note: currentTCONotes) { - Note *newNote = newPattern->addNote( *note, false ); + Note * newNote = newPattern->addNote(*note, false); MidiTime originalNotePos = newNote->pos(); - newNote->setPos( originalNotePos + ( pViewPos - earliestPos ) ); + newNote->setPos(originalNotePos + (pViewPos - earliestPos)); } // We disable the journalling system before removing, so the // removal doesn't get added to the undo/redo history - tcov->getTrackContentObject()->saveJournallingState( false ); + tcov->getTrackContentObject()->saveJournallingState(false); // No need to check for nullptr because we check while building the tcovs QVector tcov->remove(); // TODO: Is it a good idea to restore the journalling state after remove() From 6114b1d213895bcd3cc3a039c8027a0af937e97e Mon Sep 17 00:00:00 2001 From: Ian Caio Date: Mon, 30 Nov 2020 20:09:01 -0300 Subject: [PATCH 07/13] Updates PR to use TimePos After recently merge that renames MidiTime to TimePos, the code was updated to use the new class name. --- src/core/Track.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/core/Track.cpp b/src/core/Track.cpp index e9203973d53..dfad8f41681 100644 --- a/src/core/Track.cpp +++ b/src/core/Track.cpp @@ -1392,8 +1392,8 @@ void TrackContentObjectView::mergeTCOs(QVector tcovs) track->saveJournallingState(false); // Find the earliest position of all the selected TCOVs - MidiTime earliestPos = tcovs.at(0)->getTrackContentObject()->startPosition(); - MidiTime currentPos = earliestPos; + TimePos earliestPos = tcovs.at(0)->getTrackContentObject()->startPosition(); + TimePos currentPos = earliestPos; for (auto tcov: tcovs) { @@ -1427,12 +1427,12 @@ void TrackContentObjectView::mergeTCOs(QVector tcovs) } NoteVector currentTCONotes = pView->getPattern()->notes(); - MidiTime pViewPos = pView->getPattern()->startPosition(); + TimePos pViewPos = pView->getPattern()->startPosition(); for (Note * note: currentTCONotes) { Note * newNote = newPattern->addNote(*note, false); - MidiTime originalNotePos = newNote->pos(); + TimePos originalNotePos = newNote->pos(); newNote->setPos(originalNotePos + (pViewPos - earliestPos)); } From 2a368c080b8cc40d70147585c32f21aa5b3fbc27 Mon Sep 17 00:00:00 2001 From: Ian Caio Date: Tue, 8 Dec 2020 20:05:12 -0300 Subject: [PATCH 08/13] Fixes pointer declaration code style --- include/Pattern.h | 2 +- include/TrackContentObjectView.h | 4 ++-- src/gui/TrackContentObjectView.cpp | 24 ++++++++++++------------ src/tracks/Pattern.cpp | 2 +- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/include/Pattern.h b/include/Pattern.h index b010ad756d7..0a73376c024 100644 --- a/include/Pattern.h +++ b/include/Pattern.h @@ -176,7 +176,7 @@ class PatternView : public TrackContentObjectView void setMutedNoteBorderColor(QColor const & color) { m_mutedNoteBorderColor = color; } public slots: - Pattern * getPattern(); + Pattern* getPattern(); void update() override; diff --git a/include/TrackContentObjectView.h b/include/TrackContentObjectView.h index e8b8b03c516..e9461c5dd50 100644 --- a/include/TrackContentObjectView.h +++ b/include/TrackContentObjectView.h @@ -111,10 +111,10 @@ class TrackContentObjectView : public selectableObject, public ModelView // some metadata to be written to the clipboard. static void remove( QVector tcovs ); static void toggleMute( QVector tcovs ); - static void mergeTCOs(QVector tcovs); + static void mergeTCOs(QVector tcovs); // Returns true if selection can be merged and false if not - static bool canMergeSelection(QVector tcovs); + static bool canMergeSelection(QVector tcovs); QColor getColorForDisplay( QColor ); diff --git a/src/gui/TrackContentObjectView.cpp b/src/gui/TrackContentObjectView.cpp index 226a9cf6dd9..2b2d8815cd2 100644 --- a/src/gui/TrackContentObjectView.cpp +++ b/src/gui/TrackContentObjectView.cpp @@ -942,7 +942,7 @@ void TrackContentObjectView::mouseReleaseEvent( QMouseEvent * me ) */ void TrackContentObjectView::contextMenuEvent( QContextMenuEvent * cme ) { - QVector selectedTCOs = getClickedTCOs(); + QVector selectedTCOs = getClickedTCOs(); // Depending on whether we right-clicked a selection or an individual TCO we will have // different labels for the actions. @@ -1127,7 +1127,7 @@ void TrackContentObjectView::toggleMute( QVector tcovs } } -bool TrackContentObjectView::canMergeSelection(QVector tcovs) +bool TrackContentObjectView::canMergeSelection(QVector tcovs) { // We can only merge InstrumentTrack's TCOs, so check if we only have those in the selection, // and also if they all belong to the same track @@ -1136,13 +1136,13 @@ bool TrackContentObjectView::canMergeSelection(QVector bool canMerge = true; // Variable to check if all TCOs belong to the same track - TrackView * previousOwnerTrackView = nullptr; + TrackView* previousOwnerTrackView = nullptr; // Then we check every selected TCO to see if all of them are InstrumentTrack's TCOs. // If any isn't, we set canMerge to false and quit the loop. for (auto tcov: tcovs) { - TrackView * ownerTrackView = tcov->getTrackView(); + TrackView* ownerTrackView = tcov->getTrackView(); // Set the previousOwnerTrackView to the first TrackView if (!previousOwnerTrackView) @@ -1153,7 +1153,7 @@ bool TrackContentObjectView::canMergeSelection(QVector // If there are TCOs from different tracks or TCOs from tracks // other than an InstrumentTrack, can't merge them if (ownerTrackView != previousOwnerTrackView - || !dynamic_cast(ownerTrackView)) + || !dynamic_cast(ownerTrackView)) { canMerge = false; break; @@ -1163,11 +1163,11 @@ bool TrackContentObjectView::canMergeSelection(QVector return canMerge; } -void TrackContentObjectView::mergeTCOs(QVector tcovs) +void TrackContentObjectView::mergeTCOs(QVector tcovs) { // Get the track that we are merging TCOs in - InstrumentTrack * track = - dynamic_cast(tcovs.at(0)->getTrackView()->getTrack()); + InstrumentTrack* track = + dynamic_cast(tcovs.at(0)->getTrackView()->getTrack()); if (!track) { @@ -1193,7 +1193,7 @@ void TrackContentObjectView::mergeTCOs(QVector tcovs) } // Create a pattern where all notes will be added - Pattern * newPattern = dynamic_cast(track->createTCO(earliestPos)); + Pattern* newPattern = dynamic_cast(track->createTCO(earliestPos)); if (!newPattern) { qWarning("Warning: Failed to convert TCO to Pattern on mergeTCOs"); @@ -1206,7 +1206,7 @@ void TrackContentObjectView::mergeTCOs(QVector tcovs) for (auto tcov: tcovs) { // Convert TCOV to PatternView - PatternView * pView = dynamic_cast(tcov); + PatternView* pView = dynamic_cast(tcov); if (!pView) { @@ -1217,9 +1217,9 @@ void TrackContentObjectView::mergeTCOs(QVector tcovs) NoteVector currentTCONotes = pView->getPattern()->notes(); TimePos pViewPos = pView->getPattern()->startPosition(); - for (Note * note: currentTCONotes) + for (Note* note: currentTCONotes) { - Note * newNote = newPattern->addNote(*note, false); + Note* newNote = newPattern->addNote(*note, false); TimePos originalNotePos = newNote->pos(); newNote->setPos(originalNotePos + (pViewPos - earliestPos)); } diff --git a/src/tracks/Pattern.cpp b/src/tracks/Pattern.cpp index 2d99e12366a..659a1919c19 100644 --- a/src/tracks/Pattern.cpp +++ b/src/tracks/Pattern.cpp @@ -630,7 +630,7 @@ PatternView::PatternView( Pattern* pattern, TrackView* parent ) : -Pattern * PatternView::getPattern() +Pattern* PatternView::getPattern() { return m_pat; } From 011b03915eec9a3027a76dc0e1ad73988f7a179e Mon Sep 17 00:00:00 2001 From: Ian Caio Date: Tue, 8 Dec 2020 20:07:39 -0300 Subject: [PATCH 09/13] Remove unnecessary call to restoreJournallingState After removing a TCO I was calling restoreJournallingState on it, thinking it could be necessary for the journalling state be restored when the history Undo brings the TCO back. It doesn't make much sense that this would be necessary, so I'm removing it. After testing, it seems like restoring the TCO through Undo brings it back with the journalling state restored. --- src/gui/TrackContentObjectView.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/gui/TrackContentObjectView.cpp b/src/gui/TrackContentObjectView.cpp index 2b2d8815cd2..073168d289d 100644 --- a/src/gui/TrackContentObjectView.cpp +++ b/src/gui/TrackContentObjectView.cpp @@ -1229,9 +1229,6 @@ void TrackContentObjectView::mergeTCOs(QVector tcovs) tcov->getTrackContentObject()->saveJournallingState(false); // No need to check for nullptr because we check while building the tcovs QVector tcov->remove(); - // TODO: Is it a good idea to restore the journalling state after remove() - // or is the next line completely unnecessary? - tcov->getTrackContentObject()->restoreJournallingState(); } // Update length since we might have moved notes beyond the end of the pattern length From 7eb4b285048aefa562c2cd60f48f829961d0e390 Mon Sep 17 00:00:00 2001 From: Ian Caio Date: Wed, 9 Dec 2020 13:31:03 -0300 Subject: [PATCH 10/13] Apply PhysSong suggestions Change canMergeTCOs to use logical operators instead of the ternary operator. Avoids repeating the dynamic_cast unnecessarily on the canMergeSelection method. --- src/gui/TrackContentObjectView.cpp | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/gui/TrackContentObjectView.cpp b/src/gui/TrackContentObjectView.cpp index 073168d289d..d29587c3668 100644 --- a/src/gui/TrackContentObjectView.cpp +++ b/src/gui/TrackContentObjectView.cpp @@ -955,7 +955,7 @@ void TrackContentObjectView::contextMenuEvent( QContextMenuEvent * cme ) QMenu contextMenu( this ); - bool canMergeTCOs = individualTCO ? false : canMergeSelection(selectedTCOs); + bool canMergeTCOs = !individualTCO && canMergeSelection(selectedTCOs); if( fixedTCOs() == false ) { @@ -1132,35 +1132,31 @@ bool TrackContentObjectView::canMergeSelection(QVector // We can only merge InstrumentTrack's TCOs, so check if we only have those in the selection, // and also if they all belong to the same track - // We start assuming we can merge and if any condition is broken we set it to false - bool canMerge = true; - // Variable to check if all TCOs belong to the same track TrackView* previousOwnerTrackView = nullptr; - // Then we check every selected TCO to see if all of them are InstrumentTrack's TCOs. - // If any isn't, we set canMerge to false and quit the loop. for (auto tcov: tcovs) { TrackView* ownerTrackView = tcov->getTrackView(); // Set the previousOwnerTrackView to the first TrackView + // If the track isn't an InstrumentTrack can't merge the TCOs if (!previousOwnerTrackView) { + if (!dynamic_cast(ownerTrackView)) { return false; } + previousOwnerTrackView = ownerTrackView; } - // If there are TCOs from different tracks or TCOs from tracks - // other than an InstrumentTrack, can't merge them - if (ownerTrackView != previousOwnerTrackView - || !dynamic_cast(ownerTrackView)) + // If the current TCO is from a different track we can't merge them + if (ownerTrackView != previousOwnerTrackView) { - canMerge = false; - break; + return false; } } - return canMerge; + // If none of the conditions were broke, we can merge + return true; } void TrackContentObjectView::mergeTCOs(QVector tcovs) From 0d3ea7d13f9ddae12ee011e39fafd6fe7fe07e2d Mon Sep 17 00:00:00 2001 From: Ian Caio Date: Wed, 9 Dec 2020 14:19:59 -0300 Subject: [PATCH 11/13] Applies Spekular's suggestions Saves lots of lines on canMergeSelection by using a data set to store all the TCO owner tracks, so we can later check how many tracks we have. Uses std::min_element to find the earliest TCOV and get it's starting position. --- src/gui/TrackContentObjectView.cpp | 51 ++++++++++-------------------- 1 file changed, 16 insertions(+), 35 deletions(-) diff --git a/src/gui/TrackContentObjectView.cpp b/src/gui/TrackContentObjectView.cpp index d29587c3668..f316a7b17a1 100644 --- a/src/gui/TrackContentObjectView.cpp +++ b/src/gui/TrackContentObjectView.cpp @@ -24,6 +24,8 @@ #include "TrackContentObjectView.h" +#include + #include #include #include @@ -1129,34 +1131,15 @@ void TrackContentObjectView::toggleMute( QVector tcovs bool TrackContentObjectView::canMergeSelection(QVector tcovs) { - // We can only merge InstrumentTrack's TCOs, so check if we only have those in the selection, - // and also if they all belong to the same track + // We check if the owner of the first TCO is an Instrument Track + bool isInstrumentTrack = dynamic_cast(tcovs.at(0)->getTrackView()); - // Variable to check if all TCOs belong to the same track - TrackView* previousOwnerTrackView = nullptr; + // Then we create a set with all the TCOs owners + std::set ownerTracks; + for (auto tcov: tcovs) { ownerTracks.insert(tcov->getTrackView()); } - for (auto tcov: tcovs) - { - TrackView* ownerTrackView = tcov->getTrackView(); - - // Set the previousOwnerTrackView to the first TrackView - // If the track isn't an InstrumentTrack can't merge the TCOs - if (!previousOwnerTrackView) - { - if (!dynamic_cast(ownerTrackView)) { return false; } - - previousOwnerTrackView = ownerTrackView; - } - - // If the current TCO is from a different track we can't merge them - if (ownerTrackView != previousOwnerTrackView) - { - return false; - } - } - - // If none of the conditions were broke, we can merge - return true; + // Can merge if there's only one owner track and it's an Instrument Track + return isInstrumentTrack && ownerTracks.size() == 1; } void TrackContentObjectView::mergeTCOs(QVector tcovs) @@ -1176,17 +1159,15 @@ void TrackContentObjectView::mergeTCOs(QVector tcovs) track->saveJournallingState(false); // Find the earliest position of all the selected TCOVs - TimePos earliestPos = tcovs.at(0)->getTrackContentObject()->startPosition(); - TimePos currentPos = earliestPos; - - for (auto tcov: tcovs) - { - currentPos = tcov->getTrackContentObject()->startPosition(); - if (currentPos < earliestPos) + const auto earliestTCOV = std::min_element(tcovs.constBegin(), tcovs.constEnd(), + [](TrackContentObjectView* a, TrackContentObjectView* b) { - earliestPos = currentPos; + return a->getTrackContentObject()->startPosition() < + b->getTrackContentObject()->startPosition(); } - } + ); + + const TimePos earliestPos = (*earliestTCOV)->getTrackContentObject()->startPosition(); // Create a pattern where all notes will be added Pattern* newPattern = dynamic_cast(track->createTCO(earliestPos)); From bed7c710f293603b74e46073042a1f2752575a6d Mon Sep 17 00:00:00 2001 From: Ian Caio Date: Wed, 9 Dec 2020 14:41:59 -0300 Subject: [PATCH 12/13] canMergeSelection now checks number of TCOs The logic that returned we couldn't merge a single TCO was inside contextMenuEvent, when it suited more the canMergeSelection method. That was moved there and now even if canMergeSelection receives a list with just a single TCO it will return that it can't be merged. --- src/gui/TrackContentObjectView.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/gui/TrackContentObjectView.cpp b/src/gui/TrackContentObjectView.cpp index f316a7b17a1..872c5d2770d 100644 --- a/src/gui/TrackContentObjectView.cpp +++ b/src/gui/TrackContentObjectView.cpp @@ -957,8 +957,6 @@ void TrackContentObjectView::contextMenuEvent( QContextMenuEvent * cme ) QMenu contextMenu( this ); - bool canMergeTCOs = !individualTCO && canMergeSelection(selectedTCOs); - if( fixedTCOs() == false ) { contextMenu.addAction( @@ -977,7 +975,7 @@ void TrackContentObjectView::contextMenuEvent( QContextMenuEvent * cme ) : tr("Cut selection"), [this](){ contextMenuAction( Cut ); } ); - if (canMergeTCOs) + if (canMergeSelection(selectedTCOs)) { contextMenu.addAction( embed::getIconPixmap("edit_merge"), @@ -1131,6 +1129,9 @@ void TrackContentObjectView::toggleMute( QVector tcovs bool TrackContentObjectView::canMergeSelection(QVector tcovs) { + // Can't merge a single TCO + if (tcovs.size() < 2) { return false; } + // We check if the owner of the first TCO is an Instrument Track bool isInstrumentTrack = dynamic_cast(tcovs.at(0)->getTrackView()); From 1c612163ae20f728635f0b22839f7c214c136f66 Mon Sep 17 00:00:00 2001 From: Ian Caio Date: Thu, 10 Dec 2020 05:46:28 -0300 Subject: [PATCH 13/13] Changes order of restoreJournallingState calls Calls restoreJournallingState respecting the push/pop order in relation to the saveJournallingState calls. Fix a comment. --- src/gui/TrackContentObjectView.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/gui/TrackContentObjectView.cpp b/src/gui/TrackContentObjectView.cpp index 872c5d2770d..6aa78a1dd6e 100644 --- a/src/gui/TrackContentObjectView.cpp +++ b/src/gui/TrackContentObjectView.cpp @@ -1214,9 +1214,9 @@ void TrackContentObjectView::mergeTCOs(QVector tcovs) // Rearrange notes because we might have moved them newPattern->rearrangeAllNotes(); // Restore journalling states now that the operation is finished - track->restoreJournallingState(); newPattern->restoreJournallingState(); - // Update engine + track->restoreJournallingState(); + // Update song Engine::getSong()->setModified(); gui->songEditor()->update(); }