Synchronize AutoDJ next deck with top track in queue #12909

Bacadam · 2024-03-03T22:44:05Z

This fixes bug #8956, adding a signal in playlisttablemodel which is emitted when a change in track order involves the first track. The signal is connected to a slot in autodjprocessor which skips the track in next deck, replacing it with the new top track in queue.

JoergAtGithub · 2024-03-03T22:54:14Z

Welcome at Mixxx!
As a first-time contributor we need you to sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

ronso0 · 2024-03-03T23:13:02Z

I agree this is a bugfix, so this should go to the 2.4 branch.
@Bacadam please chnage the base branch here, and if there are unrelated commits shown also rebase your branch onto 2.4 and force-push.

Thank you!

ronso0

Thank you for this fix, works as desired.

ronso0 · 2024-03-03T23:14:53Z

src/library/playlisttablemodel.cpp

@@ -275,6 +279,11 @@ void PlaylistTableModel::moveTrack(const QModelIndex& sourceIndex,
    }

    m_pTrackCollectionManager->internalCollection()->getPlaylistDAO().moveTrack(m_iPlaylistId, oldPosition, newPosition);
+
+    if (oldPosition==1 || newPosition==1) {
+        qDebug() << "First track changed";


IMO we don't need this debug message here, the one in AutoDJProcessor is sufficient.

clang-format is complaining here.
You can avoid these complains by installing
pre-commit
https://github.com/mixxxdj/mixxx/wiki/Using-Git#set-up-automatic-code-checking

Bacadam · 2024-03-04T17:20:03Z

Welcome at Mixxx! As a first-time contributor we need you to sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

Thanks! I've signed it under the name Bacadam.

daschuer

I have an issue:

Load the second track from the Auto-DJ playlist to the paused deck.
Now the we have an "inconsistent state"
Fix it by moving the track up.
The track disappears from the playlist and the original first track is loaded
This should not happen.

Bacadam · 2024-03-04T23:10:19Z

Thank you for this fix, works as desired.

Thanks, I'll do the requested changes and the rebase tomorrow.

I have an issue:

* Load the second track from the Auto-DJ playlist to the paused deck.

* Now the we have an "inconsistent state"

* Fix it by moving the track up.

* The track disappears from the playlist and the original first track is loaded
  This should not happen.

Hum okay I will reproduce it and see what I can do.

Bacadam · 2024-03-06T22:18:54Z

Sorry, I force pushed before having change the base branch here and so Github bot added some irrelevant tags.

JoergAtGithub · 2024-03-12T08:22:39Z

The pre-commit check is failing. The best way to fix pre-commit issues is to install pre-commit locally on your system, as described here: https://github.com/mixxxdj/mixxx/wiki/Using-Git#set-up-automatic-code-checking
Than it will fix these issues automatically before every commit.
Alternatively, you can download the pre-commit.patch file from the artifacts of this PR

unzip it, and apply it using:
git apply pre-commit.patch

Bacadam · 2024-03-12T21:07:30Z

Okay I've installed pre-commit and I think I solved all clang-format issues with last commit

ronso0 · 2024-03-12T21:44:09Z

I think you can squash all three commits. (and force-push)

Did you manage to fix the issue @daschuer discovered?

Bacadam · 2024-03-12T21:54:17Z

The behavior @daschuer discovered is due to the calls to removeLoadedTrackFromTopOfQueue in skipNext.
I tried to copy all the skipNext code in the playlistFirstTrackChanged() slot, without the removeLoadedTrackFromTopOfQueue lines and it worked.

The question now is what are the checks in skipNext that are useful in our case and that must be done in the slot as well?

ronso0 · 2024-03-12T21:57:19Z

I suggest to first squash the current state, push, then append an experimental commit so we can discuss the code in place.

Bacadam · 2024-03-14T13:24:16Z

I copied the skipNext code and kept what I think is useful. I tested it and it works.

Bacadam · 2024-03-14T13:27:39Z

src/library/autodj/autodjprocessor.cpp

+        // if (!pLeftDeck || !pRightDeck) {
+        //     // User has changed the orientation, disable Auto DJ
+        //     toggleAutoDJ(false);
+        //     emit autoDJError(ADJ_NOT_TWO_DECKS);
+        //     return ADJ_NOT_TWO_DECKS;
+        // }


I just don't know what to do with these commented lines as I don't know what deck orientation is, and how user can change it.

orientation is the side of the crossfader a deck is assigned to (left - center - right).

Not sure whether we need to do these checks here, I'd say no.

Bacadam · 2024-03-14T13:38:58Z

I found another thing that we can consider as a bug now that this synchronization is implemented.
When you right-click on tracks or playlists and select Add to AutoDJ (top) or Add to AutoDJ (replace), the track are added in second position, and so the next deck is not modified.

Bacadam · 2024-03-14T13:50:03Z

I found another thing that we can consider as a bug now that this synchronization is implemented. When you right-click on tracks or playlists and select Add to AutoDJ (top) or Add to AutoDJ (replace), the track are added in second position, and so the next deck is not modified.

The code responsible for that is in playlistdao.cpp, and I proposed a working solution.
The problem is I need to call my playlistFirstTrackChanged()slot from playlistdao.cppand I don't know what's the right way.
For my precedent commit to work, I changed playlistFirstTrackChanged() to public slot but that may not be a really good practice?

daschuer · 2024-03-15T08:16:08Z

When you right-click on tracks or playlists and select Add to AutoDJ (top) or Add to AutoDJ (replace), the track are added in second position, and so the next deck is not modified.

I think that is the desired behavior. The use case is that the users may have already cued the track in the second deck, and just wants to collect some other tracks while browsing the library.

If hey want to play the track after the next they can load the track directly into the deck. So we have both cases covered.

Bacadam · 2024-03-26T20:34:55Z

Are we sure about that? If the users have selected the next track and browse more tracks, they probably want to add them to the end of the queue? Why would they especially want to add a track in third position?

If they use AutoDJ, they probably don't load directly in decks?

I might have a "music player" approach on that, this is how most music players work. But I can't tell for DJ softwares.

Anyway, what you prefer, I can revert my last commit if you want.

This reverts commit 9e1eec4.

Bacadam · 2024-04-12T20:32:05Z

Okay I reverted my work on Add to AutoDJ buttons, so this PR is in a clean state.

daschuer

LGTM, thank you

daschuer · 2024-04-22T19:27:03Z

@ronso0: Ready for merge?

ronso0 · 2024-06-09T18:33:40Z

src/library/autodj/autodjprocessor.cpp

@@ -1665,6 +1669,20 @@ void AutoDJProcessor::playerRateChanged(DeckAttributes* pAttributes) {
    calculateTransition(fromDeck, getOtherDeck(fromDeck), false);
 }

+void AutoDJProcessor::playlistFirstTrackChanged() {
+    qDebug() << this << "playlistFirstTrackChanged";


please wrap this in if constexpr (sDebug) { like the other debug logs.

Done, thanks for your feedback.

daschuer · 2024-08-16T08:37:38Z

@ronso0 Is this now merge-able?

ronso0 · 2024-08-20T07:53:46Z

Yes, sorry for the delay, this looks good.
Thank you!

ronso0 · 2024-08-20T07:54:53Z

@daschuer do yo already have a changelog update ready, or shall I do one for this PR?

github-actions bot added library autodj labels Mar 3, 2024

ronso0 requested changes Mar 3, 2024

View reviewed changes

ronso0 linked an issue Mar 3, 2024 that may be closed by this pull request

Auto DJ: Synchronizing queue's top item and next deck #8956

Open

daschuer requested changes Mar 4, 2024

View reviewed changes

Bacadam force-pushed the fix_8956 branch from 990d501 to 73fea4b Compare March 5, 2024 10:50

github-actions bot added ui build code quality cmake controllers labels Mar 5, 2024

Bacadam changed the base branch from main to 2.4 March 5, 2024 10:56

JoergAtGithub removed ui build code quality cmake controllers labels Mar 12, 2024

Synchronize AutoDJ next deck with top track in queue

59ed03f

Bacadam force-pushed the fix_8956 branch from 26609db to 59ed03f Compare March 12, 2024 21:59

Remove calls to removeLoadedTrackFromTopOfQueue()

c1e7c22

Bacadam commented Mar 14, 2024

View reviewed changes

Synchronize also with "Add to AutoDJ" buttons in context menu

9e1eec4

Bacadam added 2 commits March 26, 2024 21:39

Remove unnecessary comments

d9fbb59

Revert "Synchronize also with "Add to AutoDJ" buttons in context menu"

9763555

This reverts commit 9e1eec4.

ronso0 added this to the 2.4.2 milestone Apr 13, 2024

daschuer approved these changes Apr 22, 2024

View reviewed changes

daschuer requested a review from ronso0 June 9, 2024 15:26

ronso0 requested changes Jun 9, 2024

View reviewed changes

Wrap qDebug call in a test of sDebug

7aaa7eb

Bacadam requested a review from ronso0 August 1, 2024 13:04

ronso0 merged commit 886efbe into mixxxdj:2.4 Aug 20, 2024
14 checks passed

ronso0 added the changelog This PR should be included in the changelog label Aug 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Synchronize AutoDJ next deck with top track in queue #12909

Synchronize AutoDJ next deck with top track in queue #12909

Bacadam commented Mar 3, 2024

JoergAtGithub commented Mar 3, 2024

ronso0 commented Mar 3, 2024

ronso0 left a comment

ronso0 Mar 3, 2024

daschuer Mar 4, 2024

Bacadam commented Mar 4, 2024

daschuer left a comment

Bacadam commented Mar 4, 2024 •

edited

Loading

Bacadam commented Mar 6, 2024

JoergAtGithub commented Mar 12, 2024

Bacadam commented Mar 12, 2024

ronso0 commented Mar 12, 2024

Bacadam commented Mar 12, 2024 •

edited

Loading

ronso0 commented Mar 12, 2024

Bacadam commented Mar 14, 2024

Bacadam Mar 14, 2024

ronso0 Mar 14, 2024

Bacadam commented Mar 14, 2024

Bacadam commented Mar 14, 2024

daschuer commented Mar 15, 2024

Bacadam commented Mar 26, 2024

Bacadam commented Apr 12, 2024

daschuer left a comment

daschuer commented Apr 22, 2024

ronso0 Jun 9, 2024

Bacadam Jun 18, 2024

daschuer commented Aug 16, 2024

ronso0 commented Aug 20, 2024

ronso0 commented Aug 20, 2024

Synchronize AutoDJ next deck with top track in queue #12909

Synchronize AutoDJ next deck with top track in queue #12909

Conversation

Bacadam commented Mar 3, 2024

JoergAtGithub commented Mar 3, 2024

ronso0 commented Mar 3, 2024

ronso0 left a comment

Choose a reason for hiding this comment

ronso0 Mar 3, 2024

Choose a reason for hiding this comment

daschuer Mar 4, 2024

Choose a reason for hiding this comment

Bacadam commented Mar 4, 2024

daschuer left a comment

Choose a reason for hiding this comment

Bacadam commented Mar 4, 2024 • edited Loading

Bacadam commented Mar 6, 2024

JoergAtGithub commented Mar 12, 2024

Bacadam commented Mar 12, 2024

ronso0 commented Mar 12, 2024

Bacadam commented Mar 12, 2024 • edited Loading

ronso0 commented Mar 12, 2024

Bacadam commented Mar 14, 2024

Bacadam Mar 14, 2024

Choose a reason for hiding this comment

ronso0 Mar 14, 2024

Choose a reason for hiding this comment

Bacadam commented Mar 14, 2024

Bacadam commented Mar 14, 2024

daschuer commented Mar 15, 2024

Bacadam commented Mar 26, 2024

Bacadam commented Apr 12, 2024

daschuer left a comment

Choose a reason for hiding this comment

daschuer commented Apr 22, 2024

ronso0 Jun 9, 2024

Choose a reason for hiding this comment

Bacadam Jun 18, 2024

Choose a reason for hiding this comment

daschuer commented Aug 16, 2024

ronso0 commented Aug 20, 2024

ronso0 commented Aug 20, 2024

Bacadam commented Mar 4, 2024 •

edited

Loading

Bacadam commented Mar 12, 2024 •

edited

Loading