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

Tempo locking controls #13041

Merged
merged 1 commit into from
Apr 10, 2024
Merged

Tempo locking controls #13041

merged 1 commit into from
Apr 10, 2024

Conversation

FrankwaP
Copy link
Contributor

@FrankwaP FrankwaP commented Apr 3, 2024

This PR adds controls for tempo locking (tempo_lock) and unlocking (locking) so they can be used through keyboard shortcuts or MIDI inputs,a s suggested in 13038.

@github-actions github-actions bot added the engine label Apr 3, 2024
@ronso0
Copy link
Member

ronso0 commented Apr 3, 2024

Thanks for this PR!

Since this is a deck control we should eep it simple and have just one control:
bpm_toggle_lock
This will also allow to simplify the slots. You can get the track's lock state with Track::isBpmLocked().
"bpm" because "tempo" refers to the playback speed.

In case this shall be adopted for the library later on we will also make do with just one control, even if multiple tracks with locked/unlocked are selected:

  • all the same lock state: toggle lock
  • different lock state: lock all, trigger again to unlock all

@ronso0
Copy link
Member

ronso0 commented Apr 3, 2024

As a first-time contributor we need you to 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.

Please comment here when done.
Thanks!

@ronso0
Copy link
Member

ronso0 commented Apr 3, 2024

Since this is a deck control we should eep it simple and have just one control:
bpm_toggle_lock

In case you adopt this you may very well create a fixup commit, squash all (the 4) commits and force-push.

Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

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

Thank, this is much cleaner now.

Some comments though

src/controllers/controlpickermenu.cpp Outdated Show resolved Hide resolved
src/engine/controls/bpmcontrol.cpp Outdated Show resolved Hide resolved
src/engine/controls/bpmcontrol.h Outdated Show resolved Hide resolved
@ronso0
Copy link
Member

ronso0 commented Apr 5, 2024

Thanks, I'll try this next days.

@ronso0
Copy link
Member

ronso0 commented Apr 5, 2024

(feel free to ping me in case this slips off my table ; )

std:make_unique instead of new

fix crash when channel/trackpointer is empty

renaming the variables changing "tempo" for "bpm"

"Toggle lock" instead of "lock"/"unlock"

forgot to save the file lol

Update src/engine/controls/bpmcontrol.cpp

Co-authored-by: ronso0 <ronso0@mixxx.org>

fixing the description

unnecessary comment

more explicit command
Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

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

perfecto, LGTM

@ronso0
Copy link
Member

ronso0 commented Apr 9, 2024

FYI review conversations are resolved by reviewers only (and when the contributor picks a code suggestion :| ). Otherwise it's tedious for reviewers to check whether all comments have been addressed.

@ronso0
Copy link
Member

ronso0 commented Apr 10, 2024

Thank you!

Can you please take care of the documentation in https://manual.mixxx.org/2.5/chapters/appendix/mixxx_controls.html ?

@ronso0 ronso0 merged commit ada244c into mixxxdj:main Apr 10, 2024
13 checks passed
@ronso0
Copy link
Member

ronso0 commented Apr 10, 2024

In case this shall be adopted for the library later on we will also make do with just one control, even if multiple tracks with locked/unlocked are selected:

* all the same lock state: toggle lock

* different lock state: lock all, trigger again to unlock all

If you want to work on that, too, note that the control flow is a bit more elaborate:
LibraryControl
-> m_pLibraryWidget
-> active view: must be a WTrackTableView
-> toggleBpmLockOfSelectedTracks

you may also take a loot at the helper WTrackMenu::getTrackBpmLockStates()

@FrankwaP
Copy link
Contributor Author

Thank you!

Can you please take care of the documentation in https://manual.mixxx.org/2.5/chapters/appendix/mixxx_controls.html ?

Please see: mixxxdj/manual#637

@FrankwaP
Copy link
Contributor Author

In case this shall be adopted for the library later on we will also make do with just one control…

I'm not sure I understand: how is it supposed to be different from the command which toggles lock/unlock when clicking the lock icon in the track field?

BTW I've just discover the "auto unlock" when one selects a locked track with no BPM. This feels super weird, like it's a bug… why not preventing from locking in the first place?

@ronso0
Copy link
Member

ronso0 commented Apr 12, 2024

I'm not sure I understand: how is it supposed to be different from the command which toggles lock/unlock when clicking the lock icon in the track field?

It would toggle lock for all selected tracks at once.
That control probably doesn't make much sense for the 'controller only' workflow (selecting multiple tracks still requires pressing Shift on the keyboard), but --considering the track management controls that were requested/implemented recently-- it might make sense for 'mouse + keyboard' and 'controller + keyboard' workflows: Tracks knobs usually send key Up/Down controls to the focused widget and holding Shift on the keyboard meanwhile is possible.
(theoretically, need to test this)

@daschuer
Copy link
Member

@FrankwaP Thank you for your contributions. I would like to add you to the contributor list at the Mixxx about box. Shall I use your full name or a nick name?

@FrankwaP
Copy link
Contributor Author

FrankwaP commented Apr 16, 2024

Thank you for taking time to explain the logic for the multi-track toggle :-)
I will look into it next week. Have a great day!

I would like to add you to the contributor list at the Mixxx about box. Shall I use your full name or a nick name?

Thanks for the attention :-) you can use FrankwaP.

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.

3 participants