Rename Sync instances of "master" to "leader" #3921

ywwg · 2021-05-28T20:36:40Z

Also add an alias from the sync_master CO to sync_leader.

This PR depends on my stopped-explicit branch so it will appear to have those changes. This PR is a no-op change.

JoergAtGithub · 2021-05-28T20:46:14Z

Does this break the API for controller mappings?

ywwg · 2021-05-28T22:46:39Z

it shouldn't, because I have an alias from sync_master to sync_leader. Old scripts should continue to work.

ywwg · 2021-06-01T21:45:01Z

depends on #3706 which depends on #3899

Holzhaus · 2021-06-01T23:31:27Z

FYI if you pushed a broken merge, you can still amend it later with git commit --amend. Please make sure that all commit build.

ywwg · 2021-06-04T03:54:10Z

I'm glad we've moved to squash-commits by default! I prefer it

daschuer · 2021-06-04T14:12:18Z

Did we? I don't prefere it, because "blame" will become less significant.

ywwg · 2021-06-04T14:40:29Z

Oh, so it's more like "squash out commits that were objectively broken"

Holzhaus · 2021-06-04T15:58:15Z

Yes, we don't squash by default. We just want to avoid commits that don't build because it makes bisecting impossible.

Also add an alias from the sync_master CO to sync_leader

ywwg · 2021-06-16T02:59:55Z

fixed the commit order so it's not completely messed up

ywwg · 2021-07-09T19:26:30Z

fixed conflicts, ready for review

Holzhaus · 2021-07-09T19:56:28Z

Some pre commit issues.

src/engine/sync/syncable.h

Holzhaus · 2021-07-09T20:04:20Z

Some pre commit issues.

You don't need to fix the eslint issues, but the end-of-file issues would be nice.

ywwg · 2021-07-10T01:53:45Z

You don't need to fix the eslint issues, but the end-of-file issues would be nice.

yeah using the autoformatter results in a gigantic number of changes

ywwg · 2021-07-10T14:26:51Z

fixed conflict

ywwg · 2021-07-10T14:27:43Z

no diff when I run that command:
Fix End of Files.........................................................Passed

coveralls · 2021-07-10T14:58:52Z

Pull Request Test Coverage Report for Build 1051095745

167 of 204 (81.86%) changed or added relevant lines in 8 files are covered.
4 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.008%) to 28.636%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/engine/controls/ratecontrol.cpp	0	1	0.0%
src/engine/controls/bpmcontrol.cpp	0	2	0.0%
src/engine/sync/internalclock.cpp	23	27	85.19%
src/engine/sync/synccontrol.cpp	45	58	77.59%
src/engine/sync/enginesync.cpp	82	99	82.83%

Files with Coverage Reduction	New Missed Lines	%
src/engine/sync/synccontrol.cpp	1	82.05%
src/engine/sync/internalclock.cpp	3	80.51%

Totals
Change from base Build 1050590600:	0.008%
Covered Lines:	20095
Relevant Lines:	70175

💛 - Coveralls

ywwg · 2021-07-12T14:40:07Z

friendly ping

Holzhaus · 2021-07-12T16:34:16Z

I was wondering if we should really change all legacy mappings, since these are not pre-commit clean and this will cause pre-commit issues when merging main into other branches locally. It's not necessary because the old name still works due to the alias.

But many people copy&paste code from old mappings and, so term would proliferate. Let's do this once now.

Important: if you're reading this because you experienced the issue I described above, please do this: if you're only contributing a controller mapping, please fetch latest main and rebase. If you're working on a branch unrelated to mappings, commit the merge commit with SKIP=eslint git commit.

Holzhaus

Thanks. I grepped the code and found some more sync-related uses of the master term that you missed:

../src/test/synccontroltest.cpp:4:// * Quantize mode nudges tracks in sync, whether internal or deck master.
../src/test/co_dumps/co_dump_inital.csv:100:[Channel1],sync_master,0
../src/test/co_dumps/co_dump_inital.csv:112:[Channel2],sync_master,0
../src/test/co_dumps/co_dump_inital.csv:173:[Channel3],sync_master,0
../src/test/co_dumps/co_dump_inital.csv:190:[Channel4],sync_master,0
../src/test/co_dumps/co_dump_inital.csv:346:[PreviewDeck1],sync_master,0
../src/test/co_dumps/co_dump_inital.csv:9714:[Sampler3],sync_master,0
../src/test/co_dumps/co_dump_inital.csv:9725:[Sampler4],sync_master,0
../src/test/co_dumps/co_dump_inital.csv:9740:[Sampler1],sync_master,0
../src/test/co_dumps/co_dump_inital.csv:9762:[Sampler2],sync_master,0
../src/test/co_dumps/co_dump_inital.csv:9912:[InternalClock],sync_master,0
../src/skin/legacy/tooltips.cpp:553:            << tr("Tap to sync the tempo to other playing tracks or the master clock.")
../src/skin/legacy/tooltips.cpp:567:            << tr("When enabled, this device will serve as the master clock for all other decks.");
../src/engine/sync/syncable.h:144:    // Update the playback speed of the master, including scratch values.
../src/engine/controls/bpmcontrol.cpp:424:    // If we are not quantized, or there are no beats, or we're master,
../src/engine/controls/bpmcontrol.cpp:463:    // Either shortest distance is directly to the master or backwards.
../src/engine/controls/bpmcontrol.cpp:466:    // from master to my percentage. All of the control code below is based on
../src/engine/controls/bpmcontrol.cpp:483:        kLogger.trace() << "master beat distance:" << syncTargetBeatDistance;
../src/controllers/controlpickermenu.cpp:235:               tr("Increase internal master BPM by 1"), syncMenu);
../src/controllers/controlpickermenu.cpp:238:               tr("Decrease internal master BPM by 1"), syncMenu);
../src/controllers/controlpickermenu.cpp:241:               tr("Increase internal master BPM by 0.1"), syncMenu);
../src/controllers/controlpickermenu.cpp:243:               tr("Decrease internal master BPM by 0.1"), syncMenu);
../src/controllers/controlpickermenu.cpp:245:    addDeckAndSamplerControl("sync_leader", tr("Sync Leader"), tr("Toggle sync master"), syncMenu);

fossdd

Would like to see that in Mixxx. also due to its negative connotation. See https://lore.kernel.org/git/CAOAHyQwyXC1Z3v7BZAC+Bq6JBaM7FvBenA-1fcqeDV==apdWDg@mail.gmail.com/

Holzhaus · 2021-07-14T11:26:53Z

Would like to see that in Mixxx. also due to its negative connotation. See https://lore.kernel.org/git/CAOAHyQwyXC1Z3v7BZAC+Bq6JBaM7FvBenA-1fcqeDV==apdWDg@mail.gmail.com/

Yes, we already committed to doing that here: https://mixxx.org/news/2020-06-29-black-lives-matter/

fossdd · 2021-07-14T12:21:54Z

Ah perfect! didn't see that

Holzhaus · 2021-07-14T18:17:40Z

Thanks. I grepped the code and found some more sync-related uses of the master term that you missed:

../src/test/synccontroltest.cpp:4:// * Quantize mode nudges tracks in sync, whether internal or deck master.
../src/test/co_dumps/co_dump_inital.csv:100:[Channel1],sync_master,0
../src/test/co_dumps/co_dump_inital.csv:112:[Channel2],sync_master,0
../src/test/co_dumps/co_dump_inital.csv:173:[Channel3],sync_master,0
../src/test/co_dumps/co_dump_inital.csv:190:[Channel4],sync_master,0
../src/test/co_dumps/co_dump_inital.csv:346:[PreviewDeck1],sync_master,0
../src/test/co_dumps/co_dump_inital.csv:9714:[Sampler3],sync_master,0
../src/test/co_dumps/co_dump_inital.csv:9725:[Sampler4],sync_master,0
../src/test/co_dumps/co_dump_inital.csv:9740:[Sampler1],sync_master,0
../src/test/co_dumps/co_dump_inital.csv:9762:[Sampler2],sync_master,0
../src/test/co_dumps/co_dump_inital.csv:9912:[InternalClock],sync_master,0
../src/skin/legacy/tooltips.cpp:553:            << tr("Tap to sync the tempo to other playing tracks or the master clock.")
../src/skin/legacy/tooltips.cpp:567:            << tr("When enabled, this device will serve as the master clock for all other decks.");
../src/engine/sync/syncable.h:144:    // Update the playback speed of the master, including scratch values.
../src/engine/controls/bpmcontrol.cpp:424:    // If we are not quantized, or there are no beats, or we're master,
../src/engine/controls/bpmcontrol.cpp:463:    // Either shortest distance is directly to the master or backwards.
../src/engine/controls/bpmcontrol.cpp:466:    // from master to my percentage. All of the control code below is based on
../src/engine/controls/bpmcontrol.cpp:483:        kLogger.trace() << "master beat distance:" << syncTargetBeatDistance;
../src/controllers/controlpickermenu.cpp:235:               tr("Increase internal master BPM by 1"), syncMenu);
../src/controllers/controlpickermenu.cpp:238:               tr("Decrease internal master BPM by 1"), syncMenu);
../src/controllers/controlpickermenu.cpp:241:               tr("Increase internal master BPM by 0.1"), syncMenu);
../src/controllers/controlpickermenu.cpp:243:               tr("Decrease internal master BPM by 0.1"), syncMenu);
../src/controllers/controlpickermenu.cpp:245:    addDeckAndSamplerControl("sync_leader", tr("Sync Leader"), tr("Toggle sync master"), syncMenu);

