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

After disabling Explicit leader, request always follower mode #11831

Merged
merged 5 commits into from
Oct 6, 2023

Conversation

daschuer
Copy link
Member

… and let Mixxx pick a new Soft leader.

This fixes the bug that a stopped or silenced deck can become a Soft leader. #11829

@daschuer daschuer requested a review from ywwg August 14, 2023 06:10
@JoergAtGithub
Copy link
Member

JoergAtGithub commented Aug 14, 2023

I don't think this is the correct behavior. We had the handling of track end until #11035
which removed the code instead of fixing the bug in it.

@daschuer
Copy link
Member Author

This has nothing to do with end of track and explicit leader.

The scenario is that you have picked an explicit leader and you are done with the track. In current 2.4 you have the option to pick a new explicit leader or make the original done track a soft leader. The later is the bug, because there is no usecase for this. This PR picks the best deck as soft leader, which should be the other deck currently playing.

… Mixxx pick a new Soft leader.

This fixes the bug that a stopped or silenced deck can become a Soft leader
@daschuer
Copy link
Member Author

daschuer commented Sep 9, 2023

@JoergAtGithub Can you have another look?

@ywwg
Copy link
Member

ywwg commented Sep 29, 2023

@daschuer I think this is the correct fix -- once the explicit leader is done playing the DJ will want something else useful to be the new leader, and that will probably be some other playing deck. Selecting Follower will trigger the implicit leader selection, and I think that's good behavior

activateFollower(pSyncable);
// leader mode if this still be best choice
if (m_pLeaderSyncable == pSyncable) {
// Avoid that pickLeader() returns pSyncable with SyncMode::LeaderExplicit
Copy link
Member

Choose a reason for hiding this comment

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

this code feels like a workaround for a bug. Am I right that just calling activateFollower(pSyncable) is not working because that syncable is already explicit leader? I think this code should remain the same and the edge case should be fixed elsewhere

@@ -519,8 +519,6 @@ void SyncControl::slotSyncLeaderEnabledChangeRequest(double state) {
// Turning off leader goes back to follower mode.
switch (mode) {
case SyncMode::LeaderExplicit:
m_pChannel->getEngineBuffer()->requestSyncMode(SyncMode::LeaderSoft);
Copy link
Member

Choose a reason for hiding this comment

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

yes this looks right

Copy link
Member

@Holzhaus Holzhaus Sep 29, 2023

Choose a reason for hiding this comment

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

Wasn't this intentional? I think the explicit leader also enabled scratching both decks in sync. In case you don't want this but you're unhappy with Mixxx automatic selection, the trick was to make a deck explicit leader, then disable the explicit leader to make it soft leader. Does this still work?

Copy link
Member

Choose a reason for hiding this comment

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

hm good question

Copy link
Member

@ywwg ywwg Sep 29, 2023

Choose a reason for hiding this comment

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

yeah that could be confusing to users, it might pick a different deck. In practice, when implicit leader is active there's no functional difference between decks... but having leader jump around is not great.

I think I agree, we want to keep this the way it was and just focus on the specific bug

@daschuer
Copy link
Member Author

Should work now as demanded.

@daschuer
Copy link
Member Author

daschuer commented Oct 4, 2023

It is now working. It was more tricky than expected. Sorry for the rebase.

Copy link
Member

@JoergAtGithub JoergAtGithub left a comment

Choose a reason for hiding this comment

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

I built this PR and played a bit with the different state of the decks (audible/not audible, playing/not-playing, synced/not-synced, track end). I needed a bit to get used to it, but I think the behaviour is correct now.
Code LGTM!

@daschuer daschuer changed the base branch from main to 2.4 October 6, 2023 06:00
@daschuer daschuer added this to the 2.4.0 milestone Oct 6, 2023
@daschuer
Copy link
Member Author

daschuer commented Oct 6, 2023

Because this is a bug-fix I have changed the target to 2.4
Merge?

@JoergAtGithub JoergAtGithub merged commit 7e1b0e3 into mixxxdj:2.4 Oct 6, 2023
@JoergAtGithub
Copy link
Member

Thank you!

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.

Reseting explicit sync leader picks the wrong implicit leader
4 participants