Consistently use seconds instead of milliseconds throughout playback module #23794
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
8 tasks
cbjeukendrup
force-pushed
the
playbackcontroller_msecs
branch
from
1d92504 to
5c955eb
Compare
RomanPudashkin
approved these changes
Jul 29, 2024
|
@cbjeukendrup Tested on Win10. There is a regression comparing with master (and 4.3.2)
bandicam.2024-07-29.15-59-43-313.mp4 |
cbjeukendrup
force-pushed
the
playbackcontroller_msecs
branch
from
5c955eb to
b465fde
Compare
|
@DmitryArefiev Oops, there was one letter I forgot to remove 😄 But now it should be better! |
|
@cbjeukendrup All fine now. But I found another bug with measure counter in the playback panel (also regression from 4.3.2). Do you want to fix in this PR? :) Just try to input 123 (in big score): bandicam.2024-07-30.17-19-21-822.mp4 |
…module Resolves musescore#23793: the time input field in the playback toolbar was not working properly, because the amount of milliseconds resulting from the entered time was accidentally interpreted as a number of ticks, because the wrong overload of `seek` was used. This problem is avoided by eliminating the usage of the msecs_t type completely.
cbjeukendrup
force-pushed
the
playbackcontroller_msecs
branch
from
b465fde to
30d34af
Compare
|
@DmitryArefiev I found a fix for that too! @RomanPudashkin please check if it looks good :) |
PlaybackToolbarModel emitted a "playback position changed" signal in response to notifications on the `playbackState->playbackPositionChanged()` channel. `PlaybackController` also listens to this channel, and uses it to update its `m_currentTick` value. The value is used to determine the current measure/beat position in PlaybackToolbar. The problem: the order in which channel listeners are invoked is undefined. And in this case we are unlucky: PlaybackToolbarModel's listener is invoked first, so it updates the QML view; and only after that, PlaybackController's listener is invoked, so its `m_currentTick` is updated too late. So, the QML view is updated based on the old `m_currentTick`. Solution: don't listen to `playbackState->playbackPositionChanged()` directly, but listen to a channel from PlaybackController. This channel sends the notification after updating `m_currentTick`. This commit also updates the name and signature of this channel, to better reflect its purpose. This problem was a regression, maybe caused by 8b95896, but that is an unverified conjecture.
cbjeukendrup
force-pushed
the
playbackcontroller_msecs
branch
from
30d34af to
2f7f138
Compare
RomanPudashkin
approved these changes
Jul 31, 2024
|
@cbjeukendrup Yeah, all fixed! Tested #23793 on Win10, Mac13.6, LinuxUbuntu22.04 - FIXED |
4 tasks
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves: #23793
The time input field in the playback toolbar was not working properly, because the amount of milliseconds resulting from the entered time was accidentally interpreted as a number of ticks, because the wrong overload of
seekwas used. This problem is avoided by eliminating the usage of the msecs_t type completely.Will conflict with #19038; it doesn't really matter which one is merged first.