fix loop_remove

[ronso0]

This fixes two bugs with the loop cue (temp loop not attached to a hotcue):

1)
Appearantly loop_remove #4802 did only work for the first loop that was ever created for a track.
If a track already has a loop cue that was not cleared.
(and it seems Track menu > Reset > Loops only clears the temp. loop because Track emits cuesUpdated())

I fixed this in BaseTrackPlayerImpl::unloadTrack() rather than in LoopingControl::slotLoopRemove(), simply because that function already reads the loop positions and the loop cue.
Nope, that's not sufficient, clear it in slotRemoveLoop so it is not restored when the unsaved track is be loaded to another player.

2)
Currently, when an existing temp. loop is invalidated (set loop_in after loop_out) the loop cue is not cleared, i.e. the previous temp loop is restored next time the track is loaded.

---

The third commit changes LateNight so this can be tested easily. You may put that on top of 2.4 to confirm the bugs:
1)

• load a track that has a loop set (or load a track without loop, set a loop, eject, reload)
• click the Slip button (or trigger loop_remove otherwise)
  = loop is removed
• eject
• reload
  = loop is restored

2)

• load a track that has a loop set (or load a track without loop, set a loop, eject, reload)
• set loop_in after loop_out
  = loop is removed
• eject
• reload
  = previous loop is restored
  Draft until that test commit is removed.

[ywwg]

thank you for this

[ronso0]

Please review and let me know when I can remove the skin test commit.

[daschuer]

Just a first comment, the manual test is pending.

[daschuer]

It works, thank you.

The reloop button does not work, clear. I pressed it expecting an undo feature without thinking, because in case of a saved loop, there is not removing the in and out markers is barely visible.
Is this an issue?

[ronso0]

I pressed it expecting an undo feature without thinking, because in case of a saved loop, there is not removing the in and out markers is barely visible.
Is this an issue?

I don't quite understand, can you elaborate please?

Thing is, loop_remove is only available via the control (there's no GUI button in the official skins¹), so I assume it'll barely be used anyway until we add that as an action to the track menu, so I doubt anyone expects an undo feature.

¹And tbh I don't know where to put it. Loop, reloop, In and Out already have a right-click connection.

[daschuer]

OK, so let's merge. Thank you.

[ronso0]

And the TEST commit slipped in again.
Now reverting it, and merging 2.4 into main.

What's the best strategy to avoid this in the future?
a) leave in draft state until fixups are squashed / TEST commits are removed, and ping reviewers repeatedly to get attention
b) set the 'changes requested' flag and hope it (the reason) is not overlooked before merging

What do you think?

[daschuer]

Oh sorry.
How about setting up a second Draft branch with the test commit? This will also release you from the work of touching the branch after review.
The 'changes requested' will probably also work.

[ronso0]

Draft branch with the test commit? This will also release you from the work of touching the branch after review.

I see your point, but that requires rebasing the TEST branch in case the base branch receives fixes, e.g. applying requested changes, and reviewers can not be sure they the TEST branch is in sync.

Therefore I'd prefer 'Changes requested'. The 'Compare' button after force-push is our friend for this case: it's easy to verify the force-push only reverted skin changes / removed debug output / fixed typos etc.

[daschuer]

It is finally up to you. I personally consider it more work to rebase after review compared to keeping two branches during development, just because the developer and the reviewer need to find an extra time slot after the successful review.