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

Don't reset xfader when auto DJ is disabled #4714

Merged
merged 1 commit into from
Apr 10, 2022
Merged

Conversation

daschuer
Copy link
Member

@daschuer daschuer commented Apr 5, 2022

This fixes #10683

@ronso0
Copy link
Member

ronso0 commented Apr 5, 2022

As I stated in the original bug lp:1965298 I disagree with the rationale of the PR which introduced this behaviour #313 (I just never noticed since I don't use AutoDJ)

TLDR;
Let's remove the crossfader auto-centering completely.

Using automation when turning AutoDJ OFF to work around forgetting to check a central, obvious DJ control is not appropriate IMO. If that was about some rather inaccessible effect toggle or something, okay, but it's the crossfader :| And it's even more obvious since it's been moved by AutoDJ all the time.

we should remove such magic and leave the crossfader where it is, no matter if it's the last queued song or if AutoDJ is stopped manually.

You're right about not introducing changes for unaffected users in a stable release, that should be avoided if possible. However, I'd appreciate a consistent fix. Though, making the crossfader behaviour depend on the selected xfader mode is confusing IMO.

@daschuer
Copy link
Member Author

daschuer commented Apr 6, 2022

I agree for 2.4 to remove the behaviour completely.

When you think we can effort to break the existing reset feature in the stable release has well, I will change this PR accordingly. For now I think this PR is the least controversial version.

What do you think?

@ronso0
Copy link
Member

ronso0 commented Apr 6, 2022

When you think we can effort to break the existing reset feature in the stable release has well

If we want to fix the volume bug and agree that consistent, predictable behaviour is crucial there is no other choice than removing auto-centering now.

Also, I think for AutoDJ users it doesn't matter if we change the behaviour in a minor or in a major update. Let's keep in mind that Mixxx is primarily designed for manual DJing and AutoDJ is a supplementary feature. Even though there are appearantly many AutoDJ-only users, I doubt they mix its crossfader mixing with manual volume fader mixing, and if so resetting the crossfader is just a right-click away.

@ronso0
Copy link
Member

ronso0 commented Apr 6, 2022

What do other @mixxxdj/developers think?

When AutoDJ runs out (auto-off after fading to last queued track) should we

  • center the crossfader only with crossfader's mixing mode to avoid the volume reduction
  • remove auto-centering entirely

?

@Swiftb0y
Copy link
Member

Swiftb0y commented Apr 6, 2022

Also, I think for AutoDJ users it doesn't matter if we change the behaviour in a minor or in a major update. Let's keep in mind that Mixxx is primarily designed for manual DJing and AutoDJ is a supplementary feature. Even though there are appearantly many AutoDJ-only users, I doubt they mix its crossfader mixing with manual volume fader mixing, and if so resetting the crossfader is just a right-click away.

see https://www.hyrumslaw.com/. I still think we should change the behavior, but we need to make it clear enough in the changelog.

remove auto-centering entirely

I favor this. AutoDJ is already full of magic, making it seemingly unpredictable (and thus the reason I still don't trust it in live situations), so removing one more undocumented behavior is a good idea!

This fixes the volume change when the xfader is configured in const power mode, bug https://bugs.launchpad.net/bugs/1965298
@daschuer daschuer changed the title Don't reset xfader in const power mode when disabling crossfader. Don't reset xfader when auto DJ is disabled Apr 6, 2022
@daschuer daschuer added the changelog This PR should be included in the changelog label Apr 6, 2022
@daschuer
Copy link
Member Author

daschuer commented Apr 6, 2022

OK, I have removed the xfader reset unconditionally.

@ronso0
Copy link
Member

ronso0 commented Apr 6, 2022

Thank you, LGTM

@ronso0
Copy link
Member

ronso0 commented Apr 10, 2022

I also did a manual test of this one-line change. Works as intended : )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog This PR should be included in the changelog library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants