Skip to content

DlgTrackInfo: add BPM lock toggle to BPM tab - #16025

Merged
ronso0 merged 1 commit into
mixxxdj:mainfrom
Swarnadip-Kar:add_bpm_lock_to_dlgtrackinfo
Mar 9, 2026
Merged

DlgTrackInfo: add BPM lock toggle to BPM tab#16025
ronso0 merged 1 commit into
mixxxdj:mainfrom
Swarnadip-Kar:add_bpm_lock_to_dlgtrackinfo

Conversation

@Swarnadip-Kar

@Swarnadip-Kar Swarnadip-Kar commented Feb 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a Lock/Unlock BPM button to the BPM tab in the Track Editor dialog (DlgTrackInfo).

Problem

When BPM is locked, the entire BPM tab is disabled with no way to unlock it from within the Track Editor. Users must close the dialog and use the track context menu to unlock BPM.

Changes

  • Added a QPushButton (bpmLock) to the BPM tab in dlgtrackinfo.ui
  • Added slotBpmLockClicked() slot to handle button clicks
  • Modified reloadTrackBeats() to individually disable editing controls instead of the whole tab
  • Lock button text dynamically updates to "Lock BPM" / "Unlock BPM"

How to test

  1. Open Mixxx and load a track into the library
  2. Right-click the track → Properties → BPM tab
  3. Click "Lock BPM" → all editing controls should become disabled
  4. Click "Unlock BPM" → all editing controls should become enabled again
  5. Verify the button state persists when navigating between tracks

Before Screenshot

Before Screnshot

After Video

Lock_Unlock.BPM.-.Track.Editor.dialog.mp4

@Swarnadip-Kar Swarnadip-Kar changed the title DlgTrackInfo: add BPM lock/unlock toggle to BPM tab Add BPM lock/unlock toggle to BPM tab Feb 22, 2026
@Swarnadip-Kar Swarnadip-Kar changed the title Add BPM lock/unlock toggle to BPM tab DlgTrackInfo: add BPM lock toggle to BPM tab Feb 22, 2026
@Swarnadip-Kar
Swarnadip-Kar marked this pull request as draft February 22, 2026 12:04
@Swarnadip-Kar
Swarnadip-Kar marked this pull request as ready for review February 22, 2026 14:01

@ronso0 ronso0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for working on this!
Though it needs a slight rework, see my comments

Comment thread src/library/dlgtrackinfo.cpp Outdated
Comment thread src/library/dlgtrackinfo.cpp Outdated
Comment thread src/library/dlgtrackinfo.ui Outdated
@Swarnadip-Kar
Swarnadip-Kar force-pushed the add_bpm_lock_to_dlgtrackinfo branch from 8faf463 to 0bc0301 Compare February 23, 2026 11:17
@Swarnadip-Kar

Copy link
Copy Markdown
Contributor Author

Thanks for the review @ronso0! I've addressed all three points:

  • Removed checkable property from the bpmLock button in the .ui file
  • Added m_bpmLocked bool member to track the lock state locally instead of relying on button check state
  • Deferred applying the lock — setBpmLocked() now only happens in saveTrack() (i.e. on Apply/OK)

Also renamed updateBpmLockButton() to updateBpmEditControls() to better reflect its purpose, and connected to clicked() instead of toggled().

@Swarnadip-Kar
Swarnadip-Kar requested a review from ronso0 February 23, 2026 11:28
@Swarnadip-Kar
Swarnadip-Kar force-pushed the add_bpm_lock_to_dlgtrackinfo branch from 0bc0301 to 6f58ca1 Compare February 23, 2026 11:37
@Swarnadip-Kar

Copy link
Copy Markdown
Contributor Author

Realized I accidentally removed the QSignalBlocker header — it's still used elsewhere in the file. Added it back in the same commit.

@ronso0 ronso0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you, I left some comments

