Fix #16519: L/R Arrow During Playback Moves From Playhead Current Location

[22justinl]

• Left/Right Arrow: Move between beats
• Left/Right Arrow + Ctrl/Command: Move between measures

Resolves: #16519

Changed so that left and right arrows during playback move the playback cursor instead of the selection (which causes the playback cursor to move back to the selection)

• I signed the CLA
• The title of the PR describes the problem it addresses
• Each commit's message describes its purpose and effects, and references the issue it resolves
• If changes are extensive, there is a sequence of easily reviewable commits
• The code in the PR follows the coding rules
• There are no unnecessary changes
• The code compiles and runs on my machine, preferably after each commit individually
• I created a unit test or vtest to verify the changes I made (if applicable)

[RomanPudashkin]

@22justinl please rebase this PR. Thanks!

[zacjansheski]

I think this change is problematic with repeats

Here is a video from the PR

video1399164880.mp4

[zacjansheski]

Here is a video from current 4.2

video1231836572.mp4

[cbjeukendrup]

@22justinl Kind reminder about the comments above :)
If you don't have time anymore to fix them, that's alright; in that case we'll close this PR, so that a different contributor can continue your work.
Please let us know!

[cbjeukendrup]

This method also looks very questionable; it's completely unclear whether it expects raw or played ticks. If you want, you could figure that out and include a fix in a separate commit, but that's absolutely not a must. Perhaps it's better to save that for a different PR.

[22justinl]

Would something like seekPlayedTick be fine?

Also, I made PlaybackController::seek public again (it was made private in #23011) but I'm now realizing I could instead keep it private and make a new public function (something like PlaybackController::seekBeat) to be called by NotationActionController::move. Would that be better?

[cbjeukendrup]

I'm not per se against making seek public (it can already be accessed anyway via the more cumbersome way of calling dispatcher()->dispatch("rewind", ………)). But introducing a seekBeat method might be even better indeed. Perhaps it's nice to put it just below seekElement.

[cbjeukendrup]

Would something like seekPlayedTick be fine?

Judging from how m_currentTicks is used, it looks like that is actually raw ticks. So I would assume this method is rather seekRawTick, and thus should use rawTickToSecs.

[22justinl]

Hm, wouldn't it be played ticks since, for example, PlaybackController::seekElement uses a value from NotationPlayback::playPositionTickByElement, which returns played ticks?

[cbjeukendrup]

I researched it a bit more, and found a bug which I logged as #23795.

m_currentTicks is indeed a raw tick, but this function works with played ticks. However, it turns out that in all cases where this function is called, the value that will become the argument of this function is a raw tick, but it is then converted to a played tick before calling the function.

So, the solution would be to make this function receive raw ticks, and adjust the callers to remove the conversion to played ticks.

There should be exactly three callers: seekElement, seekListSelection, and setTempoMultiplier.
In the current master branch, there is a fourth one though: rewind. It tries to pass milliseconds, instead of seconds or ticks. There is no overload for milliseconds, so this (wrongly) uses the ticks overload. However, you don't need to worry about this, because I fixed it in #23794.