Fix sampler sync functionality for Mixxx skins

[urek69mazino]

This PR addresses an issue with the sampler sync functionality across several Mixxx skins, updating the sync control to improve usability and ensure consistency in behavior.

Changes Made:
Updated XML Configuration Files for Skins:

Replaced beatsync_beatsync_tempo with sync_enabled in the following XML files to align with the updated sync functionality:
Deere: res/skins/Deere/sampler_controls_row.xml (Line 88)
Shade: res/skins/Shade/sampler.xml (Line 301)
LateNight: res/skins/LateNight/samplers/sampler.xml (Line 370)
Tango: res/skins/Tango/mic_aux_sampler/sampler.xml (Lines 128 and 340)
Updated Tooltip and Connection Attributes:

Modified the TooltipId and left_connection_control attributes as required to use sync_enabled for improved control and readability.
Testing:
Verified each skin to ensure that pressing the sync button behaves as expected.
Held the sync button to confirm that the "lock sync" feature engages as described.
No issues or regressions observed after these changes.
Additional Notes:
This PR focuses solely on improving sync consistency across the mentioned skins and should not affect other skin functionality.
Please review and test the sync button behavior, especially for the lock sync feature, to confirm these changes work across various setups.

[ronso0]

Thank you for this PR!
Please sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

[ronso0]

Since this is a fix it should target the stable branch 2.4
Please rebase and force-push. Thank you!

If you need help with git just let us know.

[urek69mazino]

when i try to rebase and do a pull request the following error is being shown "Pull request creation failed. Validation failed: must be a collaborator"

[urek69mazino]

fixed it, and now I have committed the changes in the 2.4 branch

[cr7pt0gr4ph7]

fixed it, and now I have committed the changes in the 2.4 branch

@urek696969mazino It looks like your new PR #13821 contains a lot more than just your changes.

I think what @ronso0 meant was to rebase this single commit onto the 2.4 branch, force push to the PR branch of this, and change the target branch of this PR:

# (Assuming "origin" points to your GitHub repository)
git remote add upstream [email protected]:mixxxdj/mixxx.git
git fetch upstream
git checkout fix-sampler-sync
git rebase --onto upstream/2.4 upstream/main
git push origin --force-with-lease

Changing the target branch of this PR:

Step 1: Click on Edit

Step 2: Select base: main combobox

Step 3: Choose 2.4 as new base branch

[urek69mazino]

@cr7pt0gr4ph7 i have made the changes as directed by you , kindly see if any other changes need to be made

[cr7pt0gr4ph7]

Thank you for this PR! It seems that this has slipped through the cracks for at least ~10 years, going by the fact that the new sync_enabled tooltips where introduced by #298 in July of 2014...

While you're at it, could you also update the Shade skin at res/skins/Shade/sampler.xml?

mixxx/res/skins/Shade/sampler.xml

Lines 300 to 319 in edae0ca

	<PushButton>
	<TooltipId>beatsync_beatsync_tempo</TooltipId>
	<NumberStates>1</NumberStates>
	<State>
	<Number>0</Number>
	<Pressed>skin:/btn/btn_sync_sampler_overdown.png</Pressed>
	<Unpressed>skin:/btn/btn_sync_sampler.png</Unpressed>
	</State>
	<Pos>3,4</Pos>
	<Connection>
	<ConfigKey><Variable name="group"/>,beatsync</ConfigKey>
	<EmitOnPressAndRelease>true</EmitOnPressAndRelease>
	<ButtonState>LeftButton</ButtonState>
	</Connection>
	<Connection>
	<ConfigKey><Variable name="group"/>,beatsync_tempo</ConfigKey>
	<EmitOnPressAndRelease>true</EmitOnPressAndRelease>
	<ButtonState>RightButton</ButtonState>
	</Connection>
	</PushButton>

There's also a general discussion to be had (probably independent of this PR) on how (and whether) to handle beatsync_tempo in the new sync_enabled model, see comments below for further discussion.

[cr7pt0gr4ph7]

You are only changing the tooltip here. To replace the functionality of the button, you also have to replace left_connection below.

            <SetVariable name="left_connection_control"><Variable name="group"/>,sync_enabled</SetVariable>

Note that there is corresponding replacement for beatsync_tempo, so the right_connection should be removed. See issue comments for further discussion.

[cr7pt0gr4ph7]

Same applies here: You are currently only changing the tooltip.

[cr7pt0gr4ph7]

