Skip to content

Commit

Permalink
Fix updating the measure/beat field in PlaybackToolBar
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
cbjeukendrup committed Jul 30, 2024
1 parent db9537d commit 2f7f138
Show file tree
Hide file tree
Showing 8 changed files with 12 additions and 14 deletions.
2 changes: 1 addition & 1 deletion src/notation/view/abstractnotationpaintview.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -622,7 +622,7 @@ void AbstractNotationPaintView::onNotationSetup()
onPlayingChanged();
});

playbackController()->midiTickPlayed().onReceive(this, [this](uint32_t tick) {
playbackController()->currentPlaybackPositionChanged().onReceive(this, [this](audio::secs_t, midi::tick_t tick) {
movePlaybackCursor(tick);
});

Expand Down
6 changes: 3 additions & 3 deletions src/playback/internal/playbackcontroller.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -227,9 +227,9 @@ void PlaybackController::seek(const audio::secs_t secs)
currentPlayer()->seek(secs);
}

Channel<uint32_t> PlaybackController::midiTickPlayed() const
muse::async::Channel<secs_t, tick_t> PlaybackController::currentPlaybackPositionChanged() const
{
return m_tickPlayed;
return m_currentPlaybackPositionChanged;
}

TrackSequenceId PlaybackController::currentTrackSequenceId() const
Expand Down Expand Up @@ -1264,7 +1264,7 @@ void PlaybackController::setupSequencePlayer()
{
currentPlayer()->playbackPositionChanged().onReceive(this, [this](const audio::secs_t pos) {
m_currentTick = notationPlayback()->secToTick(pos);
m_tickPlayed.send(m_currentTick);
m_currentPlaybackPositionChanged.send(pos, m_currentTick);

updateCurrentTempo();

Expand Down
4 changes: 2 additions & 2 deletions src/playback/internal/playbackcontroller.h
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class PlaybackController : public IPlaybackController, public muse::actions::Act

void reset() override;

muse::async::Channel<uint32_t> midiTickPlayed() const override;
muse::async::Channel<muse::audio::secs_t, muse::midi::tick_t> currentPlaybackPositionChanged() const override;

muse::audio::TrackSequenceId currentTrackSequenceId() const override;
muse::async::Notification currentTrackSequenceIdChanged() const override;
Expand Down Expand Up @@ -212,7 +212,7 @@ class PlaybackController : public IPlaybackController, public muse::actions::Act
muse::async::Notification m_isPlayingChanged;
muse::async::Notification m_totalPlayTimeChanged;
muse::async::Notification m_currentTempoChanged;
muse::async::Channel<uint32_t> m_tickPlayed;
muse::async::Channel<muse::audio::secs_t, muse::midi::tick_t> m_currentPlaybackPositionChanged;
muse::async::Channel<muse::actions::ActionCode> m_actionCheckedChanged;

muse::audio::TrackSequenceId m_currentSequenceId = -1;
Expand Down
2 changes: 1 addition & 1 deletion src/playback/iplaybackcontroller.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ class IPlaybackController : MODULE_EXPORT_INTERFACE

virtual void reset() = 0;

virtual muse::async::Channel<uint32_t> midiTickPlayed() const = 0;
virtual muse::async::Channel<muse::audio::secs_t, muse::midi::tick_t> currentPlaybackPositionChanged() const = 0;

virtual muse::audio::TrackSequenceId currentTrackSequenceId() const = 0;
virtual muse::async::Notification currentTrackSequenceIdChanged() const = 0;
Expand Down
2 changes: 1 addition & 1 deletion src/playback/tests/mocks/playbackcontrollermock.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ class PlaybackControllerMock : public IPlaybackController

MOCK_METHOD(void, reset, (), (override));

MOCK_METHOD(muse::async::Channel<uint32_t>, midiTickPlayed, (), (const, override));
MOCK_METHOD((muse::async::Channel<muse::audio::secs_t, muse::midi::tick_t>), currentPlaybackPositionChanged, (), (const, override));

MOCK_METHOD(muse::audio::TrackSequenceId, currentTrackSequenceId, (), (const, override));
MOCK_METHOD(muse::async::Notification, currentTrackSequenceIdChanged, (), (const, override));
Expand Down
4 changes: 1 addition & 3 deletions src/playback/view/playbacktoolbarmodel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,6 @@ void PlaybackToolBarModel::load()

void PlaybackToolBarModel::setupConnections()
{
context::IPlaybackStatePtr playbackState = globalContext()->playbackState();

playbackController()->isPlayAllowedChanged().onNotify(this, [this]() {
emit isPlayAllowedChanged();
});
Expand All @@ -77,7 +75,7 @@ void PlaybackToolBarModel::setupConnections()
onActionsStateChanges({ PLAY_ACTION_CODE });
});

playbackState->playbackPositionChanged().onReceive(this, [this](secs_t secs) {
playbackController()->currentPlaybackPositionChanged().onReceive(this, [this](secs_t secs, midi::tick_t) {
updatePlayPosition(secs);
});

Expand Down
4 changes: 2 additions & 2 deletions src/stubs/playback/playbackcontrollerstub.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ void PlaybackControllerStub::reset()
{
}

muse::async::Channel<uint32_t> PlaybackControllerStub::midiTickPlayed() const
muse::async::Channel<muse::audio::secs_t, muse::midi::tick_t> PlaybackControllerStub::currentPlaybackPositionChanged() const
{
return muse::async::Channel<uint32_t>();
return muse::async::Channel<muse::audio::secs_t, muse::midi::tick_t>();
}

muse::audio::TrackSequenceId PlaybackControllerStub::currentTrackSequenceId() const
Expand Down
2 changes: 1 addition & 1 deletion src/stubs/playback/playbackcontrollerstub.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class PlaybackControllerStub : public IPlaybackController

void reset() override;

muse::async::Channel<uint32_t> midiTickPlayed() const override;
muse::async::Channel<muse::audio::secs_t, muse::midi::tick_t> currentPlaybackPositionChanged() const override;

muse::audio::TrackSequenceId currentTrackSequenceId() const override;
muse::async::Notification currentTrackSequenceIdChanged() const override;
Expand Down

0 comments on commit 2f7f138

Please sign in to comment.