Refactor to move positionChanged signal to Timeline (#7454)

Previously, this PR simply added a new signal to the Timeline class and had it emitted by the TimeLineWidget class in order to solve #7351.

However, as messmerd pointed out in his review comments, that was quite a hacky solution, and ideally the positionChanged signal would be solely managed by the backend Timeline class, while the frontend TimeLineWidget class would connect to those signals, instead of the other way around.

This PR is no longer a simple bugfix, but instead a refactoring of the Timeline/TimeLineWidget signal/slots.

Changes
- The positionChanged signal and updatePosition slot were removed from TimeLineWidget and moved to Timeline.
- Removed PlayPos, and instead store the timeline position in a TimePos along with a separate frame offset counter variable. The functions to set the ticks/timepos in Timeline emit the positionChanged signal. (Also, may emit positionJumped signal if the ticks were forcefully set--see below)
- The pos() method and PlayPos m_pos were removed from TimeLineWidget;
- The constructor for TimeLineWidget no longer requires a PlayPos reference.
- Since each TimeLineWidget stores a reference to its Timeline, a new method was added, timeline(), for other classes to access it.
- Removed array of PlayPos'es in Song. Now each Timeline holds their respective TimePos.
- Song's methods for getPlayPos were changed to return the TimePos of the respective Timeline. The non-const versions of the methods were removed because Timeline does not expose its TimePos to write.
- All of the places where Timelines are used were updated, along with their calls to access/modify the PlayPos. For example, occurrences of m_timeline->pos() were replaced with m_timeline->timeline()->pos(), and calls to m_timeline->pos().setTicks(ticks) were changed to m_timeline->timeline()->setTicks(ticks).
- ALSO: Removed m_elapsedMilliseconds, m_elapsedBars, and m_elapsedTicks from Song. The elapsed milliseconds is now handled individually by each Timeline.
- NEW: The m_jumped variable has been removed from Timeline. Now jumped events emit Timeline::positionJumped automatically whenever the ticks are forcefully set. This means it is no longer necessary to call timeline->setJumped(true) or timeline->setFrameOffset(0) whenever the playhead position is forcefully set. Many places in the codebase were already missing these calls, so this may fix some unknown bugs.

---------

Co-authored-by: Dalton Messmer <messmer.dalton@gmail.com>
Co-authored-by: saker <sakertooth@gmail.com>
Co-authored-by: Alex <allejok96@gmail.com>

include/AutomationEditor.h

@@ -157,7 +157,7 @@ protected slots:
 	void setProgressionType(int type);
 	void setTension();
 
-	void updatePosition( const lmms::TimePos & t );
+	void updatePosition();
 
 	void zoomingXChanged();
 	void zoomingYChanged();
@@ -298,7 +298,6 @@ private:
 
 signals:
 	void currentClipChanged();
-	void positionChanged( const lmms::TimePos & );
 } ;
 
 

include/PianoRoll.h

@@ -221,8 +221,8 @@ protected slots:
 	void pasteNotes();
 	bool deleteSelectedNotes();
 
-	void updatePosition(const lmms::TimePos & t );
-	void updatePositionAccompany(const lmms::TimePos & t );
+	void updatePosition();
+	void updatePositionAccompany();
 	void updatePositionStepRecording(const lmms::TimePos & t );
 
 	void zoomingChanged();
@@ -327,6 +327,7 @@ private:
 	void cancelStrumAction();
 
 	void updateScrollbars();
+	void updatePositionLinePos();
 	void updatePositionLineHeight();
 
 	QList<int> getAllOctavesForKey( int keyToMirror ) const;
@@ -530,9 +531,6 @@ private:
 	QBrush m_blackKeyActiveBackground;
 	QBrush m_blackKeyInactiveBackground;
 	QBrush m_blackKeyDisabledBackground;
-
-signals:
-	void positionChanged( const lmms::TimePos & );
 } ;
 
 

include/Song.h

@@ -100,36 +100,6 @@ public:
 	bool hasErrors();
 	QString errorSummary();
 
-	class PlayPos : public TimePos
-	{
-	public:
-		PlayPos( const int abs = 0 ) :
-			TimePos( abs ),
-			m_currentFrame( 0.0f )
-		{
-		}
-		inline void setCurrentFrame( const float f )
-		{
-			m_currentFrame = f;
-		}
-		inline float currentFrame() const
-		{
-			return m_currentFrame;
-		}
-		inline void setJumped( const bool jumped )
-		{
-			m_jumped = jumped;
-		}
-		inline bool jumped() const
-		{
-			return m_jumped;
-		}
-
-	private:
-		float m_currentFrame;
-		bool m_jumped;
-	};
-
 	void processNextBuffer();
 
 	inline int getLoadingTrackCount() const
@@ -142,31 +112,11 @@ public:
 		return getMilliseconds(m_playMode);
 	}
 
