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.

Sign up for GitHub

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

Jump to bottom

EngineBuffer: Use mixxx::audio::FramePos #4217

Merged
merged 3 commits into from
Sep 8, 2021
Merged

EngineBuffer: Use mixxx::audio::FramePos #4217

merged 3 commits into from
Sep 8, 2021

Conversation

Holzhaus
Copy link
Member

Based on #4191.

@coveralls
Copy link

coveralls commented Aug 17, 2021 •
edited
Loading

Pull Request Test Coverage Report for Build 1146995695

  • 63 of 74 (85.14%) changed or added relevant lines in 5 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.006%) to 25.961%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/engine/enginebuffer.cpp 52 63 82.54%
Files with Coverage Reduction New Missed Lines %
src/engine/controls/cuecontrol.cpp 1 68.73%
Totals Coverage Status
Change from base Build 1146568621: 0.006%
Covered Lines: 20003
Relevant Lines: 77049
💛 - Coveralls

@Holzhaus Holzhaus force-pushed the enginebuffer-framepos branch 2 times, most recently from 072b55a to 46887e3 Compare August 19, 2021 12:46
@Be-ing
Copy link
Contributor

Be-ing commented Aug 24, 2021 

 The following tests FAILED:
	284 - EngineSyncTest.BeatMapQuantizePlay (Failed)

@Holzhaus
Copy link
Member Author

Yes, I saw that but didn't find an obvious error when reading the diff. Didn't investigate it yet.

@Holzhaus Holzhaus force-pushed the enginebuffer-framepos branch from 46887e3 to fb6d926 Compare September 1, 2021 14:37
@Holzhaus
Copy link
Member Author

Holzhaus commented Sep 1, 2021

I have no idea why it works on all OSes except Windows. Can some windows user have a look at this?

@Holzhaus Holzhaus force-pushed the enginebuffer-framepos branch from fb6d926 to 9c2580b Compare September 2, 2021 12:06
@Holzhaus
EngineBuffer: Use mixxx::audio::FramePos for seeking
a0dc7c8 
Also, the `slotControlSeekAbs`/`slotControlSeekExact` method names were
misleading because they weren't actually connected to any signal or
control. Hence, these were renamed and also made regular methods, not
slots.
@Holzhaus
EngineBuffer: Use mixxx::audio::FramePos internally
1d9490d
@Holzhaus Holzhaus force-pushed the enginebuffer-framepos branch from 9c2580b to 1d9490d Compare September 7, 2021 11:30
@Holzhaus Holzhaus marked this pull request as ready for review September 7, 2021 12:30
@Holzhaus
Copy link
Member Author

Holzhaus commented Sep 7, 2021

Looks like the issue fixed itself by rebasing on latest main. Ready to review.

uklotzde
uklotzde suggested changes Sep 7, 2021
View reviewed changes
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.

One minor finding.

src/engine/enginebuffer.cpp Outdated Show resolved Hide resolved
uklotzde
uklotzde approved these changes Sep 8, 2021
View reviewed changes
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.

Thank you once again for this tedious refactoring! LGTM

@uklotzde uklotzde merged commit a0ff12b into mixxxdj:main Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@uklotzde uklotzde uklotzde approved these changes

Assignees
No one assigned
Labels
code quality
Projects
Semanting Type Refactoring
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Holzhaus @coveralls @Be-ing @uklotzde