Add Double BPM and Halve BPM to contextual menu

[Osoitz]

Automatically detected BPM is often half or double of real BPM. In the song properties dialogue one can correct this with a click, but it would be handy to have it available in the contextual menu too.

Also, if the song does not have a 4/4 rhythm but 3/4 the correct BPM is often two thirds of the detected BPM, it would be great to have this option too.

Since this means adding lots of things to the contextual menu, it might be better to put all the BPM related options under a submenu:

BPM >
          Double BPM
          Halve BPM
          2/3 x BPM
          ---
          Lock BPM
          Unlock BPM
          Clear BPM and Grid

[RJ Skerry-Ryan (rryan)]

See src/widget/wtracktableview.cpp to see where the context menu is created. "Reset Beatgrid" is a good action to model this after.

[Callum Styan (callumstyan)]

Here is a patch for this wishlist item.

I've added new actions for Double, Halve, and 2/3 BPM as requested, and renamed
one of the TrackModel capabilities (BPMLOCK) to now account for functionality related
to beat manipulation (BPM locking, doulbing, etc.).

[Callum Styan (callumstyan)]

My previous diff. did not contain all changes I'd made, here is the correct version.

[Max Linke (max-linke)]

Hi Callum

I just looked over your changes. Here are some comments

- include 'lock BPM' again
- if the BPM is locked the all options to change the BPM should be grayed out (see m_pClearBeatsAction)
- I think all this BPM stuff could go into a submenu like playlists. We have a lot of option there right now and this
  could get cramped on small screens
- you've written in a TODO that the changing of the BPM should be done in a seperate thread. Did you notice a long
  GUI-freeze when you worked on a large selection of tracks?

[Callum Styan (callumstyan)]

Hi Max

I'll be submitting another patch today with some changes but here are answers for a few of your comments:

- lock BPM wasn't removed, lock/unlock BPM and the 3 new settings are now all in a submenu of the context menu
- you're right I should grey the options out if the BPM is locked
- the other slot functions contain that TODO comment, I left it in for consistency's sake because there is the possibility that selecting a large number of tracks could lock the UI, but at the moment none of the slot functions make use of a seperate thread maybe that should be another wishlist item?

[Callum Styan (callumstyan)]

Here's an updated version of the patch.

I added the new BPM actions and menu to the deconstructor, and added all the BPM related actions into one BPM submenu. Finally, I made it so that if the BPM is locked then the Double, Halve, and 2/3 BPM items are greyed out.

As I mentioned above, I added the TODO comment which was present in most of the other 'slot' functions which acted on multiple selected items. I added the comment for consistency's sake, not because I experienced any UI lock issues. Perhaps the threading of these should be added as another wishlist item?

[Max Linke (max-linke)]

Hcan you please create the patch in your mixxx directory with

bzr diff > bpm.patch

so that I can directly apply it. With your current version I need to
edit a lot by hand so that I can apply it to trunk.

On Mon, 13 May 2013 17:36:07 -0000
Callum Styan <email address hidden> wrote:

> Here's an updated version of the patch.
>
> I added the new BPM actions and menu to the deconstructor, and added
> all the BPM related actions into one BPM submenu. Finally, I made it
> so that if the BPM is locked then the Double, Halve, and 2/3 BPM
> items are greyed out.
>
> As I mentioned above, I added the TODO comment which was present in
> most of the other 'slot' functions which acted on multiple selected
> items. I added the comment for consistency's sake, not because I
> experienced any UI lock issues. Perhaps the threading of these
> should be added as another wishlist item?
>
> ** Attachment added: "updated diff."
> https://bugs.launchpad.net/mixxx/+bug/1088031/+attachment/3675504/+files/diff2.txt
>

[Callum Styan (callumstyan)]

Hi Max, I made commits (bzr commit) to my local repo. while writing the patch, so a bzr diff will only show my most recent changes. I'll try see if I can get a better diff. for you today, otherwise I will manually edit the other one for you.

[Max Linke (max-linke)]

bzr diff --old lp:mixxx > bpm.patch

On Tue, 14 May 2013 21:51:51 -0000
Callum Styan <email address hidden> wrote:

> Hi Max, I made commits (bzr commit) to my local repo. while writing
> the patch, so a bzr diff will only show my most recent changes. I'll
> try see if I can get a better diff. for you today, otherwise I will
> manually edit the other one for you.
>

