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

Add and use mixxx::Bpm::valueOr() #4168

Merged
merged 3 commits into from
Aug 5, 2021
Merged

Add and use mixxx::Bpm::valueOr() #4168

merged 3 commits into from
Aug 5, 2021

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Aug 1, 2021

Follow-up of #4165 to resolve some FIXME comments.

@uklotzde uklotzde added this to the 2.4.0 milestone Aug 1, 2021
@uklotzde uklotzde requested a review from Holzhaus August 1, 2021 13:00
@coveralls
Copy link

coveralls commented Aug 1, 2021

Pull Request Test Coverage Report for Build 1101432505

  • 0 of 21 (0.0%) changed or added relevant lines in 2 files are covered.
  • 80 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.003%) to 26.014%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/library/basetracktablemodel.cpp 0 3 0.0%
src/library/dlgtrackinfo.cpp 0 18 0.0%
Files with Coverage Reduction New Missed Lines %
src/library/dlgtrackinfo.cpp 1 0%
src/controllers/scripting/controllerscriptenginebase.cpp 36 40.19%
src/util/cmdlineargs.cpp 43 72.73%
Totals Coverage Status
Change from base Build 1087078133: 0.003%
Covered Lines: 20008
Relevant Lines: 76913

💛 - Coveralls

@mr-smidge
Copy link
Contributor

FWIW, I tried performing an "Entire music library" export to Engine Prime using this branch, and it completed successfully. The previous codebase would occasionally trigger assertion failures in Bpm::value() for such an operation if there were tracks where analysis was not valid.

So gets my vote 👍.

@Holzhaus
Copy link
Member

Holzhaus commented Aug 5, 2021

I'm open to it, but the resulting double is not undefined, it's zero. Should it be valueOrZero? Unsure about that.

@uklotzde
Copy link
Contributor Author

uklotzde commented Aug 5, 2021

I'm open to it, but the resulting double is not undefined, it's zero. Should it be valueOrZero? Unsure about that.

Good point! On the value level there is no undefined, which would otherwise be NaN.

@uklotzde uklotzde changed the title Add and use mixxx::Bpm::valueOrUndefined() Add and use mixxx::Bpm::valueOr() Aug 5, 2021
@uklotzde
Copy link
Contributor Author

uklotzde commented Aug 5, 2021

An optional that wraps always valid BPM values would be more idiomatic. A value of 0.0 is valid for some rare, ambient, beatless tracks. Let's defer this until we rewrite in Rust ;)

@Holzhaus
Copy link
Member

Holzhaus commented Aug 5, 2021

Thanks, looks good.

@Holzhaus Holzhaus merged commit 83346e8 into mixxxdj:main Aug 5, 2021
@uklotzde uklotzde deleted the bpm branch August 5, 2021 21:55
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.

4 participants