CueControl: reject hotcue position writes when unset by gary-gdbsystems · Pull Request #16787 · mixxxdj/mixxx

gary-gdbsystems · 2026-07-29T07:07:28Z

Summary

hotcue_X_position was a plain writable ControlObject, so a script or controller could write to it even when no hotcue was set. The underlying cue never moved — CueControl::hotcuePositionChanged() already rejects a null cue — but the raw set() still stored a bogus value in the control, which was then reported back on read.

Approach

Route external writes through connectValueChangeRequest, mirroring the existing hotcue_X_color pattern:

The new slotHotcuePositionChangeRequest() rejects the request unless a hotcue is set (guards on the m_pCue pointer).
Otherwise it delegates to the existing validated move path (CueControl::hotcuePositionChanged), so moving an existing hotcue still works; the resulting cue change syncs the control back to the cue's actual (validated) position via loadCuesFromTrack().
Because the control is now confirm-required, the engine-side sync in HotcueControl::setPosition() uses setAndConfirm().

This supersedes the earlier stalled attempt in #13446. It intentionally does not call setReadOnly() (which, combined with a request handler, would log a spurious is read-only. Ignoring warning on every legitimate hotcue move) and guards on m_pCue rather than the control value (the old !m_hotcuePosition->get() check misfires at position 0).

Changes

src/engine/controls/cuecontrol.cpp — wire the change-request handler, add slotHotcuePositionChangeRequest(), use setAndConfirm() in setPosition().
src/engine/controls/cuecontrol.h — slot declaration.
src/test/hotcuecontrol_test.cpp — two tests: a write is rejected when no hotcue is set; a write moves a set hotcue.

Testing

HotcueControlTest (32 tests) and all cue/loop suites (113 tests) pass.
clang-format, clang-tidy, and the other pre-commit hooks are clean.

The hotcue_X_position control was a plain writable ControlObject, so a script or controller could write to it even when no hotcue was set. The underlying cue never moved (CueControl::hotcuePositionChanged rejects a null cue), but the raw set() still left a bogus value in the control, which was then reported back on read. Route external writes through connectValueChangeRequest, mirroring the existing hotcue_X_color pattern. The new handler rejects the request unless a hotcue is set, otherwise it delegates to the existing validated move path; the resulting cue change syncs the control back to the cue's actual position. Because the control is now confirm-required, engine-side updates use setAndConfirm(). Note: unlike the earlier approach in mixxxdj#13446, this does not call setReadOnly() (which would log a spurious "is read-only" warning on every legitimate move) and guards on the m_pCue pointer rather than the control value (which misfires at position 0). Resolves the request in mixxxdj#10409. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

cr7pt0gr4ph7

The core logic looks good, but should also be extended to cover hotcue end position.

cr7pt0gr4ph7 · 2026-08-10T17:47:33Z

+    // Use setAndConfirm() because m_hotcuePosition is confirm-required: a plain
+    // set() would be intercepted as a change request instead of updating the
+    // engine-authoritative value.
+    m_hotcuePosition->setAndConfirm(position.toEngineSamplePosMaybeInvalid());


The verbose comment can be removed:

Suggested change

// Use setAndConfirm() because m_hotcuePosition is confirm-required: a plain

// set() would be intercepted as a change request instead of updating the

// engine-authoritative value.

m_hotcuePosition->setAndConfirm(position.toEngineSamplePosMaybeInvalid());

m_hotcuePosition->setAndConfirm(position.toEngineSamplePosMaybeInvalid());

cr7pt0gr4ph7 · 2026-08-10T17:49:27Z

+    // The position may only be moved by dragging an existing hotcue, so route
+    // write requests through a handler that rejects them unless the hotcue is
+    // set. This also makes the control confirm-required, so engine-side updates
+    // must use setAndConfirm() (see HotcueControl::setPosition()).


The verbose comment can be removed:

Suggested change

// The position may only be moved by dragging an existing hotcue, so route

// write requests through a handler that rejects them unless the hotcue is

// set. This also makes the control confirm-required, so engine-side updates

// must use setAndConfirm() (see HotcueControl::setPosition()).

cr7pt0gr4ph7 · 2026-08-10T17:51:07Z

    m_hotcueEndPosition = std::make_unique<ControlObject>(
            keyForControl(QStringLiteral("endposition")));
    connect(m_hotcueEndPosition.get(),
            &ControlObject::valueChanged,
            this,
            &HotcueControl::slotHotcueEndPositionChanged,
            Qt::DirectConnection);


The hotcue end position should also reject writes when the hotcue is not set:

Suggested change

m_hotcueEndPosition = std::make_unique<ControlObject>(

keyForControl(QStringLiteral("endposition")));

connect(m_hotcueEndPosition.get(),

&ControlObject::valueChanged,

this,

&HotcueControl::slotHotcueEndPositionChanged,

Qt::DirectConnection);

m_hotcuePosition->connectValueChangeRequest(

this,

&HotcueControl::slotHotcueEndPositionChangeRequest,

Qt::DirectConnection);

cr7pt0gr4ph7 · 2026-08-10T17:52:34Z

+    // Reject the change if no hotcue is set: the position can only be moved by
+    // dragging an existing hotcue, not to create one (see issue #10409).


The overly verbose comment can be reduced to:

Suggested change

