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

engine/controls: Remove superfluous notifySeek implementations #3032

Merged
merged 1 commit into from
Jul 30, 2021

Conversation

xeruf
Copy link
Contributor

@xeruf xeruf commented Aug 19, 2020

Most of the time this isn't needed, because the process method is called afterwards and sets the new position anyways.

Hand-checked them all, not sure about the scratching yet though.

Tests haven't finished yet.

@xeruf xeruf force-pushed the enginecontrol-seek branch from 5299a8c to 7d462c4 Compare August 19, 2020 10:44
@xeruf
Copy link
Contributor Author

xeruf commented Aug 19, 2020

Okay, so the Sync seek test is failing. Unfortunately I don't know enough here - I thought since the updated position is already processed in postProcess, it doesn't need to be set separately through notifySeek.
But maybe this is merely an insignificant difference depending on implementation details?

[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from EngineSyncTest
[ RUN      ] EngineSyncTest.UserTweakPreservedInSeek
/home/janek/daten/projects/mixxx/mixxx/src/test/enginesynctest.cpp:1915: Failure
Expected equality of these values:
  0.2269025
  ControlObject::get(ConfigKey(m_sGroup1, "playposition"))
    Which is: 0.22701859
/home/janek/daten/projects/mixxx/mixxx/src/test/enginesynctest.cpp:1918: Failure
Expected equality of these values:
  0.12403101
  ControlObject::get(ConfigKey(m_sGroup1, "beat_distance"))
    Which is: 0.12527132

Btw, why I am doing this: I probably need to add a parameter to notifySeek for hotcue jumps to record macros, so I wanted to reduce the affected calls.

@Be-ing Be-ing marked this pull request as draft September 1, 2020 02:12
@@ -927,11 +927,6 @@ void BpmControl::slotUpdateRateSlider(double value) {
m_pRateRatio->set(dRateRatio);
}

void BpmControl::notifySeek(double dNewPlaypos) {
EngineControl::notifySeek(dNewPlaypos);
updateBeatDistance();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is never called anymore. Could explain the failing test.

@Be-ing Be-ing changed the base branch from master to main October 23, 2020 23:12
@xeruf xeruf marked this pull request as ready for review December 30, 2020 12:16
@xeruf xeruf force-pushed the enginecontrol-seek branch from ba26d50 to 31cf70d Compare December 30, 2020 12:24
@xeruf xeruf requested a review from Holzhaus December 30, 2020 12:51
@github-actions
Copy link

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Mar 31, 2021
@xeruf
Copy link
Contributor Author

xeruf commented Apr 6, 2021

can someone rerun the checks? The logs are expired and I'm not sure anymore whether the failing test was fixed.

@Holzhaus
Copy link
Member

Holzhaus commented Apr 6, 2021

Please pull in main, restarting the workflow isn't possible because it's too old.

@github-actions github-actions bot removed the stale Stale issues that haven't been updated for a long time. label Apr 7, 2021
@xeruf xeruf force-pushed the enginecontrol-seek branch from 31cf70d to 35386f9 Compare July 5, 2021 17:10
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, thank you. All tests still pass, so these notifySeek implementations may indeed by superfluous. @daschuer do you want to take another look?

@Holzhaus Holzhaus requested review from daschuer and ywwg July 29, 2021 14:47
@ywwg
Copy link
Member

ywwg commented Jul 29, 2021

If the tests pass then I'm ok with merging. We have a lot of tests for various seek conditions so it should be sufficiently covered. If we find a missing case, then oh well :)

@Holzhaus Holzhaus merged commit d928aac into mixxxdj:main Jul 30, 2021
@xeruf xeruf deleted the enginecontrol-seek branch November 16, 2021 16:01
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.

3 participants