Comment thread src/library/dlgtrackinfo.cpp Outdated
Comment thread src/library/dlgtrackinfo.cpp Outdated
Comment thread src/library/dlgtrackinfo.cpp Outdated
Comment thread src/library/dlgtrackinfo.cpp Outdated
Comment thread src/library/dlgtrackinfo.h Outdated
Comment thread src/library/dlgtrackinfo.ui Outdated
@Swarnadip-Kar

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review @ronso0! Addressed all 6 points in the
new commit:

  • Removed the two explanatory comments in init() and saveTrack()
  • Fixed the BPM lock to go through m_trackRecord.setBpmLocked()
    instead of directly on m_pLoadedTrack, consistent with how all
    other properties are saved
  • Removed comment above slotBpmLockClicked()
  • Moved m_bpmLocked initialization from header to constructor
  • Removed hardcoded "Lock BPM" text from .ui file to avoid
    duplicate tr() strings

Let me know if anything else needs changing!

@Swarnadip-Kar
Swarnadip-Kar requested a review from ronso0 February 25, 2026 06:32
@Swarnadip-Kar

Copy link
Copy Markdown
Contributor Author

I noticed the lock button no longer turns blue/highlighted
when BPM is locked — the checkable property was lost in the refactor.

Fix would be re-adding checkable: true to the .ui and syncing with
bpmLock->setChecked(m_bpmLocked) in updateBpmEditControls().
Should I add this as a commit here?

@ronso0

ronso0 commented Feb 25, 2026

Copy link
Copy Markdown
Member

IMO we don't need the button state/highlight. The BPM controls being greyed out and the button text are sufficient.
An icon would be nice, like we have it in the beatgrid controls section.
We may add that later on.

@ronso0 ronso0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few last cleanup requests (sorry, didn't check the ctor and overlooked the other one earlier)

Comment thread src/library/dlgtrackinfo.cpp Outdated
Comment thread src/library/dlgtrackinfo.cpp Outdated
@Swarnadip-Kar
Swarnadip-Kar force-pushed the add_bpm_lock_to_dlgtrackinfo branch from 38a5d23 to 028dc25 Compare February 26, 2026 05:31
@Swarnadip-Kar
Swarnadip-Kar requested a review from ronso0 February 26, 2026 05:34
@Swarnadip-Kar

Copy link
Copy Markdown
Contributor Author

All review comments addressed:

-Remove m_bpmLocked init from ctor since it is set in
reloadTrackBeats() before it is read, like m_trackHasBeatMap.
-Remove editingEnabled intermediate variable in
updateBpmEditControls() and inline !m_bpmLocked directly."

Thanks for the review, @ronso0!

@Swarnadip-Kar

Swarnadip-Kar commented Mar 4, 2026

Copy link
Copy Markdown
Contributor Author

The CI failure in appstreamcli validate seems unrelated to my changes — it's flagging a 403 on the donate URL and a missing content rating in org.mixxx.Mixxx.metainfo.xml, both of which don't appear to be related to my changes. Is this expected and safe to ignore?
@ronso0 — gentle ping when you get a chance!

Comment thread src/library/dlgtrackinfo.cpp Outdated
Comment thread src/library/dlgtrackinfo.cpp Outdated
// This will only be written back to the track on Apply/OK.
m_bpmLocked = track.isBpmLocked();

tabBPM->setEnabled(true);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not needed here anymore since we now have updateBpmEditControls() (and wrong if BPM are locked)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — removed tabBPM->setEnabled(true) since updateBpmEditControls() already handles that correctly.

@ronso0

ronso0 commented Mar 4, 2026

Copy link
Copy Markdown
Member

Looks good otherwise 👍

@ronso0

ronso0 commented Mar 4, 2026

Copy link
Copy Markdown
Member

Yes, the CI fail is unrelated. The site seems to be n/a temporarily, or probably just requires a more relaxed timeout, idk.

@Swarnadip-Kar

Copy link
Copy Markdown
Contributor Author

Yes, the CI fail is unrelated. The site seems to be n/a temporarily, or probably just requires a more relaxed timeout, idk.

Thanks for confirming!

@Swarnadip-Kar

Copy link
Copy Markdown
Contributor Author

Addressed review comments — removed tabBPM->setEnabled(true) since updateBpmEditControls() already handles that correctly, and updated the doc comment to use ///.

@Swarnadip-Kar
Swarnadip-Kar requested a review from ronso0 March 4, 2026 10:25
@ronso0

ronso0 commented Mar 4, 2026

Copy link
Copy Markdown
Member

Alright, now please squash the commits and we're ready to roll!

Btw how much ai did you use for this PR? I just noticed the extensive commit messages and wanna let you know that we don't need those for rather small commits like this one. If anyone is interested, either the commit itself should be clear and if not, they can look up all details/discussion in the PR as that is shown in the git history after merging it.

@Swarnadip-Kar

Swarnadip-Kar commented Mar 4, 2026

Copy link
Copy Markdown
Contributor Author

Alright, now please squash the commits and we're ready to roll!

Squashing now..

Btw how much ai did you use for this PR?

In My inital days I used AI to understand the Codebase and find the right files to edit.
Commit messages were AI-assisted for formatting. The code and debugging was mine. Will keep commit messages shorter going forward, noted!

@Swarnadip-Kar
Swarnadip-Kar force-pushed the add_bpm_lock_to_dlgtrackinfo branch from c346c55 to 8eb0d14 Compare March 4, 2026 11:22
@Swarnadip-Kar

Swarnadip-Kar commented Mar 4, 2026

Copy link
Copy Markdown
Contributor Author

@ronso0 Squashed and force-pushed

@Swarnadip-Kar

Copy link
Copy Markdown
Contributor Author

@ronso0 did this slip from cache? 😄

@ronso0

ronso0 commented Mar 6, 2026

Copy link
Copy Markdown
Member

Naa, just other stuff going on. I see you're motivated and eager to get this merged, but priorities shift on a daily basis, so please be patient.
I've planned to do one more thoutough review and test it during the weekend, monday latest.

@Swarnadip-Kar

Swarnadip-Kar commented Mar 7, 2026

Copy link
Copy Markdown
Contributor Author

Naa, just other stuff going on. I see you're motivated and eager to get this merged, but priorities shift on a daily basis, so please be patient. I've planned to do one more thoutough review and test it during the weekend, monday latest.

Of course, take your time! Really appreciate it

@ronso0 ronso0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, except the indentation.
Please fix that, then we can merge.

Comment thread src/library/dlgtrackinfo.ui Outdated
@Swarnadip-Kar
Swarnadip-Kar requested a review from ronso0 March 9, 2026 13:18
@ronso0

ronso0 commented Mar 9, 2026

Copy link
Copy Markdown
Member

Thanks, now squash & force-push please ; )

@Swarnadip-Kar
Swarnadip-Kar force-pushed the add_bpm_lock_to_dlgtrackinfo branch from 2ff5bc6 to 6af523c Compare March 9, 2026 14:25
@Swarnadip-Kar

Copy link
Copy Markdown
Contributor Author

Thanks, now squash & force-push please ; )

Squashed and force-pushed!

@ronso0

ronso0 commented Mar 9, 2026

Copy link
Copy Markdown
Member

Thanks, now squash & force-push please ; )

Squashed and force-pushed!

Yes, but you also rebased onto main it seems. Which leads to a potentially huge Compare view where the actual PR changes (there should be none when squashing) are hard to spot. Not a big deal for this PR.
Next time just rebase onto [first commit]^1 to have a clean Compare view.

@ronso0
ronso0 enabled auto-merge March 9, 2026 14:35
@Swarnadip-Kar

Copy link
Copy Markdown
Contributor Author

Yes, but you also rebased onto main it seems. Which leads to a potentially huge Compare view where the actual PR changes (there should be none when squashing) are hard to spot. Not a big deal for this PR.
Next time just rebase onto [first commit]^1 to have a clean Compare view.

Thanks for the tip! Will research and do better next time ...

@ronso0
ronso0 merged commit 68ba17a into mixxxdj:main Mar 9, 2026
17 checks passed
Swarnadip-Kar added a commit to Swarnadip-Kar/mixxx that referenced this pull request Mar 9, 2026
Rebased onto main after BPM lock toggle merge (mixxxdj#16025).
Ensures 5/4 and 4/5 scaling buttons respect the BPM lock
state, consistent with existing scaling controls.
@Swarnadip-Kar
Swarnadip-Kar deleted the add_bpm_lock_to_dlgtrackinfo branch March 28, 2026 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants