-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Use mixxx::Bpm for some Beats functions Pt. 2 #4045
Conversation
11d316f
to
90e68fc
Compare
I rebased on latest main. The added validity checks from #4044 uncovered two more code locations where the bpm value may be invalid, in EngineBuffer and SyncControl. To keep the changes small and leave the business logic unchanged, I added FIXMEs and just used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments
src/engine/sync/synccontrol.cpp
Outdated
@@ -516,7 +516,7 @@ void SyncControl::notifySeek(double dNewPlaypos) { | |||
double SyncControl::fileBpm() const { | |||
mixxx::BeatsPointer pBeats = m_pBeats; | |||
if (pBeats) { | |||
return pBeats->getBpm(); | |||
return pBeats->getBpm().getValue(); | |||
} | |||
return 0.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return mixxx::Bpm::kValueUndefined instead of 0.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/track/beatmap.cpp
Outdated
if (!isValid()) { | ||
return -1; | ||
return mixxx::Bpm(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either return mixxx::Bpm()
or {}
but not a mixture of both in various places. I would prefer the latter, because we don't need to repeat the return type multiple times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! LGTM
Follow up to #4044.