From b20522242e7fb2e9e59d3bb469b138f7212aec74 Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Wed, 3 Feb 2021 12:14:07 -0300
Subject: [PATCH 01/21] Initial Commit

	Starts implementing Note Types. The two available types are
RegularNote and StepNote. PianoRoll now paints the color with a
different color for StepNotes. Pattern::addStep now sets the type of the
note to StepNote.
	Negative size is still used to signal a step note.
---
 data/themes/classic/style.css |  1 +
 data/themes/default/style.css |  1 +
 include/Note.h                | 17 +++++++++++++++++
 include/PianoRoll.h           |  2 ++
 src/core/Note.cpp             | 16 ++++++++++++++--
 src/gui/editors/PianoRoll.cpp | 10 +++++++---
 src/tracks/Pattern.cpp        |  9 +++++++--
 7 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/data/themes/classic/style.css b/data/themes/classic/style.css
index 860aa0da1ea..b5b4e274fad 100644
--- a/data/themes/classic/style.css
+++ b/data/themes/classic/style.css
@@ -145,6 +145,7 @@ PianoRoll {
 	qproperty-backgroundShade: rgba( 255, 255, 255, 10 );
 	qproperty-noteModeColor: rgb( 255, 255, 255 );
 	qproperty-noteColor: rgb( 119, 199, 216 );
+	qproperty-stepNoteColor: #9b1313;
 	qproperty-noteTextColor: rgb( 255, 255, 255 );
 	qproperty-noteOpacity: 128;
 	qproperty-noteBorders: true; /* boolean property, set false to have borderless notes */
diff --git a/data/themes/default/style.css b/data/themes/default/style.css
index ce476f5a9ce..9121f47a3cf 100644
--- a/data/themes/default/style.css
+++ b/data/themes/default/style.css
@@ -177,6 +177,7 @@ PianoRoll {
 	qproperty-backgroundShade: rgba(255, 255, 255, 10);
 	qproperty-noteModeColor: #0bd556;
 	qproperty-noteColor: #0bd556;
+	qproperty-stepNoteColor: #9b1313;
 	qproperty-noteTextColor: #ffffff;
 	qproperty-noteOpacity: 165;
 	qproperty-noteBorders: false; /* boolean property, set false to have borderless notes */
diff --git a/include/Note.h b/include/Note.h
index c08a3e24e55..d920218c6a1 100644
--- a/include/Note.h
+++ b/include/Note.h
@@ -90,6 +90,20 @@ class LMMS_EXPORT Note : public SerializingObject
 	Note( const Note & note );
 	virtual ~Note();
 
+	// Note types
+	enum Types
+	{
+		RegularNote,
+		StepNote
+	};
+	typedef Types Type;
+
+	Type type() const
+	{
+		return m_type;
+	}
+	void setType(Type t);
+
 	// used by GUI
 	inline void setSelected( const bool selected ) { m_selected = selected; }
 	inline void setOldKey( const int oldKey ) { m_oldKey = oldKey; }
@@ -236,6 +250,9 @@ class LMMS_EXPORT Note : public SerializingObject
 	TimePos m_length;
 	TimePos m_pos;
 	DetuningHelper * m_detuning;
+
+	// The type of this note
+	Type m_type;
 };
 
 
diff --git a/include/PianoRoll.h b/include/PianoRoll.h
index 5e0ea0762c9..968cf6e1bc1 100644
--- a/include/PianoRoll.h
+++ b/include/PianoRoll.h
@@ -61,6 +61,7 @@ class PianoRoll : public QWidget
 	Q_PROPERTY(QColor lineColor MEMBER m_lineColor)
 	Q_PROPERTY(QColor noteModeColor MEMBER m_noteModeColor)
 	Q_PROPERTY(QColor noteColor MEMBER m_noteColor)
+	Q_PROPERTY(QColor stepNoteColor MEMBER m_stepNoteColor)
 	Q_PROPERTY(QColor ghostNoteColor MEMBER m_ghostNoteColor)
 	Q_PROPERTY(QColor noteTextColor MEMBER m_noteTextColor)
 	Q_PROPERTY(QColor ghostNoteTextColor MEMBER m_ghostNoteTextColor)
@@ -419,6 +420,7 @@ protected slots:
 	QColor m_lineColor;
 	QColor m_noteModeColor;
 	QColor m_noteColor;
+	QColor m_stepNoteColor;
 	QColor m_noteTextColor;
 	QColor m_ghostNoteColor;
 	QColor m_ghostNoteTextColor;
diff --git a/src/core/Note.cpp b/src/core/Note.cpp
index 080a555b5d0..771edfac774 100644
--- a/src/core/Note.cpp
+++ b/src/core/Note.cpp
@@ -44,7 +44,8 @@ Note::Note( const TimePos & length, const TimePos & pos,
 	m_panning( qBound( PanningLeft, panning, PanningRight ) ),
 	m_length( length ),
 	m_pos( pos ),
-	m_detuning( NULL )
+	m_detuning(nullptr),
+	m_type(Note::RegularNote)
 {
 	if( detuning )
 	{
@@ -71,7 +72,8 @@ Note::Note( const Note & note ) :
 	m_panning( note.m_panning ),
 	m_length( note.m_length ),
 	m_pos( note.m_pos ),
-	m_detuning( NULL )
+	m_detuning(nullptr),
+	m_type(note.m_type)
 {
 	if( note.m_detuning )
 	{
@@ -136,6 +138,14 @@ void Note::setPanning( panning_t panning )
 
 
 
+void Note::setType(Note::Type t)
+{
+	m_type = t;
+}
+
+
+
+
 TimePos Note::quantized( const TimePos & m, const int qGrid )
 {
 	float p = ( (float) m / qGrid );
@@ -176,6 +186,7 @@ void Note::saveSettings( QDomDocument & doc, QDomElement & parent )
 	parent.setAttribute( "pan", m_panning );
 	parent.setAttribute( "len", m_length );
 	parent.setAttribute( "pos", m_pos );
+	parent.setAttribute("type", static_cast<int>(m_type));
 
 	if( m_detuning && m_length )
 	{
@@ -194,6 +205,7 @@ void Note::loadSettings( const QDomElement & _this )
 	m_panning = _this.attribute( "pan" ).toInt();
 	m_length = _this.attribute( "len" ).toInt();
 	m_pos = _this.attribute( "pos" ).toInt();
+	m_type = static_cast<Type>(_this.attribute("type").toInt());
 
 	if( _this.hasChildNodes() )
 	{
diff --git a/src/gui/editors/PianoRoll.cpp b/src/gui/editors/PianoRoll.cpp
index cee6870b7a3..c9154361614 100644
--- a/src/gui/editors/PianoRoll.cpp
+++ b/src/gui/editors/PianoRoll.cpp
@@ -3172,11 +3172,15 @@ void PianoRoll::paintEvent(QPaintEvent * pe )
 			if (note->key() > bottomKey && note->key() <= topKey)
 			{
 
-				// we've done and checked all, let's draw the note
+				// We've done and checked all, let's draw the note with
+				// the appropriate color
+				QColor fillColor = note->type() == Note::RegularNote ? m_noteColor : m_stepNoteColor;
+
 				drawNoteRect(
 					p, x + m_whiteKeyWidth, y_base - key * m_keyLineHeight, note_width,
-					note, m_noteColor, m_noteTextColor, m_selectedNoteColor,
-					m_noteOpacity, m_noteBorders, drawNoteNames);
+					note, fillColor, m_noteTextColor, m_selectedNoteColor,
+					m_noteOpacity, m_noteBorders, drawNoteNames
+				);
 			}
 
 			// draw note editing stuff
diff --git a/src/tracks/Pattern.cpp b/src/tracks/Pattern.cpp
index 659a1919c19..dd6aa857685 100644
--- a/src/tracks/Pattern.cpp
+++ b/src/tracks/Pattern.cpp
@@ -297,8 +297,13 @@ void Pattern::clearNotes()
 
 Note * Pattern::addStepNote( int step )
 {
-	return addNote( Note( TimePos( -DefaultTicksPerBar ),
-				TimePos::stepPosition( step ) ), false );
+	Note * n =
+		addNote(
+			Note(TimePos(-DefaultTicksPerBar),
+			TimePos::stepPosition(step)), false
+		);
+	n->setType(Note::StepNote);
+	return n;
 }
 
 

From cfb533ae7883b9b4eb09ddaed84c4d8d4f4ce807 Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Wed, 3 Feb 2021 12:36:55 -0300
Subject: [PATCH 02/21] Update Pattern.cpp to account for the Note::Type

	Updates the methods noteAtStep(), addStepNote() and checkType()
from Pattern.cpp to account for the note type and not the note length.
---
 src/tracks/Pattern.cpp | 47 ++++++++++++++++++++++++++++--------------
 1 file changed, 31 insertions(+), 16 deletions(-)

diff --git a/src/tracks/Pattern.cpp b/src/tracks/Pattern.cpp
index dd6aa857685..124bc1304ed 100644
--- a/src/tracks/Pattern.cpp
+++ b/src/tracks/Pattern.cpp
@@ -251,20 +251,18 @@ void Pattern::removeNote( Note * _note_to_del )
 }
 
 
-// returns a pointer to the note at specified step, or NULL if note doesn't exist
-
-Note * Pattern::noteAtStep( int _step )
+// Returns a pointer to the note at specified step, or nullptr if note doesn't exist
+Note * Pattern::noteAtStep(int step)
 {
-	for( NoteVector::Iterator it = m_notes.begin(); it != m_notes.end();
-									++it )
+	for (NoteVector::Iterator it = m_notes.begin(); it != m_notes.end(); ++it)
 	{
-		if( ( *it )->pos() == TimePos::stepPosition( _step )
-						&& ( *it )->length() < 0 )
+		if ((*it)->pos() == TimePos::stepPosition(step)
+			&& (*it)->type() == Note::StepNote)
 		{
 			return *it;
 		}
 	}
-	return NULL;
+	return nullptr;
 }
 
 
@@ -295,14 +293,20 @@ void Pattern::clearNotes()
 
 
 
-Note * Pattern::addStepNote( int step )
+Note * Pattern::addStepNote(int step)
 {
 	Note * n =
 		addNote(
-			Note(TimePos(-DefaultTicksPerBar),
+			Note(TimePos(DefaultTicksPerBar / 16),
 			TimePos::stepPosition(step)), false
 		);
 	n->setType(Note::StepNote);
+	// Have to check the type again now that we changed
+	// the type of the note
+	// TODO: This means checkType is called twice:
+	// Once on addNote and once here. Rethink it so we
+	// can call it only once.
+	checkType();
 	return n;
 }
 
@@ -344,16 +348,27 @@ void Pattern::setType( PatternTypes _new_pattern_type )
 void Pattern::checkType()
 {
 	NoteVector::Iterator it = m_notes.begin();
-	while( it != m_notes.end() )
+
+	// If all notes are StepNotes, we have a BeatPattern
+	bool beatPattern = true;
+	while (it != m_notes.end())
 	{
-		if( ( *it )->length() > 0 )
+		if ((*it)->type() != Note::StepNote)
 		{
-			setType( MelodyPattern );
-			return;
+			beatPattern = false;
+			break;
 		}
 		++it;
 	}
-	setType( BeatPattern );
+
+	if (beatPattern)
+	{
+		setType(BeatPattern);
+	}
+	else
+	{
+		setType(MelodyPattern);
+	}
 }
 
 
@@ -763,7 +778,7 @@ void PatternView::mousePressEvent( QMouseEvent * _me )
 
 		Note * n = m_pat->noteAtStep( step );
 
-		if( n == NULL )
+		if (n == nullptr)
 		{
 			m_pat->addStepNote( step );
 		}

From a5cf05d31e88eb72b381832d56acae6ddf0ff333 Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Wed, 3 Feb 2021 13:19:42 -0300
Subject: [PATCH 03/21] Update PatternView::paintEvent to draw step notes

	PatternView::paintEvent now draws the pattern if the pattern
type is BeatPattern and TCOs aren't fixed (Song Editor). Color used is
still the BeatPattern color (grey) and the conditional doesn't look very
nice and can be improved.
	Pattern::beatPatternLength was also updated so it accounts for
the note type not note length. Review this method, as it looks a bit
weird (particularly the second conditional).
---
 src/tracks/Pattern.cpp | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/src/tracks/Pattern.cpp b/src/tracks/Pattern.cpp
index 124bc1304ed..38dded4f828 100644
--- a/src/tracks/Pattern.cpp
+++ b/src/tracks/Pattern.cpp
@@ -183,23 +183,20 @@ TimePos Pattern::beatPatternLength() const
 {
 	tick_t max_length = TimePos::ticksPerBar();
 
-	for( NoteVector::ConstIterator it = m_notes.begin();
-						it != m_notes.end(); ++it )
+	for (NoteVector::ConstIterator it = m_notes.begin(); it != m_notes.end(); ++it)
 	{
-		if( ( *it )->length() < 0 )
+		if ((*it)->type() == Note::StepNote)
 		{
-			max_length = qMax<tick_t>( max_length,
-				( *it )->pos() + 1 );
+			max_length = qMax<tick_t>(max_length, (*it)->pos() + 1);
 		}
 	}
 
-	if( m_steps != TimePos::stepsPerBar() )
+	if (m_steps != TimePos::stepsPerBar())
 	{
-		max_length = m_steps * TimePos::ticksPerBar() /
-						TimePos::stepsPerBar();
+		max_length = m_steps * TimePos::ticksPerBar() / TimePos::stepsPerBar();
 	}
 
-	return TimePos( max_length ).nextFullBar() * TimePos::ticksPerBar();
+	return TimePos(max_length).nextFullBar() * TimePos::ticksPerBar();
 }
 
 
@@ -958,9 +955,12 @@ void PatternView::paintEvent( QPaintEvent * )
 
 	const int x_base = TCO_BORDER_WIDTH;
 
-	// melody pattern paint event
+	// Melody pattern and Beat Pattern (on Song Editor) paint event
+	// TODO: Improve this ugly conditional
 	NoteVector const & noteCollection = m_pat->m_notes;
-	if( m_pat->m_patternType == Pattern::MelodyPattern && !noteCollection.empty() )
+	if ((m_pat->m_patternType == Pattern::MelodyPattern
+		|| (m_pat->m_patternType == Pattern::BeatPattern && !fixedTCOs()))
+		&& !noteCollection.empty())
 	{
 		// Compute the minimum and maximum key in the pattern
 		// so that we know how much there is to draw.
@@ -1073,7 +1073,6 @@ void PatternView::paintEvent( QPaintEvent * )
 
 		p.restore();
 	}
-
 	// beat pattern paint event
 	else if( beatPattern &&	( fixedTCOs() || pixelsPerBar >= 96 ) )
 	{

From df88140cae5e03a43f3a40cf6fb9ab35eeac1e12 Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Wed, 3 Feb 2021 13:41:38 -0300
Subject: [PATCH 04/21] Implements StepNotes setting a NPH with 0 frames

	Now, instead of TimePos returning 0 for negative lengths, we
create a NotePlayHandle with 0 frames when the note type is StepNote on
InstrumentTrack::play.
---
 src/core/TimePos.cpp           | 10 +++++-----
 src/tracks/InstrumentTrack.cpp |  7 +++++--
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/src/core/TimePos.cpp b/src/core/TimePos.cpp
index 4a22a1eb999..95ae24a2758 100644
--- a/src/core/TimePos.cpp
+++ b/src/core/TimePos.cpp
@@ -158,11 +158,11 @@ tick_t TimePos::getTickWithinBeat( const TimeSig &sig ) const
 
 f_cnt_t TimePos::frames( const float framesPerTick ) const
 {
-	if( m_ticks >= 0 )
-	{
-		return static_cast<f_cnt_t>( m_ticks * framesPerTick );
-	}
-	return 0;
+	//TODO: Remove next line, it's just a warning to see
+	// if any leftover code is still creating notes with
+	// negative lengths
+	if (m_ticks < 0) { qWarning("TimePos with negative length"); }
+	return static_cast<f_cnt_t>(m_ticks * framesPerTick);
 }
 
 double TimePos::getTimeInMilliseconds( bpm_t beatsPerMinute ) const
diff --git a/src/tracks/InstrumentTrack.cpp b/src/tracks/InstrumentTrack.cpp
index 4c5304fe9d2..0884346b03d 100644
--- a/src/tracks/InstrumentTrack.cpp
+++ b/src/tracks/InstrumentTrack.cpp
@@ -723,8 +723,11 @@ bool InstrumentTrack::play( const TimePos & _start, const fpp_t _frames,
 		while( nit != notes.end() &&
 					( cur_note = *nit )->pos() == cur_start )
 		{
-			const f_cnt_t note_frames =
-				cur_note->length().frames( frames_per_tick );
+			// If the note is a Step Note, frames will be 0 so the NotePlayHandle
+			// plays for the whole length of the sample
+			const f_cnt_t note_frames = cur_note->type() == Note::StepNote
+				? 0
+				: cur_note->length().frames(frames_per_tick);
 
 			NotePlayHandle* notePlayHandle = NotePlayHandleManager::acquire( this, _offset, note_frames, *cur_note );
 			notePlayHandle->setBBTrack( bb_track );

From 7a5ad82144aeff026e91e871ae7b02fc40ab632e Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Wed, 3 Feb 2021 13:48:46 -0300
Subject: [PATCH 05/21] Improves PatternView::paintEvent conditional

	Improves a conditional inside PatternView::paintEvent by
reversing the order in which they are executed.
---
 src/tracks/Pattern.cpp | 136 ++++++++++++++++++++---------------------
 1 file changed, 67 insertions(+), 69 deletions(-)

diff --git a/src/tracks/Pattern.cpp b/src/tracks/Pattern.cpp
index 38dded4f828..be0e0f5f307 100644
--- a/src/tracks/Pattern.cpp
+++ b/src/tracks/Pattern.cpp
@@ -955,12 +955,74 @@ void PatternView::paintEvent( QPaintEvent * )
 
 	const int x_base = TCO_BORDER_WIDTH;
 
-	// Melody pattern and Beat Pattern (on Song Editor) paint event
-	// TODO: Improve this ugly conditional
 	NoteVector const & noteCollection = m_pat->m_notes;
-	if ((m_pat->m_patternType == Pattern::MelodyPattern
-		|| (m_pat->m_patternType == Pattern::BeatPattern && !fixedTCOs()))
-		&& !noteCollection.empty())
+
+	// Beat pattern paint event
+	if (beatPattern && (fixedTCOs() || pixelsPerBar >= 96))
+	{
+		QPixmap stepon0;
+		QPixmap stepon200;
+		QPixmap stepoff;
+		QPixmap stepoffl;
+		const int steps = qMax( 1,
+					m_pat->m_steps );
+		const int w = width() - 2 * TCO_BORDER_WIDTH;
+
+		// scale step graphics to fit the beat pattern length
+		stepon0 = s_stepBtnOn0->scaled( w / steps,
+					      s_stepBtnOn0->height(),
+					      Qt::IgnoreAspectRatio,
+					      Qt::SmoothTransformation );
+		stepon200 = s_stepBtnOn200->scaled( w / steps,
+					      s_stepBtnOn200->height(),
+					      Qt::IgnoreAspectRatio,
+					      Qt::SmoothTransformation );
+		stepoff = s_stepBtnOff->scaled( w / steps,
+						s_stepBtnOff->height(),
+						Qt::IgnoreAspectRatio,
+						Qt::SmoothTransformation );
+		stepoffl = s_stepBtnOffLight->scaled( w / steps,
+						s_stepBtnOffLight->height(),
+						Qt::IgnoreAspectRatio,
+						Qt::SmoothTransformation );
+
+		for( int it = 0; it < steps; it++ )	// go through all the steps in the beat pattern
+		{
+			Note * n = m_pat->noteAtStep( it );
+
+			// figure out x and y coordinates for step graphic
+			const int x = TCO_BORDER_WIDTH + static_cast<int>( it * w / steps );
+			const int y = height() - s_stepBtnOff->height() - 1;
+
+			if( n )
+			{
+				const int vol = n->getVolume();
+				p.drawPixmap( x, y, stepoffl );
+				p.drawPixmap( x, y, stepon0 );
+				p.setOpacity( sqrt( vol / 200.0 ) );
+				p.drawPixmap( x, y, stepon200 );
+				p.setOpacity( 1 );
+			}
+			else if( ( it / 4 ) % 2 )
+			{
+				p.drawPixmap( x, y, stepoffl );
+			}
+			else
+			{
+				p.drawPixmap( x, y, stepoff );
+			}
+		} // end for loop
+
+		// draw a transparent rectangle over muted patterns
+		if ( muted )
+		{
+			p.setBrush( mutedBackgroundColor() );
+			p.setOpacity( 0.5 );
+			p.drawRect( 0, 0, width(), height() );
+		}
+	}
+	// Melody pattern and Beat Pattern (on Song Editor) paint event
+	else if (!noteCollection.empty())
 	{
 		// Compute the minimum and maximum key in the pattern
 		// so that we know how much there is to draw.
@@ -1073,70 +1135,6 @@ void PatternView::paintEvent( QPaintEvent * )
 
 		p.restore();
 	}
-	// beat pattern paint event
-	else if( beatPattern &&	( fixedTCOs() || pixelsPerBar >= 96 ) )
-	{
-		QPixmap stepon0;
-		QPixmap stepon200;
-		QPixmap stepoff;
-		QPixmap stepoffl;
-		const int steps = qMax( 1,
-					m_pat->m_steps );
-		const int w = width() - 2 * TCO_BORDER_WIDTH;
-
-		// scale step graphics to fit the beat pattern length
-		stepon0 = s_stepBtnOn0->scaled( w / steps,
-					      s_stepBtnOn0->height(),
-					      Qt::IgnoreAspectRatio,
-					      Qt::SmoothTransformation );
-		stepon200 = s_stepBtnOn200->scaled( w / steps,
-					      s_stepBtnOn200->height(),
-					      Qt::IgnoreAspectRatio,
-					      Qt::SmoothTransformation );
-		stepoff = s_stepBtnOff->scaled( w / steps,
-						s_stepBtnOff->height(),
-						Qt::IgnoreAspectRatio,
-						Qt::SmoothTransformation );
-		stepoffl = s_stepBtnOffLight->scaled( w / steps,
-						s_stepBtnOffLight->height(),
-						Qt::IgnoreAspectRatio,
-						Qt::SmoothTransformation );
-
-		for( int it = 0; it < steps; it++ )	// go through all the steps in the beat pattern
-		{
-			Note * n = m_pat->noteAtStep( it );
-
-			// figure out x and y coordinates for step graphic
-			const int x = TCO_BORDER_WIDTH + static_cast<int>( it * w / steps );
-			const int y = height() - s_stepBtnOff->height() - 1;
-
-			if( n )
-			{
-				const int vol = n->getVolume();
-				p.drawPixmap( x, y, stepoffl );
-				p.drawPixmap( x, y, stepon0 );
-				p.setOpacity( sqrt( vol / 200.0 ) );
-				p.drawPixmap( x, y, stepon200 );
-				p.setOpacity( 1 );
-			}
-			else if( ( it / 4 ) % 2 )
-			{
-				p.drawPixmap( x, y, stepoffl );
-			}
-			else
-			{
-				p.drawPixmap( x, y, stepoff );
-			}
-		} // end for loop
-
-		// draw a transparent rectangle over muted patterns
-		if ( muted )
-		{
-			p.setBrush( mutedBackgroundColor() );
-			p.setOpacity( 0.5 );
-			p.drawRect( 0, 0, width(), height() );
-		}
-	}
 
 	// bar lines
 	const int lineSize = 3;

From 725169a61f8075eff59273bf32f281972326b13f Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Wed, 3 Feb 2021 20:03:03 -0300
Subject: [PATCH 06/21] Adds upgrade method for backwards compatibility

	Adds an upgrade method that converts notes with negative length
to StepNotes, so old projects can be loaded properly.
	Explicitly set the Note::RegularNote value as 0.
	Make the default "type" value "0", so notes without a type are
loaded as RegularNotes.
---
 include/DataFile.h    |  1 +
 include/Note.h        |  2 +-
 src/core/DataFile.cpp | 21 ++++++++++++++++++++-
 src/core/Note.cpp     |  3 ++-
 4 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/include/DataFile.h b/include/DataFile.h
index 5d6ead5adb3..01a143dcc45 100644
--- a/include/DataFile.h
+++ b/include/DataFile.h
@@ -115,6 +115,7 @@ class LMMS_EXPORT DataFile : public QDomDocument
 	void upgrade_1_2_0_rc3();
 	void upgrade_1_3_0();
 	void upgrade_noHiddenClipNames();
+	void upgrade_noteTypes();
 
 	// List of all upgrade methods
 	static const std::vector<UpgradeMethod> UPGRADE_METHODS;
diff --git a/include/Note.h b/include/Note.h
index d920218c6a1..0e2eb4aa946 100644
--- a/include/Note.h
+++ b/include/Note.h
@@ -93,7 +93,7 @@ class LMMS_EXPORT Note : public SerializingObject
 	// Note types
 	enum Types
 	{
-		RegularNote,
+		RegularNote = 0,
 		StepNote
 	};
 	typedef Types Type;
diff --git a/src/core/DataFile.cpp b/src/core/DataFile.cpp
index 9dc413566d7..79548041434 100644
--- a/src/core/DataFile.cpp
+++ b/src/core/DataFile.cpp
@@ -59,7 +59,8 @@ const std::vector<DataFile::UpgradeMethod> DataFile::UPGRADE_METHODS = {
 	&DataFile::upgrade_0_4_0_beta1      ,   &DataFile::upgrade_0_4_0_rc2,
 	&DataFile::upgrade_1_0_99           ,   &DataFile::upgrade_1_1_0,
 	&DataFile::upgrade_1_1_91           ,   &DataFile::upgrade_1_2_0_rc3,
-	&DataFile::upgrade_1_3_0            ,   &DataFile::upgrade_noHiddenClipNames
+	&DataFile::upgrade_1_3_0            ,   &DataFile::upgrade_noHiddenClipNames,
+	&DataFile::upgrade_noteTypes
 };
 
 // Vector of all versions that have upgrade routines.
@@ -1385,6 +1386,24 @@ void DataFile::upgrade_noHiddenClipNames()
 	}
 }
 
+// Convert the negative length notes to StepNotes
+void DataFile::upgrade_noteTypes()
+{
+	QDomNodeList notes = elementsByTagName("note");
+
+	for (int i = 0; i < notes.size(); ++i)
+	{
+		QDomElement note = notes.item(i).toElement();
+
+		int noteSize = note.attribute("len").toInt();
+		if (noteSize < 0)
+		{
+			note.setAttribute("len", DefaultTicksPerBar / 16);
+			note.setAttribute("type", static_cast<int>(Note::StepNote));
+		}
+	}
+}
+
 
 void DataFile::upgrade()
 {
diff --git a/src/core/Note.cpp b/src/core/Note.cpp
index 771edfac774..fcc783e4da3 100644
--- a/src/core/Note.cpp
+++ b/src/core/Note.cpp
@@ -205,7 +205,8 @@ void Note::loadSettings( const QDomElement & _this )
 	m_panning = _this.attribute( "pan" ).toInt();
 	m_length = _this.attribute( "len" ).toInt();
 	m_pos = _this.attribute( "pos" ).toInt();
-	m_type = static_cast<Type>(_this.attribute("type").toInt());
+	// Default m_type value is 0, which corresponds to RegularNote
+	m_type = static_cast<Type>(_this.attribute("type", "0").toInt());
 
 	if( _this.hasChildNodes() )
 	{

From faa52099eaa36f7e8b7a40f8b6ceacf4847d4836 Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Wed, 10 Feb 2021 22:31:47 -0300
Subject: [PATCH 07/21] Addresses Veratil's review

	- Changes "addStepNote" so "checkType" isn't called twice in a
row.
	- Changes style on a one line conditional.
---
 src/tracks/Pattern.cpp | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/src/tracks/Pattern.cpp b/src/tracks/Pattern.cpp
index be0e0f5f307..d1ea0e0a4c3 100644
--- a/src/tracks/Pattern.cpp
+++ b/src/tracks/Pattern.cpp
@@ -292,18 +292,18 @@ void Pattern::clearNotes()
 
 Note * Pattern::addStepNote(int step)
 {
+	auto prevPatternType = m_patternType;
 	Note * n =
 		addNote(
 			Note(TimePos(DefaultTicksPerBar / 16),
 			TimePos::stepPosition(step)), false
 		);
 	n->setType(Note::StepNote);
-	// Have to check the type again now that we changed
-	// the type of the note
-	// TODO: This means checkType is called twice:
-	// Once on addNote and once here. Rethink it so we
-	// can call it only once.
-	checkType();
+	// addNote will change a BeatPattern to a MelodyPattern
+	// because it calls checkType after adding a regular note.
+	// We need to revert it back to a BeatPattern if it was one
+	// before we added the note.
+	if (prevPatternType == BeatPattern) { setType(BeatPattern); }
 	return n;
 }
 
@@ -358,14 +358,8 @@ void Pattern::checkType()
 		++it;
 	}
 
-	if (beatPattern)
-	{
-		setType(BeatPattern);
-	}
-	else
-	{
-		setType(MelodyPattern);
-	}
+	if (beatPattern) { setType(BeatPattern); }
+	else { setType(MelodyPattern); }
 }
 
 

From 9decf112521cc626ef3cbb1ecaa99c3d1896ec7f Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Thu, 11 Feb 2021 18:21:00 -0300
Subject: [PATCH 08/21] Uses ternary expression on statement

	Reduces number of lines by using ternary expression.
---
 src/tracks/Pattern.cpp | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/tracks/Pattern.cpp b/src/tracks/Pattern.cpp
index d1ea0e0a4c3..3ac49f50eda 100644
--- a/src/tracks/Pattern.cpp
+++ b/src/tracks/Pattern.cpp
@@ -358,8 +358,7 @@ void Pattern::checkType()
 		++it;
 	}
 
-	if (beatPattern) { setType(BeatPattern); }
-	else { setType(MelodyPattern); }
+	setType(beatPattern ? BeatPattern : MelodyPattern);
 }
 
 

From db0d0b73836c99cfb8a81e98e1e876adb8435df3 Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Sun, 9 Jul 2023 10:23:01 -0300
Subject: [PATCH 09/21] Addresses PR review (sakertooth)

	- Changes class setter to inline
	- Uses enum class instead of enum
	- Uses auto and const where appropriate
---
 include/Note.h                 |  9 ++++-----
 src/core/DataFile.cpp          |  8 ++++----
 src/core/Note.cpp              | 11 +----------
 src/gui/editors/PianoRoll.cpp  |  2 +-
 src/tracks/InstrumentTrack.cpp |  2 +-
 src/tracks/MidiClip.cpp        | 17 +++++++----------
 6 files changed, 18 insertions(+), 31 deletions(-)

diff --git a/include/Note.h b/include/Note.h
index e1d0e2a4fb8..ea726b4363e 100644
--- a/include/Note.h
+++ b/include/Note.h
@@ -102,18 +102,17 @@ class LMMS_EXPORT Note : public SerializingObject
 	~Note() override;
 
 	// Note types
-	enum Types
+	enum class Type
 	{
 		RegularNote = 0,
 		StepNote
 	};
-	typedef Types Type;
 
 	Type type() const
 	{
 		return m_type;
 	}
-	void setType(Type t);
+	inline void setType( Type t ) { m_type = t; }
 
 	// used by GUI
 	inline void setSelected( const bool selected ) { m_selected = selected; }
@@ -260,10 +259,10 @@ class LMMS_EXPORT Note : public SerializingObject
 	panning_t m_panning;
 	TimePos m_length;
 	TimePos m_pos;
-	DetuningHelper * m_detuning;
+	DetuningHelper * m_detuning = nullptr;
 
 	// The type of this note
-	Type m_type;
+	Type m_type = Type::RegularNote;
 };
 
 using NoteVector = QVector<Note*>;
diff --git a/src/core/DataFile.cpp b/src/core/DataFile.cpp
index 2ef9a2789a0..0322f976ada 100644
--- a/src/core/DataFile.cpp
+++ b/src/core/DataFile.cpp
@@ -1674,17 +1674,17 @@ void DataFile::upgrade_automationNodes()
 // Convert the negative length notes to StepNotes
 void DataFile::upgrade_noteTypes()
 {
-	QDomNodeList notes = elementsByTagName("note");
+	const auto notes = elementsByTagName("note");
 
 	for (int i = 0; i < notes.size(); ++i)
 	{
-		QDomElement note = notes.item(i).toElement();
+		auto note = notes.item(i).toElement();
 
-		int noteSize = note.attribute("len").toInt();
+		const auto noteSize = note.attribute("len").toInt();
 		if (noteSize < 0)
 		{
 			note.setAttribute("len", DefaultTicksPerBar / 16);
-			note.setAttribute("type", static_cast<int>(Note::StepNote));
+			note.setAttribute("type", static_cast<int>(Note::Type::StepNote));
 		}
 	}
 }
diff --git a/src/core/Note.cpp b/src/core/Note.cpp
index 33541130c67..a45a06abe8d 100644
--- a/src/core/Note.cpp
+++ b/src/core/Note.cpp
@@ -47,8 +47,7 @@ Note::Note( const TimePos & length, const TimePos & pos,
 	m_panning( qBound( PanningLeft, panning, PanningRight ) ),
 	m_length( length ),
 	m_pos( pos ),
-	m_detuning( nullptr ),
-	m_type( Note::RegularNote )
+	m_detuning( nullptr )
 {
 	if( detuning )
 	{
@@ -141,14 +140,6 @@ void Note::setPanning( panning_t panning )
 
 
 
-void Note::setType(Note::Type t)
-{
-	m_type = t;
-}
-
-
-
-
 TimePos Note::quantized( const TimePos & m, const int qGrid )
 {
 	float p = ( (float) m / qGrid );
diff --git a/src/gui/editors/PianoRoll.cpp b/src/gui/editors/PianoRoll.cpp
index 5ccfa184765..9736ccb2274 100644
--- a/src/gui/editors/PianoRoll.cpp
+++ b/src/gui/editors/PianoRoll.cpp
@@ -3501,7 +3501,7 @@ void PianoRoll::paintEvent(QPaintEvent * pe )
 			{
 				// We've done and checked all, let's draw the note with
 				// the appropriate color
-				QColor fillColor = note->type() == Note::RegularNote ? m_noteColor : m_stepNoteColor;
+				const auto fillColor = note->type() == Note::Type::RegularNote ? m_noteColor : m_stepNoteColor;
 
 				drawNoteRect(
 					p, x + m_whiteKeyWidth, noteYPos(note->key()), note_width,
diff --git a/src/tracks/InstrumentTrack.cpp b/src/tracks/InstrumentTrack.cpp
index 5902f666c87..22e8d2a3d5c 100644
--- a/src/tracks/InstrumentTrack.cpp
+++ b/src/tracks/InstrumentTrack.cpp
@@ -760,7 +760,7 @@ bool InstrumentTrack::play( const TimePos & _start, const fpp_t _frames,
 		{
 			// If the note is a Step Note, frames will be 0 so the NotePlayHandle
 			// plays for the whole length of the sample
-			const f_cnt_t note_frames = cur_note->type() == Note::StepNote
+			const auto note_frames = cur_note->type() == Note::Type::StepNote
 				? 0
 				: cur_note->length().frames(frames_per_tick);
 
diff --git a/src/tracks/MidiClip.cpp b/src/tracks/MidiClip.cpp
index 2cc8f4c4ecb..01a00dfcf86 100644
--- a/src/tracks/MidiClip.cpp
+++ b/src/tracks/MidiClip.cpp
@@ -174,7 +174,7 @@ TimePos MidiClip::beatClipLength() const
 
 	for (const auto& note : m_notes)
 	{
-		if (note->type() == Note::StepNote)
+		if (note->type() == Note::Type::StepNote)
 		{
 			max_length = qMax<tick_t>(max_length, note->pos() + 1);
 		}
@@ -185,7 +185,7 @@ TimePos MidiClip::beatClipLength() const
 		max_length = m_steps * TimePos::ticksPerBar() / TimePos::stepsPerBar();
 	}
 
-	return TimePos(max_length).nextFullBar() * TimePos::ticksPerBar();
+	return TimePos{max_length}.nextFullBar() * TimePos::ticksPerBar();
 }
 
 
@@ -243,7 +243,7 @@ Note * MidiClip::noteAtStep( int step )
 	for (const auto& note : m_notes)
 	{
 		if (note->pos() == TimePos::stepPosition(step)
-			&& note->type() == Note::StepNote)
+			&& note->type() == Note::Type::StepNote)
 		{
 			return note;
 		}
@@ -280,13 +280,13 @@ void MidiClip::clearNotes()
 
 Note * MidiClip::addStepNote( int step )
 {
-	auto prevPatternType = m_clipType;
+	const auto prevPatternType = m_clipType;
 	Note * n =
 		addNote(
 			Note(TimePos(DefaultTicksPerBar / 16),
 			TimePos::stepPosition(step)), false
 		);
-	n->setType(Note::StepNote);
+	n->setType(Note::Type::StepNote);
 	// addNote will change a BeatClip to a MelodyClip
 	// because it calls checkType after adding a regular note.
 	// We need to revert it back to a BeatClip if it was one
@@ -364,18 +364,15 @@ void MidiClip::setType( MidiClipTypes _new_clip_type )
 
 void MidiClip::checkType()
 {
-	NoteVector::Iterator it = m_notes.begin();
-
 	// If all notes are StepNotes, we have a BeatClip
 	bool beatClip = true;
-	while (it != m_notes.end())
+	for (const auto it : m_notes)
 	{
-		if ((*it)->type() != Note::StepNote)
+		if (it->type() != Note::Type::StepNote)
 		{
 			beatClip = false;
 			break;
 		}
-		++it;
 	}
 
 	setType(beatClip ? BeatClip : MelodyClip);

From 6b3c75ec7a96037ec416b0d2f75a55a8d4724edc Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Sun, 9 Jul 2023 20:27:43 -0300
Subject: [PATCH 10/21] Finished changes from review (sakertooth)

	- Used std::max instead of qMax
	- Fixed style on lines changed in the PR
---
 include/Note.h                 |  2 +-
 src/core/Note.cpp              |  4 +--
 src/gui/clips/MidiClipView.cpp | 55 +++++++++++++++++-----------------
 src/tracks/MidiClip.cpp        |  2 +-
 4 files changed, 32 insertions(+), 31 deletions(-)

diff --git a/include/Note.h b/include/Note.h
index ea726b4363e..7d38634138a 100644
--- a/include/Note.h
+++ b/include/Note.h
@@ -112,7 +112,7 @@ class LMMS_EXPORT Note : public SerializingObject
 	{
 		return m_type;
 	}
-	inline void setType( Type t ) { m_type = t; }
+	inline void setType(Type t) { m_type = t; }
 
 	// used by GUI
 	inline void setSelected( const bool selected ) { m_selected = selected; }
diff --git a/src/core/Note.cpp b/src/core/Note.cpp
index a45a06abe8d..ac61706e7f2 100644
--- a/src/core/Note.cpp
+++ b/src/core/Note.cpp
@@ -74,8 +74,8 @@ Note::Note( const Note & note ) :
 	m_panning( note.m_panning ),
 	m_length( note.m_length ),
 	m_pos( note.m_pos ),
-	m_detuning( nullptr ),
-	m_type( note.m_type )
+	m_detuning(nullptr),
+	m_type(note.m_type)
 {
 	if( note.m_detuning )
 	{
diff --git a/src/gui/clips/MidiClipView.cpp b/src/gui/clips/MidiClipView.cpp
index 92331cc4d13..89959b36b7d 100644
--- a/src/gui/clips/MidiClipView.cpp
+++ b/src/gui/clips/MidiClipView.cpp
@@ -25,6 +25,7 @@
 
 #include "MidiClipView.h"
 
+#include <algorithm>
 #include <cmath>
 #include <QApplication>
 #include <QInputDialog>
@@ -467,65 +468,65 @@ void MidiClipView::paintEvent( QPaintEvent * )
 		QPixmap stepon200;
 		QPixmap stepoff;
 		QPixmap stepoffl;
-		const int steps = qMax( 1,
-					m_clip->m_steps );
+		const int steps = std::max(1,
+					m_clip->m_steps);
 		const int w = width() - 2 * BORDER_WIDTH;
 
 		// scale step graphics to fit the beat clip length
-		stepon0 = s_stepBtnOn0->scaled( w / steps,
+		stepon0 = s_stepBtnOn0->scaled(w / steps,
 					      s_stepBtnOn0->height(),
 					      Qt::IgnoreAspectRatio,
-					      Qt::SmoothTransformation );
-		stepon200 = s_stepBtnOn200->scaled( w / steps,
+					      Qt::SmoothTransformation);
+		stepon200 = s_stepBtnOn200->scaled(w / steps,
 					      s_stepBtnOn200->height(),
 					      Qt::IgnoreAspectRatio,
-					      Qt::SmoothTransformation );
-		stepoff = s_stepBtnOff->scaled( w / steps,
+					      Qt::SmoothTransformation);
+		stepoff = s_stepBtnOff->scaled(w / steps,
 						s_stepBtnOff->height(),
 						Qt::IgnoreAspectRatio,
-						Qt::SmoothTransformation );
-		stepoffl = s_stepBtnOffLight->scaled( w / steps,
+						Qt::SmoothTransformation);
+		stepoffl = s_stepBtnOffLight->scaled(w / steps,
 						s_stepBtnOffLight->height(),
 						Qt::IgnoreAspectRatio,
-						Qt::SmoothTransformation );
+						Qt::SmoothTransformation);
 
-		for( int it = 0; it < steps; it++ )	// go through all the steps in the beat clip
+		for (int it = 0; it < steps; it++)	// go through all the steps in the beat clip
 		{
-			Note * n = m_clip->noteAtStep( it );
+			Note * n = m_clip->noteAtStep(it);
 
 			// figure out x and y coordinates for step graphic
-			const int x = BORDER_WIDTH + static_cast<int>( it * w / steps );
+			const int x = BORDER_WIDTH + static_cast<int>(it * w / steps);
 			const int y = height() - s_stepBtnOff->height() - 1;
 
-			if( n )
+			if (n)
 			{
 				const int vol = n->getVolume();
-				p.drawPixmap( x, y, stepoffl );
-				p.drawPixmap( x, y, stepon0 );
-				p.setOpacity( sqrt( vol / 200.0 ) );
-				p.drawPixmap( x, y, stepon200 );
-				p.setOpacity( 1 );
+				p.drawPixmap(x, y, stepoffl);
+				p.drawPixmap(x, y, stepon0);
+				p.setOpacity(sqrt( vol / 200.0 ));
+				p.drawPixmap(x, y, stepon200);
+				p.setOpacity(1);
 			}
-			else if( ( it / 4 ) % 2 )
+			else if ((it / 4) % 2)
 			{
-				p.drawPixmap( x, y, stepoffl );
+				p.drawPixmap(x, y, stepoffl);
 			}
 			else
 			{
-				p.drawPixmap( x, y, stepoff );
+				p.drawPixmap(x, y, stepoff);
 			}
 		} // end for loop
 
 		// draw a transparent rectangle over muted clips
-		if ( muted )
+		if (muted)
 		{
-			p.setBrush( mutedBackgroundColor() );
-			p.setOpacity( 0.5 );
-			p.drawRect( 0, 0, width(), height() );
+			p.setBrush(mutedBackgroundColor());
+			p.setOpacity(0.5);
+			p.drawRect(0, 0, width(), height());
 		}
 	}
 	// Melody clip and Beat clip (on Song Editor) paint event
-	else if( !noteCollection.empty() )
+	else if (!noteCollection.empty())
 	{
 		// Compute the minimum and maximum key in the clip
 		// so that we know how much there is to draw.
diff --git a/src/tracks/MidiClip.cpp b/src/tracks/MidiClip.cpp
index 01a00dfcf86..8fe2b89eb48 100644
--- a/src/tracks/MidiClip.cpp
+++ b/src/tracks/MidiClip.cpp
@@ -238,7 +238,7 @@ void MidiClip::removeNote( Note * _note_to_del )
 
 
 // Returns a pointer to the note at specified step, or nullptr if note doesn't exist
-Note * MidiClip::noteAtStep( int step )
+Note * MidiClip::noteAtStep(int step)
 {
 	for (const auto& note : m_notes)
 	{

From 4b696159f06d4d6b30a1f22e0eeb5900f7a87092 Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Sun, 9 Jul 2023 21:06:43 -0300
Subject: [PATCH 11/21] Uses std::find_if to save codelines

	As suggested by sakertooth, by using std::find_if we are able to
simplify the checkType method to two lines.
---
 src/tracks/MidiClip.cpp | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/src/tracks/MidiClip.cpp b/src/tracks/MidiClip.cpp
index 8fe2b89eb48..32c1b268d56 100644
--- a/src/tracks/MidiClip.cpp
+++ b/src/tracks/MidiClip.cpp
@@ -25,6 +25,7 @@
 
 #include "MidiClip.h"
 
+#include <algorithm>
 #include <QDomElement>
 
 #include "GuiApplication.h"
@@ -365,15 +366,7 @@ void MidiClip::setType( MidiClipTypes _new_clip_type )
 void MidiClip::checkType()
 {
 	// If all notes are StepNotes, we have a BeatClip
-	bool beatClip = true;
-	for (const auto it : m_notes)
-	{
-		if (it->type() != Note::Type::StepNote)
-		{
-			beatClip = false;
-			break;
-		}
-	}
+	auto beatClip = std::find_if(m_notes.begin(), m_notes.end(), [](auto note) { return note->type() != Note::Type::StepNote; }) == m_notes.end();
 
 	setType(beatClip ? BeatClip : MelodyClip);
 }

From 89a62f6a651b54b7e269e2372984d60cea3960ed Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Mon, 17 Jul 2023 16:22:06 -0300
Subject: [PATCH 12/21] Addresses review from sakertooth

	- Reverts m_detuning in-class initialization
	- Removes testing warning
	- Removes unnecessary comment
---
 include/Note.h       | 3 +--
 src/core/TimePos.cpp | 6 +-----
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/include/Note.h b/include/Note.h
index 7d38634138a..9e5fe13c104 100644
--- a/include/Note.h
+++ b/include/Note.h
@@ -259,9 +259,8 @@ class LMMS_EXPORT Note : public SerializingObject
 	panning_t m_panning;
 	TimePos m_length;
 	TimePos m_pos;
-	DetuningHelper * m_detuning = nullptr;
+	DetuningHelper * m_detuning;
 
-	// The type of this note
 	Type m_type = Type::RegularNote;
 };
 
diff --git a/src/core/TimePos.cpp b/src/core/TimePos.cpp
index 5e8455e01c2..5c62bea8789 100644
--- a/src/core/TimePos.cpp
+++ b/src/core/TimePos.cpp
@@ -161,10 +161,6 @@ tick_t TimePos::getTickWithinBeat( const TimeSig &sig ) const
 
 f_cnt_t TimePos::frames( const float framesPerTick ) const
 {
-	//TODO: Remove next line, it's just a warning to see
-	// if any leftover code is still creating notes with
-	// negative lengths
-	if (m_ticks < 0) { qWarning("TimePos with negative length"); }
 	return static_cast<f_cnt_t>(m_ticks * framesPerTick);
 }
 
@@ -221,4 +217,4 @@ double TimePos::ticksToMilliseconds(double ticks, bpm_t beatsPerMinute)
 }
 
 
-} // namespace lmms
\ No newline at end of file
+} // namespace lmms

From b3511c295d0a1e46af4b3f1e810846dbf0c653eb Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Sat, 22 Jul 2023 21:35:51 -0300
Subject: [PATCH 13/21] Addresses DomClark's review

	- Rename the Note Types enum to avoid redundancy
	- Uses std::all_of instead of std::find_if on MidiClip checkType
	- Rewrites addStepNote so it sets the note type before adding it
to the clip, avoiding having to manually change the type of the clip
after adding the note
---
 include/Note.h                 |  6 +++---
 src/core/DataFile.cpp          |  2 +-
 src/gui/editors/PianoRoll.cpp  |  2 +-
 src/tracks/InstrumentTrack.cpp |  2 +-
 src/tracks/MidiClip.cpp        | 23 +++++++----------------
 5 files changed, 13 insertions(+), 22 deletions(-)

diff --git a/include/Note.h b/include/Note.h
index 9e5fe13c104..4e83fe14699 100644
--- a/include/Note.h
+++ b/include/Note.h
@@ -104,8 +104,8 @@ class LMMS_EXPORT Note : public SerializingObject
 	// Note types
 	enum class Type
 	{
-		RegularNote = 0,
-		StepNote
+		Regular = 0,
+		Step
 	};
 
 	Type type() const
@@ -261,7 +261,7 @@ class LMMS_EXPORT Note : public SerializingObject
 	TimePos m_pos;
 	DetuningHelper * m_detuning;
 
-	Type m_type = Type::RegularNote;
+	Type m_type = Type::Regular;
 };
 
 using NoteVector = QVector<Note*>;
diff --git a/src/core/DataFile.cpp b/src/core/DataFile.cpp
index 0322f976ada..5cb4e231276 100644
--- a/src/core/DataFile.cpp
+++ b/src/core/DataFile.cpp
@@ -1684,7 +1684,7 @@ void DataFile::upgrade_noteTypes()
 		if (noteSize < 0)
 		{
 			note.setAttribute("len", DefaultTicksPerBar / 16);
-			note.setAttribute("type", static_cast<int>(Note::Type::StepNote));
+			note.setAttribute("type", static_cast<int>(Note::Type::Step));
 		}
 	}
 }
diff --git a/src/gui/editors/PianoRoll.cpp b/src/gui/editors/PianoRoll.cpp
index 9736ccb2274..82152f9b7ec 100644
--- a/src/gui/editors/PianoRoll.cpp
+++ b/src/gui/editors/PianoRoll.cpp
@@ -3501,7 +3501,7 @@ void PianoRoll::paintEvent(QPaintEvent * pe )
 			{
 				// We've done and checked all, let's draw the note with
 				// the appropriate color
-				const auto fillColor = note->type() == Note::Type::RegularNote ? m_noteColor : m_stepNoteColor;
+				const auto fillColor = note->type() == Note::Type::Regular ? m_noteColor : m_stepNoteColor;
 
 				drawNoteRect(
 					p, x + m_whiteKeyWidth, noteYPos(note->key()), note_width,
diff --git a/src/tracks/InstrumentTrack.cpp b/src/tracks/InstrumentTrack.cpp
index 22e8d2a3d5c..01de94affd1 100644
--- a/src/tracks/InstrumentTrack.cpp
+++ b/src/tracks/InstrumentTrack.cpp
@@ -760,7 +760,7 @@ bool InstrumentTrack::play( const TimePos & _start, const fpp_t _frames,
 		{
 			// If the note is a Step Note, frames will be 0 so the NotePlayHandle
 			// plays for the whole length of the sample
-			const auto note_frames = cur_note->type() == Note::Type::StepNote
+			const auto note_frames = cur_note->type() == Note::Type::Step
 				? 0
 				: cur_note->length().frames(frames_per_tick);
 
diff --git a/src/tracks/MidiClip.cpp b/src/tracks/MidiClip.cpp
index 32c1b268d56..b635fc967d0 100644
--- a/src/tracks/MidiClip.cpp
+++ b/src/tracks/MidiClip.cpp
@@ -175,7 +175,7 @@ TimePos MidiClip::beatClipLength() const
 
 	for (const auto& note : m_notes)
 	{
-		if (note->type() == Note::Type::StepNote)
+		if (note->type() == Note::Type::Step)
 		{
 			max_length = qMax<tick_t>(max_length, note->pos() + 1);
 		}
@@ -244,7 +244,7 @@ Note * MidiClip::noteAtStep(int step)
 	for (const auto& note : m_notes)
 	{
 		if (note->pos() == TimePos::stepPosition(step)
-			&& note->type() == Note::Type::StepNote)
+			&& note->type() == Note::Type::Step)
 		{
 			return note;
 		}
@@ -281,19 +281,10 @@ void MidiClip::clearNotes()
 
 Note * MidiClip::addStepNote( int step )
 {
-	const auto prevPatternType = m_clipType;
-	Note * n =
-		addNote(
-			Note(TimePos(DefaultTicksPerBar / 16),
-			TimePos::stepPosition(step)), false
-		);
-	n->setType(Note::Type::StepNote);
-	// addNote will change a BeatClip to a MelodyClip
-	// because it calls checkType after adding a regular note.
-	// We need to revert it back to a BeatClip if it was one
-	// before we added the note.
-	if (prevPatternType == BeatClip) { setType(BeatClip); }
-	return n;
+	Note stepNote = Note(TimePos(DefaultTicksPerBar / 16), TimePos::stepPosition(step));
+	stepNote.setType(Note::Type::Step);
+
+	return addNote(stepNote, false);
 }
 
 
@@ -366,7 +357,7 @@ void MidiClip::setType( MidiClipTypes _new_clip_type )
 void MidiClip::checkType()
 {
 	// If all notes are StepNotes, we have a BeatClip
-	auto beatClip = std::find_if(m_notes.begin(), m_notes.end(), [](auto note) { return note->type() != Note::Type::StepNote; }) == m_notes.end();
+	const auto beatClip = std::all_of(m_notes.begin(), m_notes.end(), [](auto note) { return note->type() == Note::Type::Step; });
 
 	setType(beatClip ? BeatClip : MelodyClip);
 }

From dbefa9c0f3e731985fc98e2a82ff35efeb34dad3 Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Mon, 21 Aug 2023 01:05:24 -0300
Subject: [PATCH 14/21] Updates MidiExport to use Note Types

	- Now MidiExport is updated to use note types instead of relying
on negative length notes.
	- For that change it was necessary to find a way of letting
MidiExport know how long step notes should be. The solution found was to
add an attribute to the Instrument XML called "beatlen", which would
hold the number of frames of the instrument's beat. That would be
converted to ticks, so we could calculate how long the MIDI notes would
have to be to play the whole step note. If the attribute was not found,
the default value of 16 ticks would be used as a length of step notes,
as a fallback.
---
 plugins/MidiExport/MidiExport.cpp | 33 ++++++++++++++++++++++++++-----
 plugins/MidiExport/MidiExport.h   |  4 +++-
 src/tracks/InstrumentTrack.cpp    |  1 +
 3 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/plugins/MidiExport/MidiExport.cpp b/plugins/MidiExport/MidiExport.cpp
index 0d18d8ae156..27c3f96a600 100644
--- a/plugins/MidiExport/MidiExport.cpp
+++ b/plugins/MidiExport/MidiExport.cpp
@@ -27,6 +27,7 @@
 
 #include "MidiExport.h"
 
+#include "Engine.h"
 #include "TrackContainer.h"
 #include "DataFile.h"
 #include "InstrumentTrack.h"
@@ -113,6 +114,7 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks,
 			int base_pitch = 0;
 			double base_volume = 1.0;
 			int base_time = 0;
+			int base_beatLen = 16;
 
 			MidiNoteVector midiClip;
 
@@ -128,6 +130,17 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks,
 						base_pitch += masterPitch;
 					}
 					base_volume = LocaleHelper::toDouble(it.attribute("volume", "100"))/100.0;
+					// From the PR 5902 forward (Note Types), the instrument XML includes the beat length
+					// in frames. We try to fetch it and convert to the length in ticks. If there's no beat
+					// length in XML, the default beat length of 16 ticks will be used.
+					QDomNode iNode = it.elementsByTagName("instrument").at(0);
+					if(!iNode.isNull()){
+						QDomElement i = iNode.toElement();
+						if(i.hasAttribute("beatlen"))
+						{
+							base_beatLen = i.attribute("beatlen", "0").toInt() / Engine::framesPerTick();
+						}
+					}
 				}
 
 				if (n.nodeName() == "midiclip")
@@ -137,7 +150,7 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks,
 				}
 
 			}
-			processPatternNotes(midiClip, INT_MAX);
+			processPatternNotes(midiClip, base_beatLen, INT_MAX);
 			writeMidiClipToTrack(mtrack, midiClip);
 			size = mtrack.writeToBuffer(buffer.data());
 			midiout.writeRawData((char *)buffer.data(), size);
@@ -188,6 +201,7 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks,
 
 		int base_pitch = 0;
 		double base_volume = 1.0;
+		int base_beatLen = 16;
 
 		// for each pattern in the pattern editor
 		for (QDomNode n = element.firstChild(); !n.isNull(); n = n.nextSibling())
@@ -201,6 +215,13 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks,
 					base_pitch += masterPitch;
 				}
 				base_volume = LocaleHelper::toDouble(it.attribute("volume", "100")) / 100.0;
+				QDomNode iNode = it.elementsByTagName("instrument").at(0);
+				if(!iNode.isNull()){
+					QDomElement i = iNode.toElement();
+					if(i.hasAttribute("beatlen")){
+						base_beatLen = i.attribute("beatlen", "0").toInt()/Engine::framesPerTick();
+					}
+				}
 			}
 
 			if (n.nodeName() == "midiclip")
@@ -247,7 +268,7 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks,
 					st.pop_back();
 				}
 
-				processPatternNotes(nv, pos);
+				processPatternNotes(nv, base_beatLen, pos);
 				writeMidiClipToTrack(mtrack, nv);
 
 				// next pattern track
@@ -279,6 +300,7 @@ void MidiExport::writeMidiClip(MidiNoteVector &midiClip, const QDomNode& n,
 		mnote.volume = qMin(qRound(base_volume * LocaleHelper::toDouble(note.attribute("vol", "100")) * (127.0 / 200.0)), 127);
 		mnote.time = base_time + note.attribute("pos", "0").toInt();
 		mnote.duration = note.attribute("len", "0").toInt();
+		mnote.type = static_cast<Note::Type>(note.attribute("type", "0").toInt());
 		midiClip.push_back(mnote);
 	}
 }
@@ -311,6 +333,7 @@ void MidiExport::writePatternClip(MidiNoteVector& src, MidiNoteVector& dst,
 			note.pitch = srcNote.pitch;
 			note.time = base + time;
 			note.volume = srcNote.volume;
+			note.type = srcNote.type;
 			dst.push_back(note);
 		}
 	}
@@ -318,7 +341,7 @@ void MidiExport::writePatternClip(MidiNoteVector& src, MidiNoteVector& dst,
 
 
 
-void MidiExport::processPatternNotes(MidiNoteVector& nv, int cutPos)
+void MidiExport::processPatternNotes(MidiNoteVector& nv, int beatLen, int cutPos)
 {
 	std::sort(nv.begin(), nv.end());
 	int cur = INT_MAX, next = INT_MAX;
@@ -329,9 +352,9 @@ void MidiExport::processPatternNotes(MidiNoteVector& nv, int cutPos)
 			next = cur;
 			cur = it->time;
 		}
-		if (it->duration < 0)
+		if (it->type == Note::Type::Step)
 		{
-			it->duration = qMin(qMin(-it->duration, next - cur), cutPos - it->time);
+			it->duration = qMin(qMin(beatLen, next - cur), cutPos - it->time);
 		}
 	}
 }
diff --git a/plugins/MidiExport/MidiExport.h b/plugins/MidiExport/MidiExport.h
index 1e355e45ac9..e2c0833373b 100644
--- a/plugins/MidiExport/MidiExport.h
+++ b/plugins/MidiExport/MidiExport.h
@@ -30,6 +30,7 @@
 
 #include "ExportFilter.h"
 #include "MidiFile.hpp"
+#include "Note.h"
 
 class QDomNode;
 
@@ -46,6 +47,7 @@ struct MidiNote
 	uint8_t pitch;
 	int duration;
 	uint8_t volume;
+	Note::Type type;
 
 	inline bool operator<(const MidiNote &b) const
 	{
@@ -78,7 +80,7 @@ class MidiExport: public ExportFilter
 	void writeMidiClipToTrack(MTrack &mtrack, MidiNoteVector &nv);
 	void writePatternClip(MidiNoteVector &src, MidiNoteVector &dst,
 				int len, int base, int start, int end);
-	void processPatternNotes(MidiNoteVector &nv, int cutPos);
+	void processPatternNotes(MidiNoteVector &nv, int beatLen, int cutPos);
 
 	void error();
 
diff --git a/src/tracks/InstrumentTrack.cpp b/src/tracks/InstrumentTrack.cpp
index 01de94affd1..f40c887fc85 100644
--- a/src/tracks/InstrumentTrack.cpp
+++ b/src/tracks/InstrumentTrack.cpp
@@ -831,6 +831,7 @@ void InstrumentTrack::saveTrackSpecificSettings( QDomDocument& doc, QDomElement
 	{
 		QDomElement i = doc.createElement( "instrument" );
 		i.setAttribute( "name", m_instrument->descriptor()->name );
+		i.setAttribute("beatlen", static_cast<int>(beatLen(nullptr)));
 		QDomElement ins = m_instrument->saveState( doc, i );
 		if(m_instrument->key().isValid()) {
 			ins.appendChild( m_instrument->key().saveXML( doc ) );

From 0054ecedd95f1e8284d92e375dfdea665221f828 Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Mon, 28 Aug 2023 16:01:35 -0300
Subject: [PATCH 15/21] Fixes ambiguity on enum usage

	Due to changes in the name of enum classes, there was an
ambiguity caused in NotePlayHandle.cpp. That was fixed.
---
 src/core/NotePlayHandle.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/core/NotePlayHandle.cpp b/src/core/NotePlayHandle.cpp
index 70007ebf187..5c2faa3dc55 100644
--- a/src/core/NotePlayHandle.cpp
+++ b/src/core/NotePlayHandle.cpp
@@ -53,7 +53,7 @@ NotePlayHandle::NotePlayHandle( InstrumentTrack* instrumentTrack,
 								NotePlayHandle *parent,
 								int midiEventChannel,
 								Origin origin ) :
-	PlayHandle( Type::NotePlayHandle, _offset ),
+	PlayHandle( PlayHandle::Type::NotePlayHandle, _offset ),
 	Note( n.length(), n.pos(), n.key(), n.getVolume(), n.getPanning(), n.detuning() ),
 	m_pluginData( nullptr ),
 	m_instrumentTrack( instrumentTrack ),

From 38dd8ecfa9840b807f30f2c5b2f1af0f03abeced Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Tue, 17 Oct 2023 17:54:46 -0300
Subject: [PATCH 16/21] Addresses new code reviews

	- Addresses code review from PhysSong and Messmerd
---
 include/DataFile.h             | 2 +-
 include/Note.h                 | 5 +----
 src/core/Note.cpp              | 1 +
 src/core/TimePos.cpp           | 4 ++++
 src/gui/clips/MidiClipView.cpp | 7 +++----
 5 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/include/DataFile.h b/include/DataFile.h
index ef245fd7e9d..6e25d4df1fa 100644
--- a/include/DataFile.h
+++ b/include/DataFile.h
@@ -128,7 +128,7 @@ class LMMS_EXPORT DataFile : public QDomDocument
 	void upgrade_bbTcoRename();
 	void upgrade_sampleAndHold();
 	void upgrade_midiCCIndexing();
-  void upgrade_noteTypes();
+	void upgrade_noteTypes();
 
 	// List of all upgrade methods
 	static const std::vector<UpgradeMethod> UPGRADE_METHODS;
diff --git a/include/Note.h b/include/Note.h
index 3f172116a09..08cbce3dbeb 100644
--- a/include/Note.h
+++ b/include/Note.h
@@ -114,10 +114,7 @@ class LMMS_EXPORT Note : public SerializingObject
 		Step
 	};
 
-	Type type() const
-	{
-		return m_type;
-	}
+	Type type() const { return m_type; }
 	inline void setType(Type t) { m_type = t; }
 
 	// used by GUI
diff --git a/src/core/Note.cpp b/src/core/Note.cpp
index f04a23a693c..ed3a00f1017 100644
--- a/src/core/Note.cpp
+++ b/src/core/Note.cpp
@@ -200,6 +200,7 @@ void Note::loadSettings( const QDomElement & _this )
 	m_length = _this.attribute( "len" ).toInt();
 	m_pos = _this.attribute( "pos" ).toInt();
 	// Default m_type value is 0, which corresponds to RegularNote
+	static_assert(0 == static_cast<int>(Type::Regular));
 	m_type = static_cast<Type>(_this.attribute("type", "0").toInt());
 
 	if( _this.hasChildNodes() )
diff --git a/src/core/TimePos.cpp b/src/core/TimePos.cpp
index c0ba76672ce..d745edc053e 100644
--- a/src/core/TimePos.cpp
+++ b/src/core/TimePos.cpp
@@ -161,6 +161,10 @@ tick_t TimePos::getTickWithinBeat( const TimeSig &sig ) const
 
 f_cnt_t TimePos::frames( const float framesPerTick ) const
 {
+	// Before, step notes used to have negative length. This
+	// assert is a safeguard against negative length being
+	// introduced again (now using Note Types instead #5902)
+	assert(m_ticks >= 0);
 	return static_cast<f_cnt_t>(m_ticks * framesPerTick);
 }
 
diff --git a/src/gui/clips/MidiClipView.cpp b/src/gui/clips/MidiClipView.cpp
index 5c067bb06dc..7c04090b149 100644
--- a/src/gui/clips/MidiClipView.cpp
+++ b/src/gui/clips/MidiClipView.cpp
@@ -468,8 +468,7 @@ void MidiClipView::paintEvent( QPaintEvent * )
 		QPixmap stepon200;
 		QPixmap stepoff;
 		QPixmap stepoffl;
-		const int steps = std::max(1,
-					m_clip->m_steps);
+		const int steps = std::max(1, m_clip->m_steps);
 		const int w = width() - 2 * BORDER_WIDTH;
 
 		// scale step graphics to fit the beat clip length
@@ -492,7 +491,7 @@ void MidiClipView::paintEvent( QPaintEvent * )
 
 		for (int it = 0; it < steps; it++)	// go through all the steps in the beat clip
 		{
-			Note * n = m_clip->noteAtStep(it);
+			Note* n = m_clip->noteAtStep(it);
 
 			// figure out x and y coordinates for step graphic
 			const int x = BORDER_WIDTH + static_cast<int>(it * w / steps);
@@ -503,7 +502,7 @@ void MidiClipView::paintEvent( QPaintEvent * )
 				const int vol = n->getVolume();
 				p.drawPixmap(x, y, stepoffl);
 				p.drawPixmap(x, y, stepon0);
-				p.setOpacity(sqrt( vol / 200.0 ));
+				p.setOpacity(std::sqrt(vol / 200.0));
 				p.drawPixmap(x, y, stepon200);
 				p.setOpacity(1);
 			}

From 31a2bd3d494f89251b1ebb1895e7a34f30dec8fc Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Tue, 17 Oct 2023 18:04:58 -0300
Subject: [PATCH 17/21] Fixes note drawing on Song Editor

	- Notes were not being draw on the song editor for BeatClips.
This commit fixes this.
---
 src/gui/clips/MidiClipView.cpp | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/gui/clips/MidiClipView.cpp b/src/gui/clips/MidiClipView.cpp
index 7c04090b149..b13d6e00397 100644
--- a/src/gui/clips/MidiClipView.cpp
+++ b/src/gui/clips/MidiClipView.cpp
@@ -525,7 +525,12 @@ void MidiClipView::paintEvent( QPaintEvent * )
 		}
 	}
 	// Melody clip and Beat clip (on Song Editor) paint event
-	else if (m_clip->m_clipType == MidiClip::Type::MelodyClip && !noteCollection.empty())
+	else if
+	(
+		!noteCollection.empty() &&
+			(m_clip->m_clipType == MidiClip::Type::MelodyClip ||
+			m_clip->m_clipType == MidiClip::Type::BeatClip)
+	)
 	{
 		// Compute the minimum and maximum key in the clip
 		// so that we know how much there is to draw.

From 43018d72981f18971f9c351e1e61e5149396c4a8 Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Wed, 18 Oct 2023 08:20:42 -0300
Subject: [PATCH 18/21] Adds cassert header to TimePos.cpp

	- Adds header to use assert() on TimePos.cpp
---
 src/core/TimePos.cpp | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/core/TimePos.cpp b/src/core/TimePos.cpp
index d745edc053e..50726fdc139 100644
--- a/src/core/TimePos.cpp
+++ b/src/core/TimePos.cpp
@@ -23,6 +23,7 @@
  *
  */
 
+#include <cassert>
 #include "TimePos.h"
 
 #include "MeterModel.h"

From 389a2455f80a3f4150d583b74ae6a2db3abb21e1 Mon Sep 17 00:00:00 2001
From: IanCaio <iancaio_dev@hotmail.com>
Date: Wed, 25 Oct 2023 23:15:39 -0300
Subject: [PATCH 19/21] Apply suggestions from code review

Fixes style on some lines

Co-authored-by: Dalton Messmer <33463986+messmerd@users.noreply.github.com>
---
 plugins/MidiExport/MidiExport.cpp | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/plugins/MidiExport/MidiExport.cpp b/plugins/MidiExport/MidiExport.cpp
index e85dc22ada7..193d943efe5 100644
--- a/plugins/MidiExport/MidiExport.cpp
+++ b/plugins/MidiExport/MidiExport.cpp
@@ -134,9 +134,10 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks,
 					// in frames. We try to fetch it and convert to the length in ticks. If there's no beat
 					// length in XML, the default beat length of 16 ticks will be used.
 					QDomNode iNode = it.elementsByTagName("instrument").at(0);
-					if(!iNode.isNull()){
+					if (!iNode.isNull())
+					{
 						QDomElement i = iNode.toElement();
-						if(i.hasAttribute("beatlen"))
+						if (i.hasAttribute("beatlen"))
 						{
 							base_beatLen = i.attribute("beatlen", "0").toInt() / Engine::framesPerTick();
 						}
@@ -216,9 +217,11 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks,
 				}
 				base_volume = LocaleHelper::toDouble(it.attribute("volume", "100")) / 100.0;
 				QDomNode iNode = it.elementsByTagName("instrument").at(0);
-				if(!iNode.isNull()){
+				if (!iNode.isNull())
+				{
 					QDomElement i = iNode.toElement();
-					if(i.hasAttribute("beatlen")){
+					if (i.hasAttribute("beatlen"))
+					{
 						base_beatLen = i.attribute("beatlen", "0").toInt()/Engine::framesPerTick();
 					}
 				}

From dcb41af1eae5e5795cd88d82ad1a59d700caabf5 Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Fri, 3 Nov 2023 18:22:09 -0300
Subject: [PATCH 20/21] Reverts some changes on MidiExport

	- Some changes were reverted on MidiExport and InstrumentTrack.
We were storing the beat length on the XML of Instrument Tracks, but in
reality the beat length is a per note attribute, and some instruments
could run into a segmentation fault when calling beat length without a
NotePlayHandle (i.e.: AFP). Because of that I reverted this change, so
the beat length is not stored on the XML anymore, and instead we have a
magic number on the MidiExport class that holds a default beat length
which is actually an upper limit for the MIDI notes of step notes. In
the future we can improve this by finding a way to store the beat length
on the note class to use it instead. The MidiExport logic is not
worsened at all because previously the beat length wasn't even
considered during export (it was actually improved making the exported
notes extend until the next one instead of cutting shorter).
---
 plugins/MidiExport/MidiExport.cpp | 31 ++++---------------------------
 plugins/MidiExport/MidiExport.h   | 12 +++++++++++-
 src/tracks/InstrumentTrack.cpp    |  1 -
 3 files changed, 15 insertions(+), 29 deletions(-)

diff --git a/plugins/MidiExport/MidiExport.cpp b/plugins/MidiExport/MidiExport.cpp
index 193d943efe5..2600a40f2f9 100644
--- a/plugins/MidiExport/MidiExport.cpp
+++ b/plugins/MidiExport/MidiExport.cpp
@@ -114,7 +114,6 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks,
 			int base_pitch = 0;
 			double base_volume = 1.0;
 			int base_time = 0;
-			int base_beatLen = 16;
 
 			MidiNoteVector midiClip;
 
@@ -130,18 +129,6 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks,
 						base_pitch += masterPitch;
 					}
 					base_volume = LocaleHelper::toDouble(it.attribute("volume", "100"))/100.0;
-					// From the PR 5902 forward (Note Types), the instrument XML includes the beat length
-					// in frames. We try to fetch it and convert to the length in ticks. If there's no beat
-					// length in XML, the default beat length of 16 ticks will be used.
-					QDomNode iNode = it.elementsByTagName("instrument").at(0);
-					if (!iNode.isNull())
-					{
-						QDomElement i = iNode.toElement();
-						if (i.hasAttribute("beatlen"))
-						{
-							base_beatLen = i.attribute("beatlen", "0").toInt() / Engine::framesPerTick();
-						}
-					}
 				}
 
 				if (n.nodeName() == "midiclip")
@@ -151,7 +138,7 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks,
 				}
 
 			}
-			processPatternNotes(midiClip, base_beatLen, INT_MAX);
+			processPatternNotes(midiClip, INT_MAX);
 			writeMidiClipToTrack(mtrack, midiClip);
 			size = mtrack.writeToBuffer(buffer.data());
 			midiout.writeRawData((char *)buffer.data(), size);
@@ -202,7 +189,6 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks,
 
 		int base_pitch = 0;
 		double base_volume = 1.0;
-		int base_beatLen = 16;
 
 		// for each pattern in the pattern editor
 		for (QDomNode n = element.firstChild(); !n.isNull(); n = n.nextSibling())
@@ -216,15 +202,6 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks,
 					base_pitch += masterPitch;
 				}
 				base_volume = LocaleHelper::toDouble(it.attribute("volume", "100")) / 100.0;
-				QDomNode iNode = it.elementsByTagName("instrument").at(0);
-				if (!iNode.isNull())
-				{
-					QDomElement i = iNode.toElement();
-					if (i.hasAttribute("beatlen"))
-					{
-						base_beatLen = i.attribute("beatlen", "0").toInt()/Engine::framesPerTick();
-					}
-				}
 			}
 
 			if (n.nodeName() == "midiclip")
@@ -271,7 +248,7 @@ bool MidiExport::tryExport(const TrackContainer::TrackList &tracks,
 					st.pop_back();
 				}
 
-				processPatternNotes(nv, base_beatLen, pos);
+				processPatternNotes(nv, pos);
 				writeMidiClipToTrack(mtrack, nv);
 
 				// next pattern track
@@ -344,7 +321,7 @@ void MidiExport::writePatternClip(MidiNoteVector& src, MidiNoteVector& dst,
 
 
 
-void MidiExport::processPatternNotes(MidiNoteVector& nv, int beatLen, int cutPos)
+void MidiExport::processPatternNotes(MidiNoteVector& nv, int cutPos)
 {
 	std::sort(nv.begin(), nv.end());
 	int cur = INT_MAX, next = INT_MAX;
@@ -357,7 +334,7 @@ void MidiExport::processPatternNotes(MidiNoteVector& nv, int beatLen, int cutPos
 		}
 		if (it->type == Note::Type::Step)
 		{
-			it->duration = qMin(qMin(beatLen, next - cur), cutPos - it->time);
+			it->duration = qMin(qMin(DefaultBeatLength, next - cur), cutPos - it->time);
 		}
 	}
 }
diff --git a/plugins/MidiExport/MidiExport.h b/plugins/MidiExport/MidiExport.h
index e2c0833373b..7c77c7af252 100644
--- a/plugins/MidiExport/MidiExport.h
+++ b/plugins/MidiExport/MidiExport.h
@@ -65,6 +65,16 @@ class MidiExport: public ExportFilter
 	MidiExport();
 	~MidiExport() override = default;
 
+	// Default Beat Length in ticks for step notes
+	// TODO: The beat length actually varies per note, however the method that
+	// calculates it (InstrumentTrack::beatLen) requires a NotePlayHandle to do
+	// so. While we don't figure out a way to hold the beat length of each note
+	// on its member variables, we will use a default value as a beat length that
+	// will be used as an upper limit of the midi note length. This doesn't worsen
+	// the current logic used for MidiExport because right now the beat length is
+	// not even considered during the generation of the MIDI.
+	static constexpr int DefaultBeatLength = 1500;
+
 	gui::PluginView* instantiateView(QWidget *) override
 	{
 		return nullptr;
@@ -80,7 +90,7 @@ class MidiExport: public ExportFilter
 	void writeMidiClipToTrack(MTrack &mtrack, MidiNoteVector &nv);
 	void writePatternClip(MidiNoteVector &src, MidiNoteVector &dst,
 				int len, int base, int start, int end);
-	void processPatternNotes(MidiNoteVector &nv, int beatLen, int cutPos);
+	void processPatternNotes(MidiNoteVector &nv, int cutPos);
 
 	void error();
 
diff --git a/src/tracks/InstrumentTrack.cpp b/src/tracks/InstrumentTrack.cpp
index 0c83be51601..7f88bb51a83 100644
--- a/src/tracks/InstrumentTrack.cpp
+++ b/src/tracks/InstrumentTrack.cpp
@@ -848,7 +848,6 @@ void InstrumentTrack::saveTrackSpecificSettings( QDomDocument& doc, QDomElement
 	{
 		QDomElement i = doc.createElement( "instrument" );
 		i.setAttribute( "name", m_instrument->descriptor()->name );
-		i.setAttribute("beatlen", static_cast<int>(beatLen(nullptr)));
 		QDomElement ins = m_instrument->saveState( doc, i );
 		if(m_instrument->key().isValid()) {
 			ins.appendChild( m_instrument->key().saveXML( doc ) );

From 67b0b2418b61950ac22c6562793306010b78e35d Mon Sep 17 00:00:00 2001
From: Ian Caio <iancaio_dev@hotmail.com>
Date: Sat, 18 Nov 2023 14:06:18 -0300
Subject: [PATCH 21/21] Fix the order of included files

---
 src/core/TimePos.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/core/TimePos.cpp b/src/core/TimePos.cpp
index 50726fdc139..09c1019bcef 100644
--- a/src/core/TimePos.cpp
+++ b/src/core/TimePos.cpp
@@ -23,9 +23,9 @@
  *
  */
 
-#include <cassert>
 #include "TimePos.h"
 
+#include <cassert>
 #include "MeterModel.h"
 
 namespace lmms