+	//! Returns the elapsed milliseconds since the start of the song
+	//! This function attempts to give the correct value even despite mid-song tempo changes
 	inline int getMilliseconds(PlayMode playMode) const
 	{
-		return m_elapsedMilliSeconds[static_cast<std::size_t>(playMode)];
-	}
-
-	inline void setToTime(TimePos const & pos)
-	{
-		setToTime(pos, m_playMode);
-	}
-
-	inline void setToTime(TimePos const & pos, PlayMode playMode)
-	{
-		m_elapsedMilliSeconds[static_cast<std::size_t>(playMode)] = pos.getTimeInMilliseconds(getTempo());
-		getPlayPos(playMode).setTicks(pos.getTicks());
-	}
-
-	inline void setToTimeByTicks(tick_t ticks)
-	{
-		setToTimeByTicks(ticks, m_playMode);
-	}
-
-	inline void setToTimeByTicks(tick_t ticks, PlayMode playMode)
-	{
-		m_elapsedMilliSeconds[static_cast<std::size_t>(playMode)] = TimePos::ticksToMilliseconds(ticks, getTempo());
-		getPlayPos(playMode).setTicks(ticks);
+		return 1000 * getTimeline(playMode).getElapsedSeconds();
 	}
 
 	inline int getBars() const
@@ -254,21 +204,22 @@ public:
 		return m_playMode;
 	}
 
-	inline PlayPos & getPlayPos( PlayMode pm )
+	const TimePos& getPlayPos(PlayMode pm) const
 	{
-		return m_playPos[static_cast<std::size_t>(pm)];
+		return getTimeline(pm).pos();
 	}
-	inline const PlayPos & getPlayPos( PlayMode pm ) const
-	{
-		return m_playPos[static_cast<std::size_t>(pm)];
-	}
-	inline PlayPos & getPlayPos()
+	const TimePos& getPlayPos() const
 	{
 		return getPlayPos(m_playMode);
 	}
-	inline const PlayPos & getPlayPos() const
+
+	void setPlayPos(tick_t ticks, PlayMode playMode)
 	{
-		return getPlayPos(m_playMode);
+		getTimeline(playMode).setTicks(ticks);
+	}
+	void setPlayPos(tick_t ticks)
+	{
+		setPlayPos(ticks, m_playMode);
 	}
 
 	auto getTimeline(PlayMode mode) -> Timeline& { return m_timelines[static_cast<std::size_t>(mode)]; }
@@ -430,12 +381,9 @@ private:
 
 	inline f_cnt_t currentFrame() const
 	{
-		return getPlayPos(m_playMode).getTicks() * Engine::framesPerTick() +
-			getPlayPos(m_playMode).currentFrame();
+		return getTimeline(m_playMode).ticks() * Engine::framesPerTick() + getTimeline(m_playMode).frameOffset();
 	}
 
-	void setPlayPos( tick_t ticks, PlayMode playMode );
-
 	void saveControllerStates( QDomDocument & doc, QDomElement & element );
 	void restoreControllerStates( const QDomElement & element );
 
@@ -489,16 +437,11 @@ private:
 	std::array<Timeline, PlayModeCount> m_timelines;
 
 	PlayMode m_playMode;
-	PlayPos m_playPos[PlayModeCount];
 	bar_t m_length;
 
 	const MidiClip* m_midiClipToPlay;
 	bool m_loopMidiClip;
 
-	double m_elapsedMilliSeconds[PlayModeCount];
-	tick_t m_elapsedTicks;
-	bar_t m_elapsedBars;
-
 	VstSyncController m_vstSyncController;
     
 	int m_loopRenderCount;
@@ -523,13 +466,12 @@ private:
 signals:
 	void projectLoaded();
 	void playbackStateChanged();
-	void playbackPositionChanged();
+	void playbackPositionJumped();
 	void lengthChanged( int bars );
 	void tempoChanged( lmms::bpm_t newBPM );
 	void timeSignatureChanged( int oldTicksPerBar, int ticksPerBar );
 	void controllerAdded( lmms::Controller * );
 	void controllerRemoved( lmms::Controller * );
-	void updateSampleTracks();
 	void stopped();
 	void modified();
 	void projectFileNameChanged();

include/SongEditor.h

@@ -89,7 +89,7 @@ public slots:
 	void setEditModeSelect();
 	void toggleProportionalSnap();
 
-	void updatePosition( const lmms::TimePos & t );
+	void updatePosition();
 	void updatePositionLine();
 	void selectAllClips( bool select );
 

include/TimeLineWidget.h

@@ -80,8 +80,8 @@ public:
 		Disabled
 	};
 
-	TimeLineWidget(int xoff, int yoff, float ppb, Song::PlayPos& pos, Timeline& timeline,
-				const TimePos& begin, Song::PlayMode mode, QWidget* parent);
+	TimeLineWidget(int xoff, int yoff, float ppb, Timeline& timeline,
+				const TimePos& begin, QWidget* parent);
 	~TimeLineWidget() override;
 
 	inline QColor const & getBarLineColor() const { return m_barLineColor; }
@@ -133,11 +133,6 @@ public:
 		m_cursorSelectRight = QCursor{m_cursorSelectRight.pixmap(), s.width(), s.height()};
 	}
 
-	inline Song::PlayPos & pos()
-	{
-		return( m_pos );
-	}
-
 	static AutoScrollState defaultAutoScrollState();
 	AutoScrollState autoScroll() const { return m_autoScroll; }
 	void setAutoScroll(AutoScrollState state) { m_autoScroll = state; }
@@ -158,6 +153,11 @@ public:
 					m_ppb / TimePos::ticksPerBar() );
 	}
 
+	Timeline* timeline()
+	{
+		return m_timeline;
+	}
+
 	bool isRecording() const { return m_isRecording; }
 	void setRecording(bool recording) { m_isRecording = recording; }
 
@@ -165,12 +165,10 @@ public:
 	void setPlayheadVisible(bool visible) { m_isPlayheadVisible = visible; }
 
 signals:
-	void positionChanged(const lmms::TimePos& postion);
 	void regionSelectedFromPixels( int, int );
 	void selectionFinished();
 
 public slots:
-	void updatePosition();
 	void setSnapSize( const float snapSize )
 	{
 		m_snapSize = snapSize;
@@ -225,11 +223,9 @@ private:
 	int m_xOffset;
 	float m_ppb;
 	float m_snapSize = 1.f;
-	Song::PlayPos & m_pos;
 	Timeline* m_timeline;
 	// Leftmost position visible in parent editor
 	const TimePos & m_begin;
-	const Song::PlayMode m_mode;
 
 	AutoScrollState m_autoScroll = defaultAutoScrollState();
 

include/Timeline.h

@@ -26,6 +26,8 @@
 
 #include <QObject>
 
+#include "AudioEngine.h"
+#include "Engine.h"
 #include "JournallingObject.h"
 #include "TimePos.h"
 
@@ -43,6 +45,32 @@ public:
 		KeepPosition
 	};
 
+	auto pos() const -> const TimePos& { return m_pos; }
+
+	auto ticks() const -> tick_t { return m_pos.getTicks(); }
+
+	//! Forcefully sets the current ticks, resets the frame offset, and sets the elapsed seconds based on the global position (ignoring potential mid-song tempo changes)
+	//! This function will emit the `positionJumped` signal to allow other widgets to update accordingly
+	void setTicks(tick_t ticks)
+	{
+		m_pos.setTicks(ticks);
+		m_frameOffset = 0;
+		m_elapsedSeconds = ticks * Engine::framesPerTick() / Engine::audioEngine()->outputSampleRate();
+		emit positionJumped();
+		emit positionChanged();
+	}
+
+	//! Advances the current timeline position by a certain number of ticks, in addition to updating the elapsed time based on the current tempo.
+	void incrementTicks(tick_t increment)
+	{
+		m_pos.setTicks(ticks() + increment);
+		m_elapsedSeconds += increment * Engine::framesPerTick() / Engine::audioEngine()->outputSampleRate();
+		emit positionChanged();
+	}
+
+	auto frameOffset() const -> f_cnt_t { return m_frameOffset; }
+	void setFrameOffset(const f_cnt_t frame) { m_frameOffset = frame; }
+
 	auto loopBegin() const -> TimePos { return m_loopBegin; }
 	auto loopEnd() const -> TimePos { return m_loopEnd; }
 	auto loopEnabled() const -> bool { return m_loopEnabled; }
@@ -58,11 +86,15 @@ public:
 	void setPlayStartPosition(TimePos position) { m_playStartPosition = position; }
 	void setStopBehaviour(StopBehaviour behaviour);
 
+	auto getElapsedSeconds() const -> double { return m_elapsedSeconds + frameOffset() / Engine::audioEngine()->outputSampleRate(); }
+
 	auto nodeName() const -> QString override { return "timeline"; }
 
 signals:
 	void loopEnabledChanged(bool enabled);
 	void stopBehaviourChanged(lmms::Timeline::StopBehaviour behaviour);
+	void positionChanged();
+	void positionJumped();
 
 protected:
 	void saveSettings(QDomDocument& doc, QDomElement& element) override;
@@ -72,6 +104,11 @@ private:
 	TimePos m_loopBegin = TimePos{0};
 	TimePos m_loopEnd = TimePos{DefaultTicksPerBar};
 	bool m_loopEnabled = false;
+	TimePos m_pos = TimePos{0};
+
+	f_cnt_t m_frameOffset = 0;
+
+	double m_elapsedSeconds = 0;
 
 	StopBehaviour m_stopBehaviour = StopBehaviour::BackToStart;
 	TimePos m_playStartPosition = TimePos{-1};