AutoDJ: handle "enabled" CO change requests like clicks on the GUI button #11495

ronso0 · 2023-04-18T12:25:25Z

otherwise, if AutoDJ refuses to start because of unmet conditions, no error messages are shown and the CO remains in 'enabled' state.

Fixes #11494

daschuer · 2023-04-18T19:07:33Z

I experience a stack overflow during due a recursive loop during testing:

#57 AutoDJProcessor::toggleAutoDJ(bool) (this=0x0, enable=<optimized out>)
    at ../../src/library/autodj/autodjprocessor.cpp:432
#58 0x0000555555dfed91 in DlgAutoDJ::toggleAutoDJButton(bool)
    (this=0x55556a42c4a0, enable=<optimized out>)
    at ../../src/library/autodj/dlgautodj.cpp:279
#59 0x00007ffff3cbf328 in QtPrivate::QSlotObjectBase::call(QObject*, void**)
    (a=0x7fffff801a70, r=0x55556a42c4a0, this=0x55556a4ba1e0)
    at ../../include/QtCore/../../src/corelib/kernel/qobjectdefs_impl.h:394
#60 QMetaObject::activate(QObject*, int, int, void**)
    (sender=0x55555b644550, signalOffset=<optimized out>, local_signal_index=<optimized out>, argv=<optimized out>) at kernel/qobject.cpp:3784
#61 0x0000555555df2086 in AutoDJProcessor::autoDJStateChangeRequest(bool)
    (this=<optimized out>, _t1=<optimized out>)
    at mixxx-lib_autogen/include/moc_autodjprocessor.cpp:805
#62 0x00007ffff3cbf328 in QtPrivate::QSlotObjectBase::call(QObject*, void**)
    (a=0x7fffff801ba0, r=0x55555b644550, this=0x55555b63e950)
    at ../../include/QtCore/../../src/corelib/kernel/qobjectdefs_impl.h:394
#63 QMetaObject::activate(QObject*, int, int, void**)
    (sender=0x55555b6682d0, signalOffset=<optimized out>, local_signal_index=<optimized out>, argv=<optimized out>) at kernel/qobject.cpp:3784
#64 0x00005555559a5d67 in ControlDoublePrivate::valueChangeRequest(double)
    (this=this@entry=0x55555b6682d0, _t1=<optimized out>)
--Type <RET> for more, q to quit, c to continue without paging--
    at mixxx-lib_autogen/include/moc_control.cpp:151
#65 0x00005555559a6658 in ControlDoublePrivate::set(double, QObject*)
    (this=0x55555b6682d0, value=<optimized out>, pSender=0x55555b63f4c0)
    at ../../src/control/control.cpp:208
#66 0x0000555555df6b73 in ControlObject::set(double)
    (value=1, this=<optimized out>) at ../../src/control/controlobject.h:83
#67 AutoDJProcessor::toggleAutoDJ(bool) (this=0x0, enable=<optimized out>)
    at ../../src/library/autodj/autodjprocessor.cpp:432

daschuer · 2023-04-18T19:42:57Z

The loop can be broken by m_pEnabledAutoDJ->setAndConfirm() inside AutoDJProcessor::toggleAutoDJ() maybe we also need to adjust the checks of the m_pEnabledAutoDJ->get(), because it returns now the confirmed value and not like before unconfirmed value.

daschuer

.

ronso0 · 2023-04-18T20:40:31Z

Yep, I agree. I simply forgot to change set to setAndConfirm.

ronso0 · 2023-04-19T00:09:34Z

The previous fix / hack with the signal sent to DlgAutoDJ and back to AutoDJPrcessor felt too much like a ahck so I tried to fix the root cause by emitting an error code signal (instead of return values).

ronso0 · 2023-04-19T00:13:03Z

src/library/autodj/autodjprocessor.cpp

-                m_pEnabledAutoDJ->set(0.0);
-            }
+            qDebug() << "Queue is empty now, disable Auto DJ";
+            m_pEnabledAutoDJ->setAndConfirm(0.0);


I think we don't need these in the enable anymore.
Didn't look close enough, yet, to be sure.

src/library/autodj/autodjprocessor.cpp

ronso0 · 2023-04-19T00:14:58Z

src/library/autodj/dlgautodj.cpp

+
+// TODO If there's a way to migrate the translations move this
+// to AutoDJProcessor in order to keep this class minimal
+void DlgAutoDJ::autoDJError(AutoDJProcessor::AutoDJError error) {


I have no experience with Transifex: is it smart enough to keep the existing translations?

Yes, as long as the original string and the name of the surrounding QObject is not changed.

ronso0 · 2023-04-19T00:21:34Z

src/library/autodj/dlgautodj.cpp

    switch (error) {
    case AutoDJProcessor::ADJ_NOT_TWO_DECKS:
        QMessageBox::warning(nullptr,
                tr("Auto DJ"),
                tr("Auto DJ requires two decks assigned to opposite sides of the crossfader."),
                QMessageBox::Ok);
-        // Make sure the button becomes unpushed.
-        pushButtonAutoDJ->setChecked(false);


here and below: obsolete, since AutoDJProcessor::autoDJStateChanged signal >> DlgAutoDJ::autoDJStateChanged updates the GUI

ronso0 · 2023-04-19T13:13:10Z

In the first run I didn't build the tests and didn't notice they require the return value.

This is now unchanged and the error codes are emitted additionally after the new/unchanged AutoDJ state is emitted to update the GUI. The separate error handling also allows to show error messages in some yet uncovered situations.

This allows to handle both clicks on the Enable GUI button and 'enabled' CO's change requests in the same place. Also, there's now a separate slot in DlgAutoDJ that receive error codes and pop up respective messages, which means any AutoDJProcessor method can warn the user.

daschuer · 2023-05-02T22:12:22Z

src/library/autodj/autodjprocessor.cpp

@@ -625,7 +629,11 @@ void AutoDJProcessor::crossfaderChanged(double value) {
                    }
                    pToDeck->play();
                } else {
+                    // TODO Is this also only possible if the user changed the
+                    // deck orientation?


This happens if the track in the To Deck has been ejected or shorter than 200 ms.

Okay, I understood the above condition, and it seems 'eject' and a manually loaded, very short track could be the only cause, since IIUC tracks shorter than kMinimumTrackDurationSec are skipped

mixxx/src/library/autodj/autodjprocessor.cpp

Lines 1223 to 1228 in 0d5b4a7

VERIFY_OR_DEBUG_ASSERT(toDeckDuration >= kMinimumTrackDurationSec) {

// Track has no duration or too short. This should not happen, because short

// tracks are skipped after load.

loadNextTrackFromQueue(*pToDeck, false);

return;

}

If AutoDJ stops unexpectedly a popup would be helpful to notify users.
But if that happens after user interaction (eject) we don't need it IMO

daschuer

LGTM and works good.
Can you please remove the question in the comment, once it is clarified?

ronso0 added this to the 2.3.5 milestone Apr 18, 2023

github-actions bot added the library label Apr 18, 2023

ronso0 linked an issue Apr 18, 2023 that may be closed by this pull request

AutoDJ shortcut (SHIFT+F12) and "symbol" turns on, but feature doesn't turn on due to "two decks playing at the same time." #11494

Closed

daschuer requested changes Apr 18, 2023

View reviewed changes

ronso0 force-pushed the autodj-refuse-start-hotkey branch from 7827453 to c2e715c Compare April 19, 2023 00:07

ronso0 commented Apr 19, 2023

View reviewed changes

src/library/autodj/autodjprocessor.cpp Show resolved Hide resolved

ronso0 commented Apr 19, 2023

View reviewed changes

ronso0 force-pushed the autodj-refuse-start-hotkey branch from c2e715c to d717db0 Compare April 19, 2023 00:16

ronso0 commented Apr 19, 2023

View reviewed changes

ronso0 force-pushed the autodj-refuse-start-hotkey branch from d717db0 to 7f50877 Compare April 19, 2023 12:56

ronso0 force-pushed the autodj-refuse-start-hotkey branch from 7f50877 to d9f9f68 Compare April 19, 2023 20:19

daschuer reviewed May 2, 2023

View reviewed changes

daschuer requested changes May 2, 2023

View reviewed changes

ronso0 added 2 commits May 3, 2023 01:07

AutoDJ: show error message when deck orientaton requirements are unmet

dc3bede

AutoDJ feature: remove redundant GUI updates

216a080

ronso0 force-pushed the autodj-refuse-start-hotkey branch from d9f9f68 to 216a080 Compare May 2, 2023 23:07

daschuer merged commit 3a87ad2 into mixxxdj:2.3 May 3, 2023
8 checks passed

ronso0 deleted the autodj-refuse-start-hotkey branch May 3, 2023 06:20

ronso0 mentioned this pull request Sep 11, 2023

Auto DJ Icon not consistent #11954

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AutoDJ: handle "enabled" CO change requests like clicks on the GUI button #11495

AutoDJ: handle "enabled" CO change requests like clicks on the GUI button #11495

ronso0 commented Apr 18, 2023

daschuer commented Apr 18, 2023

daschuer commented Apr 18, 2023

daschuer left a comment

ronso0 commented Apr 18, 2023

ronso0 commented Apr 19, 2023

ronso0 Apr 19, 2023

ronso0 Apr 19, 2023

daschuer Apr 19, 2023

ronso0 Apr 19, 2023

ronso0 commented Apr 19, 2023

daschuer May 2, 2023

ronso0 May 2, 2023

daschuer left a comment

	VERIFY_OR_DEBUG_ASSERT(toDeckDuration >= kMinimumTrackDurationSec) {
	// Track has no duration or too short. This should not happen, because short
	// tracks are skipped after load.
	loadNextTrackFromQueue(*pToDeck, false);
	return;
	}

AutoDJ: handle "enabled" CO change requests like clicks on the GUI button #11495

AutoDJ: handle "enabled" CO change requests like clicks on the GUI button #11495

Conversation

ronso0 commented Apr 18, 2023

daschuer commented Apr 18, 2023

daschuer commented Apr 18, 2023

daschuer left a comment

Choose a reason for hiding this comment

ronso0 commented Apr 18, 2023

ronso0 commented Apr 19, 2023

ronso0 Apr 19, 2023

Choose a reason for hiding this comment

ronso0 Apr 19, 2023

Choose a reason for hiding this comment

daschuer Apr 19, 2023

Choose a reason for hiding this comment

ronso0 Apr 19, 2023

Choose a reason for hiding this comment

ronso0 commented Apr 19, 2023

daschuer May 2, 2023

Choose a reason for hiding this comment

ronso0 May 2, 2023

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment