Addresses my own code review

	This commit addresses some fixes from a review I made from the
code on Github and also one requested change from Veratil.

	Changes:
	- Fixes code style issues.
	- Adds a helper MACRO to return the offset between the inValue
and outValue of a node and use it where getValueOffset was called.
	- Removes conditional that was not needed inside
AutomationPattern::valueAt.
	- Updates InlineAutomation.h to use the helper MACROs and
account for the outValue when deciding whether to save or discard an
inline automation.
	- Adds TODO comments on two loops present on
src/code/AutomationPattern.cpp that could be optimized.
	- Fixes some comments.
	- Uses INVAL(it) instead of valueAt(POS(it)) on the flip methods
when possible.
	- Adds a resetOutValue() method to AutomationNode and use it
where convenient.
	- Fixes a small "bug" where flipping a pattern from the TCO
context menu, when the pattern is smaller than the TCO, would use the
inValue of the last node for the node created at the end of the TCO
instead of the outValue (which is the value that the position would have
if the pattern continued).

include/AutomationNode.h

@@ -29,6 +29,7 @@
 // MACROs to help handling automation nodes
 #define INVAL(x) ((x).value().getInValue())
 #define OUTVAL(x) ((x).value().getOutValue())
+#define OFFSET(x) ((x).value().getValueOffset())
 #define POS(x) ((x).key())
 
 class AutomationPattern;
@@ -87,6 +88,14 @@ class AutomationNode
 		return m_outValue - m_inValue;
 	}
 
+	/**
+	 * @brief Resets the outValue so it matches inValue
+	 */
+	inline void resetOutValue()
+	{
+		m_outValue = m_inValue;
+	}
+
 	/**
 	 * @brief Gets the tangent of the left side of the node
 	 * @return Float with the tangent from the inValue side

include/InlineAutomation.h

@@ -26,6 +26,7 @@
 #define INLINE_AUTOMATION_H
 
 #include "AutomationPattern.h"
+#include "AutomationNode.h"
 #include "shared_object.h"
 
 
@@ -53,12 +54,17 @@ class InlineAutomation : public FloatModel, public sharedObject
 	{
 		if( m_autoPattern != NULL && m_autoPattern->getTimeMap().isEmpty() == false )
 		{
-			// prevent saving inline automation if there's just one value which equals value
-			// of model which is going to be saved anyways
+			// Prevent saving inline automation if there's just one node at the beginning of
+			// the pattern, which has a InValue equal to the value of model (which is going
+			// to be saved anyways) and no offset between the InValue and OutValue
+			AutomationPattern::timeMap::const_iterator firstNode =
+				m_autoPattern->getTimeMap().begin();
+
 			if (isAtInitValue() &&
 				m_autoPattern->getTimeMap().size() == 1 &&
-				m_autoPattern->getTimeMap().begin().key() == 0 &&
-				m_autoPattern->getTimeMap().begin().value().getInValue() == value())
+				POS(firstNode) == 0 &&
+				INVAL(firstNode) == value() &&
+				OFFSET(firstNode) == 0)
 			{
 				return false;
 			}

src/core/AutomationPattern.cpp

@@ -246,6 +246,9 @@ MidiTime AutomationPattern::putValue(const MidiTime & time,
 	// quantization value. Control Key to override
 	if (!ignoreSurroundingPoints)
 	{
+		// TODO: This loop can be optimized by going through the nodes
+		// between the quantized times instead of calling removeValue
+		// for each tick even when there are no nodes there.
 		for (int i = newTime + 1; i < newTime + quantization(); ++i)
 		{
 			AutomationPattern::removeValue(i);
@@ -298,6 +301,9 @@ MidiTime AutomationPattern::putValues(const MidiTime & time,
 	// quantization value. Control Key to override
 	if (!ignoreSurroundingPoints)
 	{
+		// TODO: This loop can be optimized by going through the nodes
+		// between the quantized times instead of calling removeValue
+		// for each tick even when there are no nodes there.
 		for (int i = newTime + 1; i < newTime + quantization(); ++i)
 		{
 			AutomationPattern::removeValue(i);
@@ -388,7 +394,7 @@ MidiTime AutomationPattern::setDragValue(const MidiTime & time,
 		timeMap::iterator it = m_timeMap.find(newTime);
 		if (it != m_timeMap.end())
 		{
-			if (it.value().getValueOffset() != 0)
+			if (OFFSET(it) != 0)
 			{
 				m_dragKeepOutValue = true;
 				m_dragOutValue = OUTVAL(it);
@@ -445,8 +451,9 @@ float AutomationPattern::valueAt( const MidiTime & _time ) const
 		return m_timeMap[_time].getInValue();
 	}
 
-	// lowerBound returns next value with greater key, therefore we take
-	// the previous element to get the current value
+	// lowerBound returns next value with equal or greater key. Since we already
+	// checked if the key contains a node, we know the returned node has a greater
+	// key than _time. Therefore we take the previous element to calculate the current value
 	timeMap::const_iterator v = m_timeMap.lowerBound(_time);
 
 	if( v == m_timeMap.begin() )
@@ -466,10 +473,7 @@ float AutomationPattern::valueAt( const MidiTime & _time ) const
 
 
 // This method will get the value at an offset from a node, so we use the outValue of
-// that node and the inValue of the next node to the calculations. This assumes that offset
-// will not be zero, because when the midi time given to AutomationPattern::valueAt(MidiTime)
-// matches a node's position, that node's value will be returned and this method won't be even
-// called.
+// that node and the inValue of the next node for the calculations.
 float AutomationPattern::valueAt( timeMap::const_iterator v, int offset ) const
 {
 	QMutexLocker m(&m_patternMutex);
@@ -481,7 +485,7 @@ float AutomationPattern::valueAt( timeMap::const_iterator v, int offset ) const
 		return INVAL(v);
 	}
 
-	if( m_progressionType == DiscreteProgression || v == m_timeMap.end() )
+	if (m_progressionType == DiscreteProgression)
 	{
 		return OUTVAL(v);
 	}
@@ -548,7 +552,7 @@ void AutomationPattern::flipY(int min, int max)
 
 	timeMap::iterator it = m_timeMap.lowerBound(0);
 
-	if (it == m_timeMap.end()) return;
+	if (it == m_timeMap.end()) { return; }
 
 	float tempValue = 0;
 	float outValueOffset = 0;
@@ -562,7 +566,7 @@ void AutomationPattern::flipY(int min, int max)
 		else
 		{
 			tempValue = max - valueAt(POS(it));
-			outValueOffset = it.value().getValueOffset() * -1.0;
+			outValueOffset = OFFSET(it) * -1.0;
 			putValues(MidiTime(POS(it)), tempValue, tempValue + outValueOffset, false, true);
 		}
 		++it;
@@ -589,7 +593,7 @@ void AutomationPattern::flipX(int length)
 
 	timeMap::const_iterator it = m_timeMap.lowerBound(0);
 
-	if (it == m_timeMap.end()) return;
+	if (it == m_timeMap.end()) { return; }
 
 	// Temporary map where we will store the flipped version
 	// of our pattern
@@ -611,13 +615,13 @@ void AutomationPattern::flipX(int length)
 		{
 			// We are flipping an area that goes beyond the last node. So we add a node to the
 			// beginning of the flipped timeMap representing the value of the end of the area
-			tempValue = valueAt(realLength);
+			tempValue = valueAt(length);
 			tempMap[0] = AutomationNode(this, tempValue, 0);
 
 			// Now flip the nodes we have in relation to the length
 			do
 			{
-				tempValue = valueAt(POS(it));
+				tempValue = INVAL(it);
 				tempOutValue = OUTVAL(it);
 				MidiTime newTime = MidiTime(length - POS(it));
 
@@ -630,7 +634,7 @@ void AutomationPattern::flipX(int length)
 		{
 			do
 			{
-				tempValue = valueAt(POS(it));
+				tempValue = INVAL(it);
 				tempOutValue = OUTVAL(it);
 				MidiTime newTime;
 
@@ -647,7 +651,7 @@ void AutomationPattern::flipX(int length)
 	{
 		do
 		{
-			tempValue = valueAt(POS(it));
+			tempValue = INVAL(it);
 			tempOutValue = OUTVAL(it);
 
 			MidiTime newTime = MidiTime(realLength - POS(it));
@@ -832,7 +836,7 @@ bool AutomationPattern::isAutomated( const AutomatableModel * _m )
 
 
 /**
- * @brief returns a list of all the automation patterns everywhere that are connected to a specific model
+ * @brief returns a list of all the automation patterns that are connected to a specific model
  * @param _m the model we want to look for
  */
 QVector<AutomationPattern *> AutomationPattern::patternsForModel( const AutomatableModel * _m )

src/gui/editors/AutomationEditor.cpp

@@ -558,7 +558,7 @@ void AutomationEditor::mousePressEvent( QMouseEvent* mouseEvent )
 
 					m_action = ERASE_VALUES;
 				}
-				else if(m_mouseDownRight) // And right click resets outValues
+				else if (m_mouseDownRight) // And right click resets outValues
 				{
 					// If we clicked an outValue reset it
 					if (clickedNode != tm.end())
@@ -620,7 +620,7 @@ void AutomationEditor::mousePressEvent( QMouseEvent* mouseEvent )
 					// If we clicked an outValue reset it
 					if (clickedNode != tm.end())
 					{
-						clickedNode.value().setOutValue(INVAL(clickedNode));
+						clickedNode.value().resetOutValue();
 						Engine::getSong()->setModified();
 					}
 
@@ -740,7 +740,7 @@ void AutomationEditor::resetNodes(int tick0, int tick1)
 	{
 		if (POS(it) > end) { return; }
 
-		it.value().setOutValue(INVAL(it));
+		it.value().resetOutValue();
 		++it;
 	}
 }
@@ -779,7 +779,7 @@ void AutomationEditor::mouseMoveEvent(QMouseEvent * mouseEvent )
 						// When we clicked the node, we might have clicked slightly off
 						// so we account for that offset for a smooth drag
 						x -= m_moveXOffset;
-						posTicks = (x * MidiTime::ticksPerBar()/m_ppb) + m_currentPosition;
+						posTicks = (x * MidiTime::ticksPerBar() / m_ppb) + m_currentPosition;
 
 						// If we moved the mouse past the beginning correct the position in ticks
 						posTicks = qMax(posTicks, 0);