Add an option to replace the autoDJ queue with the selected tracks#1163
Add an option to replace the autoDJ queue with the selected tracks#1163rryan merged 4 commits intomixxxdj:masterfrom potocpav:djqueue
Conversation
potocpav
left a comment
potocpav
left a comment
There was a problem hiding this comment.
Added some comments/questions to the changes
| BOTTOM, | ||
| REPLACE, | ||
| }; | ||
|
|
There was a problem hiding this comment.
Is this enum defined in the right place? Should it be scoped? It leads to e.g. PlaylistDAO::AutoDJSendLoc::BOTTOM, which seems a little ugly.
There was a problem hiding this comment.
It's fine with me -- regardless it'll be easy to move later.
There was a problem hiding this comment.
Hm, maybe it should eject the loaded, stopped track, remove it from the top of the queue, and load the new first track in the queue.
There was a problem hiding this comment.
Looks like a more useful variant to me
There was a problem hiding this comment.
I would not touch the track, loaded on the deck. This could be an action out of sight.
Removing the top track from Auto DJ will remove the loaded track anyway if the user enables AutoDJ.
| #include "library/coverart.h" | ||
| #include "library/dlgtagfetcher.h" | ||
| #include "library/libraryview.h" | ||
| #include "library/trackcollection.h" |
There was a problem hiding this comment.
This is to pull in PlaylistDAO::AutoDJSendLoc
|
|
||
| bool PlaylistDAO::clearPlaylist(const int playlistId) { | ||
| int position = | ||
| (m_pAutoDJProcessor && m_pAutoDJProcessor->nextTrackLoaded()) ? 2 : 1; |
There was a problem hiding this comment.
When replacing the autoDJ queue, the first track is retained if it is loaded in a deck. Is it the desired behavior?
There was a problem hiding this comment.
Yes, AutoDJProcessor relies on the currently playing track being at the top of the ADJ queue.
There was a problem hiding this comment.
Hm, though this is a general "clearPlaylist" function so it shouldn't have AutoDJ logic built in. Maybe a better name would be "removeTracksFromPlaylist(int startIndex)" and then you would pass in the appropriate start index from sendToAutoDJ as the add-to-top handling works.
|
|
||
| bool PlaylistDAO::replaceTracksInPlaylist(const QList<TrackId>& trackIds, const int playlistId) { | ||
| return clearPlaylist(playlistId) | ||
| && appendTracksToPlaylist(trackIds, playlistId); |
There was a problem hiding this comment.
If the clearPlaylist fails, the appeldTracksToPlaylist is skipped (addressing the comment in the previous PR).
| sendToAutoDJ(true); // add track to Auto-DJ Queue (top) | ||
| sendToAutoDJ(PlaylistDAO::AutoDJSendLoc::TOP); // add track to Auto-DJ Queue (top) | ||
| break; | ||
| // case DlgPrefLibrary::ADD_TRACK_REPLACE: |
There was a problem hiding this comment.
could you remove this comment please?
| } | ||
|
|
||
| bool PlaylistDAO::replaceTracksInPlaylist(const QList<TrackId>& trackIds, const int playlistId) { | ||
| return clearPlaylist(playlistId) |
There was a problem hiding this comment.
Since this is so short and only used in one place, I would write it directly in sendToAutoDJ instead of adding a method.
There was a problem hiding this comment.
OK, that's better.
|
Thanks very much @potocpav! Have you signed our contributor agreement yet? |
|
@rryan yes I did, this is not my first contribution. |
… autoDJ logic to `sendToAutoDJ`.
|
Thank you @potocpav -- this LGTM. I assume the Travis mac build failure is just noise. |
4 participants
Issue: https://bugs.launchpad.net/mixxx/+bug/1566206
This continues the work that started in #915.