[Daniel Schürmann (daschuer)]

Hi Callum

bzr help diff
is your friend ;-)

You may looking for

bzr diff -r1 > double_bpm.patch

An other approach is to publish you branch and issue a merge request.
Advantage:
* Your commit history will be preserved
* Your will find your name as committer.

bzr launchpad-login callumstyan
bzr push lp:~callumstyan/mixxx/branch-name

[Daniel Schürmann (daschuer)]

bzr diff --old lp:mixxx > bpm.patch
will work as well but there is the risk that new commits to lp:mixxx will get reverted unintendedly.

[Callum Styan (callumstyan)]

Thank you Max and Daniel! I'd looked at the documentation for bzr diff but was unable to find the solution I was looking for. This is the first time I have used bzr :)

Max, here is the diff., hopefully this works for you! :)

[Max Linke (max-linke)]

Have you tried what happens if you call the BPM menu when you select several tracks and one is locked?

The desired behaviour is that then all but the unlock button is grayed out.

[Max Linke (max-linke)]

And you could also make the lock the first option in the submenu, so that people do not accidently click double BPM

[Callum Styan (callumstyan)]

I haven't tried that. I'll look into it and make the menu item ordering change today.

[Callum Styan (callumstyan)]

Fixed the menu ordering. Confirmed that the menu isn't behaving as desired, I'll fix that today.

[Callum Styan (callumstyan)]

Hi Max, I've been trying to get the disabling of the menu items to work properly but I'm a bit stuck.

It seems I don't understand how exactly the track indicies work, as what I've been trying hasnt worked.
On line 585 of wtracktableview.cpp there is: if (index.data().toBool()){ inside of which bpmLock is disabled, and and else statement which does the opposite.

From what I gathered, index.data().toBool() was a boolean value for if the bpm lock was set on the current index. However, when I tried to use this as a condition for checking all selected indicies to see if any were locked, it did not work as I expected. Even if none of the selected indicies are locked I get into the condition for one being locked.

[Max Linke (max-linke)]

Try to have a look how it is handled for "properties" the last entry in
the menu and also how we handle the reset BPM case.
Check for allowClear and oneSelected in the code from trunk

best Max

On Mon, 20 May 2013 18:39:41 -0000
Callum Styan <email address hidden> wrote:

> Hi Max, I've been trying to get the disabling of the menu items to
> work properly but I'm a bit stuck.
>
> It seems I don't understand how exactly the track indicies work, as
> what I've been trying hasnt worked. On line 585 of
> wtracktableview.cpp there is: if (index.data().toBool()){ inside of
> which bpmLock is disabled, and and else statement which does the
> opposite.
>
> >From what I gathered, index.data().toBool() was a boolean value for
> >if
> the bpm lock was set on the current index. However, when I tried to
> use this as a condition for checking all selected indicies to see if
> any were locked, it did not work as I expected. Even if none of the
> selected indicies are locked I get into the condition for one being
> locked.
>

[Callum Styan (callumstyan)]

Hi Max,
if I handle it the same way as in the 'Clear Beats' case it seems to work, somewhat. If I lock one song, and then select more the context menu will then show only unlock. However if I then unlock that one song, the menu will still only display unlock. If all selections are unselected and then reselected it updates as intended.

I noticed this on some other sections of the context menu as well, including the only lock/unlock portion of the menu. This seems to be because of conditions that only have an if case, and disable certain menu actions. An else case used to enable them fixes this.

I'm just cleaning up this new fix and then I will post it sometime tonight.

[Callum Styan (callumstyan)]

Tested multiple songs with one locked, menu items are grayed out as intended.

[Max Linke (max-linke)]

I commited your patch.

[Be (be.ing)]

It would be convenient to bring up this menu when right clicking the BPM on the deck. Searching for a track already in a deck in the library to right click on it and change the BPM is cumbersome.

[Daniel Schürmann (daschuer)]

Thank you for the request.
Please check if this request is a duplicate of Bug #1277689 Right-click context menu for track text widgets.
You may click that it effects you, or add valuable info if you have any.

[Swiftb0y (swiftb0y)]

Mixxx now uses GitHub for bug tracking. This bug has been migrated to:
https://github.com/mixxxdj/mixxx/issues/6754