Fix PianoRoll playhead not moving during Record-Play #7454
Conversation
src/gui/editors/PianoRoll.cpp
Outdated
| @@ -290,7 +290,7 @@ PianoRoll::PianoRoll() : | |||
| this, SLOT( updatePositionStepRecording( const lmms::TimePos& ) ) ); | |||
|
|
|||
| // update timeline when in record-accompany mode | |||
| connect(m_timeLine, &TimeLineWidget::positionChanged, this, &PianoRoll::updatePositionAccompany); | |||
| connect(&Engine::getSong()->getTimeline(Song::PlayMode::Song), SIGNAL(positionChanged(const lmms::TimePos&)), this, SLOT(updatePositionAccompany(const lmms::TimePos&))); | |||
There was a problem hiding this comment.
| connect(&Engine::getSong()->getTimeline(Song::PlayMode::Song), SIGNAL(positionChanged(const lmms::TimePos&)), this, SLOT(updatePositionAccompany(const lmms::TimePos&))); | |
| connect(&Engine::getSong()->getTimeline(Song::PlayMode::Song), &Timeline::positionChanged, this, &PianoRoll::updatePositionAccompany); |
We're trying to avoid using the SIGNAL and SLOT macros in new code
There was a problem hiding this comment.
Actually, there may be a better approach than adding a signal to Timeline... Let me think this over some more
There was a problem hiding this comment.
I don't really like how Timeline and TimeLineWidget have implemented their signals and slots for changes in position. I think ideally Timeline should be the model and only it should emit any positionChanged signals, while TimeLineWidget should be the view and its updatePosition slot should not emit any signals. The way it is currently, the TimeLineWidget is trying to do the job of both a model and a view, and that complicates things.
There was a problem hiding this comment.
@DomClark Do you agree with my assessment? It looks to me like a proper solution to this bug may require rethinking how the timeline position signals/slots are implemented.
Co-authored-by: Dalton Messmer <[email protected]>
|
Fix looks simple. @messmerd did this fix your issue? |
This pull request fixes #7351 by changing the PianoRoll's connection from
m_timelineto&Engine::getSong()->getTimeline(Song::PlayMode::Song), and additionally adding the signalpositionChangedto theTimelineclass.The new signal is necessary because
getTimeline()returns aTimeline, which does not have apositionChangedsignal, whilem_timelineis aTimeLineWidgetwhich does.This pr is marked as a draft, since after fixing the bug I found another one: When pressing stop after record-play, the piano roll play head does not go back to the start, but instead stops at its current position.Edit: I have decided to leave fixing that bug to a future pr, as it appears that it existed prior to the regression. Instead, this pr simply fixes the pianoroll playhead movement.