LoopingControl: Handle invalid start position properly (lp1942715) #4266
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
An invalid start position resets the loop anyway. See this for details: https://bugs.launchpad.net/mixxx/+bug/1942715
uklotzde
reviewed
Sep 6, 2021
| @@ -965,7 +965,8 @@ void LoopingControl::slotLoopStartPos(double positionSamples) { | |||
| loopInfo.startPosition = position; | |||
| m_pCOLoopStartPosition->set(position.toEngineSamplePosMaybeInvalid()); | |||
|
|
|||
| if (loopInfo.endPosition.isValid() && loopInfo.endPosition <= loopInfo.startPosition) { | |||
| if (loopInfo.startPosition.isValid() && loopInfo.endPosition.isValid() && | |||
There was a problem hiding this comment.
We already checked for position.isValid() above. I don't see a reason why we cannot combine these code paths like if (!loopInfo.startPosition.isValid() || !loopInfo.endPosition().isValid() || loopInfo.endPosition <= loopInfo.startPosition) ...?
uklotzde
reviewed
Sep 6, 2021
| emit loopReset(); | ||
| loopInfo.endPosition = mixxx::audio::kInvalidFramePos; | ||
| m_pCOLoopEndPosition->set(kNoTrigger); | ||
| setLoopingEnabled(false); | ||
| } | ||
|
|
||
| loopInfo.seekMode = LoopSeekMode::MovedOut; |
There was a problem hiding this comment.
Let's move these lines up, closer to where loopInfo is initialized.
Then use loopInfo.startPosition instead of position for consistency and to reveal the dependencies. For me this is easier to follow. Less variables to remember, more concise.
Compile time enforcement: Use a local scope so that the temporary position becomes unavailable for the remainder of the function body.
…Pos()` This sets loopInfo.startPosition first and then uses it instead of `position` for consistency and to reveal the dependencies. Also usees a local scope so that the temporary `position` becomes unavailable for the remainder of the function body.
uklotzde
approved these changes
Sep 6, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
An invalid start position resets the loop anyway.
See this for details: https://bugs.launchpad.net/mixxx/+bug/1942715