@ywwg can you fix those? then we can merge imho.

ywwg · 2021-07-19T18:11:47Z

ok picking this back up, thanks for finding the other remaining instances!

Remove sync_mode from control picker menu, it's an internal CO that should not need to be mapped.

ywwg · 2021-07-19T18:22:35Z

Updated based on that list. I removed the addDeckAndSamplerControl for sync_mode, it really should not be mapped (users should map sync_enabled and/or sync_leader.

Holzhaus

@ywwg Thanks. Unfortunately, I think you messed up a merge or something. I see a bunch of unrelated changes in the diff. Please fix that.

Also, these is one remaining occurence of the term related to sync - inenginebuffer.cpp, line 1328. Maybe you can correct that while you're at it.

ywwg · 2021-07-20T14:07:54Z

I skimmed through the diff and couldn't find any unintended changes, could you give me an example?

CMakeLists.txt

ywwg · 2021-07-21T02:57:41Z

merged from main, I think that fixed the problem diffs

Squashed commit of the following: commit 6a50c70 Merge: 805beb7 ac7daeb Author: Owen Williams <owilliams@mixxx.org> Date: Tue Jul 20 22:55:05 2021 -0400 Merge branch 'main' into syncleader commit 805beb7 Author: Owen Williams <owilliams@mixxx.org> Date: Tue Jul 20 10:08:16 2021 -0400 Sync Lock: missed a conversion to "leader" commit 23942ee Author: Owen Williams <owilliams@mixxx.org> Date: Mon Jul 19 14:20:30 2021 -0400 Sync Lock: Update remaining instances of "master" Remove sync_mode from control picker menu, it's an internal CO that should not need to be mapped. commit 6fbf69a Merge: eb76915 36781b1 Author: Owen Williams <owilliams@mixxx.org> Date: Mon Jul 19 14:12:06 2021 -0400 Merge branch 'syncleader' commit 36781b1 Merge: e79703d 3ef395b Author: Owen Williams <owilliams@mixxx.org> Date: Sat Jul 10 10:07:09 2021 -0400 Merge branch 'main' into syncleader commit e79703d Author: Owen Williams <owilliams@mixxx.org> Date: Sat Jul 10 09:57:49 2021 -0400 Sync Lock: convert enums to enum classes commit 8559d2f Author: Owen Williams <owilliams@mixxx.org> Date: Fri Jul 9 15:32:18 2021 -0400 Sync Lock: rename a few more instances of sync_master to sync_leader commit 9f374f4 Merge: 9e85796 422912b Author: Owen Williams <owilliams@mixxx.org> Date: Fri Jul 9 15:25:26 2021 -0400 Merge branch 'main' into syncleader commit 9e85796 Author: Owen Williams <owilliams@mixxx.org> Date: Fri May 28 16:34:16 2021 -0400 Rename Sync instances of "master" to "leader" Also add an alias from the sync_master CO to sync_leader commit 2d2b485 Author: Owen Williams <owilliams@mixxx.org> Date: Tue Jun 15 22:21:04 2021 -0400 Sync Lock: Don't recalc half/double multiplier on every callback commit fa980d9 Author: Owen Williams <owilliams@mixxx.org> Date: Mon Mar 15 15:51:51 2021 -0400 Sync Lock: rename functions to make them clearer

Holzhaus · 2021-07-22T20:44:34Z

Merged in 0fb764a.

…#3921)

github-actions bot added code quality controller mappings skins labels May 28, 2021

ywwg marked this pull request as draft May 28, 2021 20:47

ywwg force-pushed the syncleader branch 3 times, most recently from 3f679d2 to 9d6ea2a Compare May 28, 2021 22:46

ywwg mentioned this pull request Jun 4, 2021

Sync Lock: Differentiate track loading from beats updating #3944

Merged

ywwg added 3 commits June 15, 2021 22:21

Sync Lock: rename functions to make them clearer

fa980d9

Sync Lock: Don't recalc half/double multiplier on every callback

2d2b485

Rename Sync instances of "master" to "leader"

9e85796

Also add an alias from the sync_master CO to sync_leader

ywwg force-pushed the syncleader branch from 17c2c68 to 9e85796 Compare June 16, 2021 02:59

daschuer mentioned this pull request Jun 20, 2021

Sync Lock: make sure to reinit leader params after scratching ends #4005

Merged

Merge branch 'main' into syncleader

9f374f4

ywwg marked this pull request as ready for review July 9, 2021 19:26

Sync Lock: rename a few more instances of sync_master to sync_leader

8559d2f

Holzhaus reviewed Jul 9, 2021

View reviewed changes

src/engine/sync/syncable.h Outdated Show resolved Hide resolved

Holzhaus requested changes Jul 12, 2021

View reviewed changes

fossdd approved these changes Jul 14, 2021

View reviewed changes

ywwg added 2 commits July 19, 2021 14:12

Merge branch 'syncleader'

6fbf69a

Sync Lock: Update remaining instances of "master"

23942ee

Remove sync_mode from control picker menu, it's an internal CO that should not need to be mapped.

github-actions bot added build library labels Jul 19, 2021

Holzhaus requested changes Jul 20, 2021

View reviewed changes

Sync Lock: missed a conversion to "leader"

805beb7

Holzhaus reviewed Jul 20, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Merge branch 'main' into syncleader

6a50c70

Holzhaus closed this Jul 22, 2021

Holzhaus added a commit to Holzhaus/mixxx that referenced this pull request Aug 6, 2021

EngineSync: Rebname local leader bpm variables (leftover from mixxxdj…

5088610

…#3921)

Holzhaus added a commit to Holzhaus/mixxx that referenced this pull request Aug 6, 2021

EngineSync: Rebname local leader bpm variables (leftover from mixxxdj…

8faf04d

…#3921)

Holzhaus added a commit to Holzhaus/mixxx that referenced this pull request Aug 6, 2021

EngineSync: Rebname local leader bpm variables (leftover from mixxxdj…

e99bf9d

…#3921)

ronso0 pushed a commit to ronso0/mixxx that referenced this pull request Sep 24, 2021

EngineSync: Rebname local leader bpm variables (leftover from mixxxdj…

4cb95bd

…#3921)

Holzhaus mentioned this pull request Sep 8, 2023

Update documentation with changes from offensive language removal mixxxdj/manual#578

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename Sync instances of "master" to "leader" #3921

Rename Sync instances of "master" to "leader" #3921

ywwg commented May 28, 2021

JoergAtGithub commented May 28, 2021

ywwg commented May 28, 2021

ywwg commented Jun 1, 2021

Holzhaus commented Jun 1, 2021

ywwg commented Jun 4, 2021

daschuer commented Jun 4, 2021

ywwg commented Jun 4, 2021

Holzhaus commented Jun 4, 2021

ywwg commented Jun 16, 2021

ywwg commented Jul 9, 2021

Holzhaus commented Jul 9, 2021

Holzhaus commented Jul 9, 2021 •

edited

Loading

ywwg commented Jul 10, 2021

ywwg commented Jul 10, 2021

ywwg commented Jul 10, 2021

coveralls commented Jul 10, 2021 •

edited

Loading

ywwg commented Jul 12, 2021

Holzhaus commented Jul 12, 2021

Holzhaus left a comment •

edited

Loading

fossdd left a comment

Holzhaus commented Jul 14, 2021

fossdd commented Jul 14, 2021 via email

Holzhaus commented Jul 14, 2021

ywwg commented Jul 19, 2021

ywwg commented Jul 19, 2021

Holzhaus left a comment

ywwg commented Jul 20, 2021

ywwg commented Jul 21, 2021

Holzhaus commented Jul 22, 2021

Rename Sync instances of "master" to "leader" #3921

Rename Sync instances of "master" to "leader" #3921

Conversation

ywwg commented May 28, 2021

JoergAtGithub commented May 28, 2021

ywwg commented May 28, 2021

ywwg commented Jun 1, 2021

Holzhaus commented Jun 1, 2021

ywwg commented Jun 4, 2021

daschuer commented Jun 4, 2021

ywwg commented Jun 4, 2021

Holzhaus commented Jun 4, 2021

ywwg commented Jun 16, 2021

ywwg commented Jul 9, 2021

Holzhaus commented Jul 9, 2021

Holzhaus commented Jul 9, 2021 • edited Loading

ywwg commented Jul 10, 2021

ywwg commented Jul 10, 2021

ywwg commented Jul 10, 2021

coveralls commented Jul 10, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1051095745

💛 - Coveralls

ywwg commented Jul 12, 2021

Holzhaus commented Jul 12, 2021

Holzhaus left a comment • edited Loading

Choose a reason for hiding this comment

fossdd left a comment

Choose a reason for hiding this comment

Holzhaus commented Jul 14, 2021

fossdd commented Jul 14, 2021 via email

Holzhaus commented Jul 14, 2021

ywwg commented Jul 19, 2021

ywwg commented Jul 19, 2021

Holzhaus left a comment

Choose a reason for hiding this comment

ywwg commented Jul 20, 2021

ywwg commented Jul 21, 2021

Holzhaus commented Jul 22, 2021

Holzhaus commented Jul 9, 2021 •

edited

Loading

coveralls commented Jul 10, 2021 •

edited

Loading

Holzhaus left a comment •

edited

Loading