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

Fix LP1855321 - Prevent jumping to main cue #2593

Merged

Holzhaus merged 6 commits into mixxxdj:master from goddisignz:LP1855321_zero_playposition_jump_to_cue

Apr 9, 2020

Contributor

goddisignz commented Mar 23, 2020 •

edited by daschuer

Loading

This PR fixes https://bugs.launchpad.net/mixxx/+bug/1855321.

The code I removed (or its functionality) should only be executed when a track is loaded and not when its hot cues or other cue points are updated. Even if we are at playposition 0:00 and change a cue point this is not equivalent to a track load.

Checked if jumping to main cue, intro start, and track start still work as before -> yes they do.

Contributor Author

goddisignz commented Mar 24, 2020

Apparantly the checks have a different notion of what should happen and when. I will look at that again.

Contributor Author

goddisignz commented Mar 24, 2020

So I now adapted the tests. It may seem lazy to just remove the code that produces the failing tests but I really do not see the use case here.
After the track is loaded and we already jumped to the main cue we can not set it to a different position without already being there, or am I wrong?
Even if we could, do we really want to follow the main cue? If so, I can implement the special case which only reacts on moving the main cue but ignores setting others.

Contributor

ninomp commented Mar 24, 2020

Hello! I am the person who introduced this behaviour. My use case was loading a track that has not been analyzed before. In that case, cue point is placed (or rather moved from 0:00.000) when analysis is finished, which is some time after the track is loaded. That's why I implemented "cue point following" (that is how I call this feature).

Member

daschuer commented Mar 24, 2020

It was introduced here: #1242

daschuer requested changes

View reviewed changes

Member

daschuer left a comment

It looks like we actually need to fix that code.
Would you mind to reset your branch to master and implement the change in a new commit?
Thank you .

src/engine/controls/cuecontrol.cpp Show resolved Hide resolved

Contributor

Be-ing commented Mar 24, 2020

I propose we target this to the 2.3 milestone. I don't think it should block the beta release though.

daschuer added this to the 2.3.0 milestone

daschuer added the minor bug label

Member

ronso0 commented Mar 24, 2020

I don't think it should block the beta release though.

I disagree. This bug is a major regression and a real annoyance in a basic, regular cue/hotcue workflow.
We should consider that the beta gets some attension also among reviewers, and having this bug in a beta gives my a bad feeling...

Member

ronso0 commented Mar 24, 2020

...somehow at the same level as unreliable Cue button feedback (although I don't experience since I use a no-blinking mode)

Member

ywwg commented Mar 25, 2020

Can you add a specific test for the procedure that's listed in the original bug? It should be pretty easy to do that with code and confirm that the playhead doesn't jump to the wrong places.

Member

ywwg commented Mar 25, 2020

Hi @ronso0 -- designating a PR as a "non-blocker" simply means that we won't hold up the freeze or final release of 2.3 based on this specific bug. Because this bug is relatively small and specific, it will probably be fixed well before 2.3 is finally released.

Member

ronso0 commented Mar 25, 2020

It's small & specific, maybe it's easy, can't telll, but it's not fixed, yet... ;)
Am I right with my assumptions about the long anticipated beta release & web reviewers and their judgments, and how that might affect if newbies try Mixxx or not?

Member

ywwg commented Mar 25, 2020

how that might affect if newbies try Mixxx or not?

it doesn't make a big difference, really. We could hypothesize a user that encounters this exact bug and decides mixxx sucks, but honestly that's unlikely. People usually run in to issues with:

1: setting up controllers
2: setting up sound hardware
3: setting up waveforms

before they even get to a point with an obscure / annoying hotcue bug.

We still get over a million downloads a year, and there's no other open source alternative, so Mixxx's long term future is safe :)


          Fix LP1855321 - Prevent undesired jumping to main cue

e07af9e

Contributor Author

goddisignz commented Mar 25, 2020

After the discussion/clarification of the first PR, an update:

When a track is loaded and already analyzed, jump to the correct entry point according to settings.
When the track has to be analyzed, jump to the updated entry point after analysis is complete.
If the play position is currently at the main cue and the main cue is moved due to en-/disabling quantization, follow it.
The same holds true for track intro.

Member

daschuer commented Mar 25, 2020 •

edited

Loading

When the track has to be analyzed, jump to the updated entry point after analysis is complete

... If the track is not playing or was manual cued in the meantime.