And also here: You are only changing the tooltip.

[cr7pt0gr4ph7]

@ronso0 (or somebody else who's knowledgeable about how this is supposed to work):
As mentioned in my review comment, there are some behavioral mismatches between the old and new sync behavior that should probably be resolved before merging this.

As a short recap, here is the current behavior, as verified using the source code and a pretty current build of Mixxx's main branch where the quantize setting for the samplers was modified through the developer tools dialog.

Old behavior with beatsync/beatsync_tempo

The old beatsync behavior was like this (and still is, if you use beatsync/beatsync_tempo directly):

• Left-click: Instanteanously sync tempo and phase to what I will call temporary sync target, for the lack of a better term.
• Right-click: Instanteanously sync tempo to the temporary sync target.

Where the temporary sync target is picked by EngineSync::pickNonSyncSyncTarget according to the rules explained in its comments:

mixxx/src/engine/sync/enginesync.h

Lines 57 to 65 in edae0ca

	/// Used to pick a sync target for cases where Leader sync mode is not sufficient.
	/// Guaranteed to pick a Syncable that is a real deck and has an EngineBuffer,
	/// but can return nullptr if there are no choices.
	/// First choice is Leader sync, if it's a real deck,
	/// then it will fall back to the first playing syncable deck,
	/// then it will fall back to the first playing deck,
	/// then it will fall back to the first non-playing deck.
	/// If there is literally nothing loaded, returns nullptr.
	Syncable* pickNonSyncSyncTarget(EngineChannel* pDontPick) const;

Main points:

• Works even if no deck is currently the sync leader, and just makes a "best guess" in that case.
• We do not care about the quantize setting for the sampler.

New behavior with sync_enabled

While the new behavior for sync_enabled is currently like this:

• Left-click (short press when button is not toggled on): Instanteanously sync tempo (and phase, if quantize is enabled) to the temporary sync target.
• Left-click (long press): Enable sync tempo (and phase, if quantize is enabled for both the current and the target deck) to the temporary sync target.
• Left-click (short press when button is toggled on): Disable sync tempo (and phase)
• Independent of sync_enabled setting: Pressing play on a deck, changing its position via seek etc. automatically snap phase to the temporary sync target so that the first beat is aligned with the current beat of the temporary sync target if quantize is enabled for the current deck/sampler (so we do not care about the quantize setting of the target deck).

Main points:

• Still works even if no deck is currently the sync leader, and just makes a "best guess" in that case.
• The quantize setting is very much relevant now -- but there is currently no way to control the quantize setting for samplers in the UI.
  We probably want one, though, because otherwise it is not possible to sync tempo and phase, only tempo.
  • One option might be to set quantize to 1 for all samplers (which automatically aligns the phase when the sampler play button is pressed, even when sync is disabled), but there are probably cases where the users wants this phase lock behavior, and others where the user wants to play the sampler immediately/without phase alignment.

[cr7pt0gr4ph7]

@cr7pt0gr4ph7 i have made the changes as directed by you , kindly see if any other changes need to be made

Thank you! Though it seems that you haven't done the rebase yet. The output should look like this:

❯ git checkout fix-sampler-sync
Already on 'fix-sampler-sync'
Your branch is up to date with 'origin/fix-sampler-sync'.

❯ git rebase --onto upstream/2.4 upstream/main
Successfully rebased and updated refs/heads/fix-sampler-sync.

❯ git push origin --force-with-lease
Enumerating objects: 23, done.
Counting objects: 100% (23/23), done.
Delta compression using up to 8 threads
Compressing objects: 100% (9/9), done.
Writing objects: 100% (12/12), 1016 bytes | 1016.00 KiB/s, done.
Total 12 (delta 11), reused 3 (delta 3), pack-reused 0 (from 0)
remote: Resolving deltas: 100% (11/11), completed with 11 local objects.
To github.com:urek69mazino/mixxx.git
 + b301ccac64...19b608363d fix-sampler-sync -> fix-sampler-sync (forced update)

When viewing your version history via gitk, the state before the rebase should look like this:

And afterwards, it should look like this:

[cr7pt0gr4ph7]

@urek69mazino Could you send me the output of git remote -v?

[cr7pt0gr4ph7]

@urek69mazino For your convenience, you can find a rebased version of your changes at cr7pt0gr4ph7/mixxx:fix-sampler-sync, so you can force push the commits in that branch to your own fix-sampler-sync branch.