Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi Preview Fix #3382

Merged
merged 21 commits into from
Jan 5, 2021
Merged

Conversation

daschuer
Copy link
Member

This fixes an issue when you preview more than one cues at a time.
In 2.3 the last released button moves the playposition to the associated hot cue position.
This can't no longer work with cue loops involved.

I have changed the code that internally only one hot cue can be previewing.
If you preview one hot-cue and press a second one, the second takes over. The release of the first hot cue is ignored from that point. If it was a loop, looping is disabled.

This also cleans up looking.

@uklotzde
Copy link
Contributor

You might consider to pick and apply this commit: uklotzde@4c2d90f

I didn't check if it still applies here and what changes are required. The idea is simple, just keep the code that belongs together at a single place.

@daschuer
Copy link
Member Author

Done.

@daschuer daschuer added this to the 2.4.0 milestone Dec 1, 2020
Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some minor coding comments. Overall the code seems to be much more readable than before.

I didn't check if everything works as expected. The cue handling is complicated, I have to trust the tests.

@@ -30,6 +30,8 @@ constexpr double CUE_MODE_CUP = 5.0;
/// This is the position of a fresh loaded tack without any seek
constexpr double kDefaultLoadPosition = 0.0;
constexpr int kNoHotCueNumber = 0;
/// Used for a common tarcking of the previewing Hotcue in m_currentlyPreviewingIndex
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

@@ -416,10 +416,16 @@ void CueControl::detachCue(HotcueControl* pControl) {
pControl->resetCue();
}

// This is called from the EngineWokerThread and ends with the initial seek v
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo and left over character

@@ -617,6 +596,14 @@ void CueControl::loadCuesFromTrack() {
}
}

// Detach all hotcues that are no longer present
for (int hotCue = 0; hotCue < m_iNumHotCues; ++hotCue) {
if (!active_hotcues.contains(hotCue)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix the variable naming along the way?

// Seek to cue point
double cuePoint = m_pCuePoint->get();

// Note: We do not mess with ply here, we continue playing or previewing.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

@@ -1640,14 +1519,20 @@ void CueControl::introEndSet(double value) {
TrackPointer pLoadedTrack = m_pLoadedTrack;
lock.unlock();

// Update Track's cue.
// CO's are update loadCuesFromTrack()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing "in" (multiple times)

}
void setPreviewingPosition(double position) {
m_previewingPosition = position;
void storePreviewingActivateData() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of the function does not tell me what it is actually doing. What is "activate data"?

src/engine/controls/cuecontrol.h Show resolved Hide resolved
pCue->setStartPosition(startPosition);
if (endPosition == Cue::kNoPosition) {
pCue->setType(mixxx::CueType::HotCue);
if (!pCue) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (pCue) { update } else { create }

@@ -327,11 +327,14 @@ TrackPointer BaseTrackPlayerImpl::unloadTrack() {
}
}
if (!pLoopCue) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (pLoopCue) { update } else { create }

mixxx::CueType type,
int hotCueIndex,
double sampleStartPosition,
double sampleEndPosition) {
QMutexLocker lock(&m_qMutex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the lock down to the point when it is actually needed. We don't need to protect the global system allocator or the Qt connect functions.

@Be-ing
Copy link
Contributor

Be-ing commented Dec 10, 2020

To clarify, this fixes a regression in the main branch, correct? If so, it's not relevant for 2.3.

@daschuer
Copy link
Member Author

Yes, merge target main is correct.

@daschuer
Copy link
Member Author

Done

@daschuer
Copy link
Member Author

daschuer commented Jan 5, 2021

@uklotzde Is this OK now?

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, didn't notice. LGTM

@uklotzde uklotzde merged commit f083c4f into mixxxdj:main Jan 5, 2021
@daschuer daschuer deleted the hotcue_focus_off_by_one_main branch September 26, 2021 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants