-
-
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
Remove Beats::findNthBeat fuzzy behavior #4099
Conversation
6929e13
to
a177036
Compare
a177036
to
95f7d36
Compare
Pull Request Test Coverage Report for Build 1062636948
💛 - Coveralls |
This resolves #3418 btw. |
This would not be the first time that some weird logic in Mixxx turned out to be totally useless. |
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.
one small nit for clarity
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.
thanks!
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
BeatmapTest.TestBpmAround
fails for unknown reasons. Either the test is wrong, or our test coverage of thefindNthBeat
method is insufficient. I'll have to check later on.EDIT: Works now.
I only removed two tests that explicitly tested this weird behavior. All other tests passed, so if this fuzzy behavior was indeed necessary for some feature, it should have been mentioned in a comment or required by a test case.
I suggest to merge this, unless we find major issues. Maybe we need it for beatjumps/looping, but in that case we should request snapping explicitly before called
findNthBeat
. I wrote a small convenience method that could be used for this.