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

Sync Lock: disallow selection of Explicit Leader #12431

Merged
merged 3 commits into from
Dec 15, 2023

Conversation

ywwg
Copy link
Member

@ywwg ywwg commented Dec 12, 2023

In response to the sync_leader CO, enable soft leader instead of explicit leader. This is due to the large number of side effects and bugs in the explicit leader code. Effectively this removes the Explicit Leader feature from mixxx 2.4. The user can still select which deck they want to be soft leader if that selection is valid, but Mixxx will not allow the user to select an invalid leader -- specifically, a stopped deck while another deck is playing. If all decks are stopped, the first deck with a loaded track will always be leader.

Fixes: #11788.

We can reenable this later if we can fix those issues.

This helps work around stopped-leader bugs, like mixxxdj#11788.

We can reenable this later if we can fix those issues.
@ronso0
Copy link
Member

ronso0 commented Dec 12, 2023

Please remember to update the manual as well. Thank you!

@JoergAtGithub
Copy link
Member

Please describe the intended behavior in more words. Otherwise it's impossible to decide if the code implements it correct.

@daschuer
Copy link
Member

Error: /home/runner/work/mixxx/mixxx/src/engine/sync/enginesync.cpp:63:17: error: c++11 range-loop might detach Qt container (QList) [-Wclazy-range-loop-detach]
                for (Syncable* pSyncable : m_syncables) {
                ^
                                           qAsConst(  )

Please use std::as_const()

if (pSyncable->getBaseBpm().isValid()) {
if (!pSyncable->isPlaying()) {
bool anyPlaying = false;
// Only allow a non-playing deck to be leader if no deck is playing
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a use case for this?

  • Load two decks.
  • sync both.
  • select a soft-leader
  • play the soft leader -> it is picked anyway
  • play the follower -> it becomes the new leader

So it looks like users choice is ignored anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

So it looks like users choice is ignored anyway.

I think I can fix that be preferring the current leader if there are multiple equivalent choices

This prevents unexpected switches of leader status.

Also corrects a test that did not match its comments.
ywwg added a commit to ywwg/manual that referenced this pull request Dec 13, 2023
We've removed this feature due to bugs, but most of the useful functionality is still available
See mixxxdj/mixxx#12431
@ywwg
Copy link
Member Author

ywwg commented Dec 13, 2023

mixxxdj/manual#600

@daschuer
Copy link
Member

Thank you. What is the workflow where the user has a benefit if a preselected soft leader. Can you give a hint for workflow we can test?

@ywwg
Copy link
Member Author

ywwg commented Dec 13, 2023

Thank you. What is the workflow where the user has a benefit if a preselected soft leader. Can you give a hint for workflow we can test?

Mostly it's to prevent user confusion when the decks are stopped, like this:

  1. enable sync on both decks (both stopped)
  2. by default, deck 1 is the sync leader
  3. Try to select deck two as sync leader -- nothing happens! annoying!
  4. play deck two -- now it activates as sync leader.

Alternatively we could always choose internal clock as sync leader when all decks are stopped.

@daschuer
Copy link
Member

With this branch (not tested)

  • Select deck 2 as implicit leader
  • play deck 1, deck 1 becomes again implicit leader
  • users choice is not respected - annoying!

So yes, hiding the leader crown on stopped deck seems to be a good workaround for the visual issue.

@ywwg
Copy link
Member Author

ywwg commented Dec 13, 2023

Oof unfortunately that behavior breaks a LOT of tests and assumptions, and it's not trivial to fix. I am unable to make it work easily. I think we can update the docs to say that "Mixxx will always prefer a playing deck for leader, overriding the user's preference" which is actually what we want, based on the original bug report.

@daschuer
Copy link
Member

Original we could also not change the soft leaders in stopped decks. So to make progress here, how about just remove the the added logic that allows to shifting a soft leader to an other stopped deck and merge this.

During testing I have discovered that it is currently not allowed to select a soft leader. The feature would be nice use this to switch to the internal clock as leader.

@ywwg
Copy link
Member Author

ywwg commented Dec 14, 2023

ok done.

@ywwg
Copy link
Member Author

ywwg commented Dec 14, 2023

In the future I'd like to make it so that when all decks are stopped, the InternalClock is leader, but doing that causes too many test failures / side effects right now. I think it can be solved with some logic fixes around setting the internal clock BPM but that's out of scope for 2.4

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

LGTM and works good. Thank you for taking care.

@daschuer daschuer merged commit 04b7dd1 into mixxxdj:2.4 Dec 15, 2023
7 of 11 checks passed
@daschuer daschuer added this to the 2.4.0 milestone Dec 15, 2023
@Russe
Copy link
Contributor

Russe commented Dec 17, 2023

Excellent, thanks for resolving this issue. I tested the most recent beta, now the problem (#11788) should never happen again.

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.

5 participants