update beatloop size for loops set manually with IN/OUT

[ronso0]

This updates the beatloop_size for loops set manually with IN/OUT, which also enables loop_halve/_double.
This is on top of #12509, so check only the last two commits.

That was discussed in #1187 and rejected for some reasons (or rather concerns and assumptions IMO).
I disabled the respective tests.

Fixing #10511 in #12509 made the previous inconsistency obvious (#12509 (comment)).

The second commit tries to address the concerns raised in #1187:
When a track with an unusual loop size is ejected, that loop size is kept (now not only for saved loops but also for temp. loops).
If a track with no loop is loaded, beatloop_size is reset to default (4 beats), or, if there's a saved loop, its size is adopted.

What do you think?
(I know it's a bit late for such a change)

[ywwg]

Yeah this is a tough one -- it feels like a lot of risky change so close to release. I feel like our test coverage is pretty good for beatloop size stuff though? In my opinion, given the bugs you are describing, they seem pretty minor and cosmetic. There is some annoyance, but the current experience is not that bad. Whereas, if there are bugs introduced here, we risk breaking loops which is a major feature.

My position would be to save this fix for the next patch release.

[ronso0]

You're right, this should go to 2.4.1
I'm pretty confident though this doesn't break anything, actually it makes workflows consistent. I started with #12509 to fix a minor bug but this grew a bit beyond a small bugfix due to the consequneces / other bug it revealed.

[ronso0]

It is not clear to me why LoopingControlTest, LoopInOutButtons_QuantizeEnabled fails with this change.

Furthermore, if I do the test steps manually with 2.4 I don't get the expected result:

• enable quantize
• set playpos slightly after a beat
• set IN
  = ✔️ IN snaps to beat, playpos is unchanged
• move forward by 4 beats
  = ✔️ playpos is slightly after the beat
• set OUT
  = ✔️ OUT snaps to beat
  = (loop is activated)
  = ❌ playpos jumps to [loop_in + playpos/beat offset] (which feels correct)
  expected: playpos remains after the beat

[ronso0]

I feel like our test coverage is pretty good for beatloop size stuff though?

Yes. And no, see #12522 (comment)
I assume the test succeed because no visuals are involved?
IMO Mixxx behaves correctly, but the test expects behaviour that deosn't make sense to me (why not jump inside the loop like with other loop activation controls?).

Can anyone confirm my observation, or does it behave as expected?

[ywwg]

expected: playpos remains after the beat

confirmed this is a bad expectation in the test and that we have a bug -- if there is an existing inactive loop, setting an out point just after a beat creates a loop but does not cause a seek into the loop. There is a missing seek-inside-loop check somewhere

[ronso0]

The second commit tries to address the concerns raised in #1187:
When a track with an unusual loop size¹ is ejected, that loop size is kept (now not only for saved loops but also for temp. loops).
If a track with no loop is loaded, beatloop_size is reset to default (4 beats), or, if there's a saved loop, its size is adopted.
¹a loop size that's not in the list of 2^n sizes

and quoting from #12509
I'd even vote for not adopting any loop size after track load. I don't understand why loop halve/double should immediatly affect the existing loop (temp or saved), that's unhandy if I just want to preprare a new loop to be activated with beatloop_activate.
We can keep the previous loop size if it is in the list of 2^n sizes, else reset to the default value (4).

[github-actions]

This PR is marked as stale because it has been open 90 days with no activity.

[ronso0]

Rebased onto main, will do manual test soonish.