The rest sounds good.

Contributor Author

goddisignz commented Mar 25, 2020 •

edited

Loading

Good Remark...
The manual cueing is apparently already handled in the analyzersilence class. Only if the main cue was at 0.0, its position is updated.


          do not follow cue when playing

97fc298

Contributor Author

goddisignz commented Mar 27, 2020

How comes that the Travis status is still pending? It finished two days ago...

ninomp reviewed

View reviewed changes

src/engine/controls/cuecontrol.cpp Outdated Show resolved Hide resolved

Member

Holzhaus commented Mar 28, 2020

How comes that the Travis status is still pending? It finished two days ago...

There was probably a connection issue when travis tried to notify github. Just click on details, all builds passed.

ninomp reviewed

View reviewed changes

Contributor

ninomp left a comment •

edited

Loading

I tested this and noticed one minor issue: if quantization is enabled and Seek on load is set to Main cue, track jumps when analysis of track finishes. I'll make a PR against your repo tomorrow.

One minor thing: method isTrackAtZeroPos() is also unused and can be removed.

src/engine/controls/cuecontrol.cpp Show resolved Hide resolved

goddisignz and others added 3 commits

March 29, 2020 15:20


          removed unused and unnecessary methods

e96c90e


          Prevent jump when track analysis finishes while quantization is enabled

131de5f


          Fetch audible start position only when it is needed

efe411f

Contributor

ninomp commented Mar 30, 2020

Just created a PR with a fix for the issue I mentioned yesterday: https://github.com/goddisignz/mixxx/pull/1


          Merge branch 'ninomp-lp1855321_additional_fix' into LP1855321_zero_pl…

40b0b79

…ayposition_jump_to_cue

Contributor Author

goddisignz commented Mar 30, 2020

I just saw that the EngineBufferE2ETest.ScratchTestStart now fails for OSX with cmake. Is this something I/someone should investigate? Because I do not have a Mac to reconstruct this.

Member

ywwg commented Mar 30, 2020

I just saw that the EngineBufferE2ETest.ScratchTestStart now fails for OSX with cmake. Is this something I/someone should investigate? Because I do not have a Mac to reconstruct this.

does it still pass with master? or is it just this branch? It's possible to look at the waveforms and see what's wrong

Member

Holzhaus commented Mar 30, 2020 •

edited

Loading

I just saw that the EngineBufferE2ETest.ScratchTestStart now fails for OSX with cmake. Is this something I/someone should investigate? Because I do not have a Mac to reconstruct this.

does it still pass with master? or is it just this branch? It's possible to look at the waveforms and see what's wrong

I experienced this also occasionally on Linux. This isn't reproducible reliably - when you simply rerun the tests they usually all pass. This could indicate that there is some race condition in the engine scratch code somewhere. I restarted the build - let's see if this work now. In that case the issue is unrelated. Nevertheless, we should investigate the issue.

Contributor Author

goddisignz commented Mar 31, 2020

It's possible to look at the waveforms and see what's wrong

Although the test passed now, how can I do that? Does the Travis build and test generate any data that I can download?

IIRC, the test failed and there was something in the log about the faad library missing at the same place. Can't tell if this is relevant or not.

goddisignz requested a review from daschuer

April 4, 2020 15:52

Contributor

ninomp commented Apr 4, 2020

LGTM

Holzhaus approved these changes

View reviewed changes

Member

Holzhaus left a comment

I tested this with both preanalyzed and non-preanalyzed tracks. It works pretty good. Toggling quantize also shifts the position only if I didn't seek manually. The additional test that checks the new behaviour looks good and passes. The code also LGTM.

Holzhaus merged commit b94350b into mixxxdj:master

Holzhaus added a commit to Holzhaus/mixxx that referenced this pull request


          test/cuecontrol_test: Fix compile error due to missing method

0d75373

The test was added in mixxxdj#2593 but the Track::setSampleRate method was removed in
didn't help detecting this in this case.

Holzhaus mentioned this pull request

test/cuecontrol_test: Fix compile error due to missing method #2638

Merged

mixxxbot mentioned this pull request

issues with hotcues at 0:00 #9820

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

ywwg ywwg left review comments

ninomp ninomp left review comments

Be-ing Be-ing left review comments

Holzhaus Holzhaus approved these changes

daschuer Awaiting requested review from daschuer

Labels