// Reject the change if no hotcue is set: the position can only be moved by

// dragging an existing hotcue, not to create one (see issue #10409).

// Reject the change if no hotcue is set

cr7pt0gr4ph7 · 2026-08-10T17:53:41Z

+}
+
 void HotcueControl::slotHotcueEndPositionChanged(double newEndPosition) {
    emit hotcueEndPositionChanged(this, newEndPosition);


The hotcue end position should also reject writes if the hotcue is not set (and maybe also if the current hotcue type doesn't support an end position?):

Suggested change

emit hotcueEndPositionChanged(this, newEndPosition);

// Reject the change if no hotcue is set

if (!m_pCue) {

return;

}

emit hotcueEndPositionChanged(this, newEndPosition);

cr7pt0gr4ph7 · 2026-08-10T17:54:15Z

@@ -138,6 +138,7 @@ class HotcueControl : public QObject {
    void slotHotcueSwap(double v);
    void slotHotcueEndPositionChanged(double newPosition);


The hotcue end position should also reject change requests:

Suggested change

void slotHotcueEndPositionChanged(double newPosition);

void slotHotcueEndPositionChanged(double newPosition);

void slotHotcueEndPositionChangeRequest(double newPosition);

cr7pt0gr4ph7 · 2026-08-10T17:54:55Z

                         .isValid());
 }

+TEST_F(HotcueControlTest, PositionChangeRequestRejectedWhenUnset) {


This should be extended to also test that the hotcue end position rejects changes when not set.

cr7pt0gr4ph7 · 2026-08-10T17:56:08Z

Suggested change

m_hotcueEndPosition->setAndConfirm(endPosition.toEngineSamplePosMaybeInvalid());

Address review feedback on the hotcue-position guard: apply the same confirm-required + reject-when-unset treatment to hotcue_endposition, so writing it with no hotcue set no longer stores a bogus value. Route endposition writes through slotHotcueEndPositionChangeRequest (rejects when m_pCue is null), and use setAndConfirm() in setEndPosition() since the control is now confirm-required. Also trim the verbose comments on the position path per review.

gary-gdbsystems · 2026-08-11T21:08:40Z

Thanks. Extended the reject-when-unset guard to hotcue_endposition (same as position — rejects when no hotcue is set, and setEndPosition() now uses setAndConfirm()), and trimmed the comments.

Only saved loops have an end position, so I can also reject endposition writes for non-loop hotcues in a follow-up if preferred. I kept a one-line note explaining the setAndConfirm(); can remove it if you'd rather.

cr7pt0gr4ph7 · 2026-08-13T14:11:59Z

Only saved loops have an end position, so I can also reject endposition writes for non-loop hotcues in a follow-up if preferred. I kept a one-line note explaining the setAndConfirm(); can remove it if you'd rather.

Just for completeness: Jump hotcues also have an endposition, which is used to store the position that is being jumped to.

github-actions Bot added engine code quality labels Jul 29, 2026

ronso0 added the AI The issue or PR makes heavy use of AI, which requires extra attention and effort to review label Jul 29, 2026

cr7pt0gr4ph7 suggested changes Aug 10, 2026

View reviewed changes

Navigation Menu

Uh oh!

Uh oh!

CueControl: reject hotcue position writes when unset - #16787

CueControl: reject hotcue position writes when unset#16787
gary-gdbsystems wants to merge 2 commits into
mixxxdj:mainfrom
gary-gdbsystems:hotcue-position-readonly

gary-gdbsystems commented Jul 29, 2026 •

edited

Loading

Uh oh!

cr7pt0gr4ph7 left a comment

Uh oh!

cr7pt0gr4ph7 Aug 10, 2026

Uh oh!

cr7pt0gr4ph7 Aug 10, 2026

Uh oh!

cr7pt0gr4ph7 Aug 10, 2026

Uh oh!

cr7pt0gr4ph7 Aug 10, 2026

Uh oh!

cr7pt0gr4ph7 Aug 10, 2026

Uh oh!

cr7pt0gr4ph7 Aug 10, 2026

Uh oh!

cr7pt0gr4ph7 Aug 10, 2026

Uh oh!

cr7pt0gr4ph7 Aug 10, 2026

Uh oh!

gary-gdbsystems commented Aug 11, 2026

Uh oh!

cr7pt0gr4ph7 commented Aug 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		// Reject the change if no hotcue is set: the position can only be moved by
		// dragging an existing hotcue, not to create one (see issue #10409).

		@@ -138,6 +138,7 @@ class HotcueControl : public QObject {
		void slotHotcueSwap(double v);
		void slotHotcueEndPositionChanged(double newPosition);

	void slotHotcueEndPositionChanged(double newPosition);
	void slotHotcueEndPositionChanged(double newPosition);
	void slotHotcueEndPositionChangeRequest(double newPosition);


	m_hotcueEndPosition->setAndConfirm(endPosition.toEngineSamplePosMaybeInvalid());

Uh oh!

Uh oh!

Conversation

gary-gdbsystems commented Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Approach

Changes

Testing

Uh oh!

cr7pt0gr4ph7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gary-gdbsystems commented Aug 11, 2026

Uh oh!

cr7pt0gr4ph7 commented Aug 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gary-gdbsystems commented Jul 29, 2026 •

edited

Loading