Merge pull request #1762 from theGreatWhiteShark/phil-fix-1759

Fix legacy InstrumentComponent load (#1759)

src/core/Basics/InstrumentComponent.cpp

@@ -186,6 +186,18 @@ std::vector<std::shared_ptr<InstrumentLayer>>::iterator InstrumentComponent::end
 	return __layers.end();
 }
 
+const std::vector<std::shared_ptr<InstrumentLayer>> InstrumentComponent::get_layers() const {
+	std::vector<std::shared_ptr<InstrumentLayer>> layersUsed;
+
+	for ( const auto& layer : __layers ) {
+		if ( layer != nullptr ) {
+			layersUsed.push_back( layer );
+		}
+	}
+
+	return std::move( layersUsed );
+}
+
 };
 
 /* vim: set softtabstop=4 noexpandtab: */

src/core/Basics/InstrumentComponent.h

@@ -56,6 +56,15 @@ class InstrumentComponent : public H2Core::Object<InstrumentComponent>
 
 		std::shared_ptr<InstrumentLayer>	operator[]( int ix );
 		std::shared_ptr<InstrumentLayer>	get_layer( int idx );
+	/**
+	 * Get all initialized layers.
+	 *
+	 * In it's current design #__layers is always of #MAX_LAYERS
+	 * length and all layer not used are set to nullptr. This
+	 * convenience function is used to query only those
+	 * #InstrumentLayer which were properly initialized.
+	 */
+	const std::vector<std::shared_ptr<InstrumentLayer>> get_layers() const;
 		void				set_layer( std::shared_ptr<InstrumentLayer> layer, int idx );
 
 		void				set_drumkit_componentID( int related_drumkit_componentID );
@@ -136,7 +145,6 @@ inline std::shared_ptr<InstrumentLayer> InstrumentComponent::get_layer( int idx
 	assert( idx >= 0 && idx < m_nMaxLayers );
 	return __layers[ idx ];
 }
-
 };
 
 

src/core/CoreActionController.cpp

@@ -1259,10 +1259,10 @@ std::shared_ptr<Drumkit> CoreActionController::retrieveDrumkit( const QString& s
 
 	std::shared_ptr<Drumkit> pDrumkit = nullptr;
 
-	// We do not attempt to retrieve the drumkit from disk since this
-	// function is intended to be used for validating or upgrading
-	// drumkits via CLI or OSC command. It should always refer to the
-	// latest copy found on disk.
+	// We do not attempt to retrieve the drumkit from SoundLibrary
+	// since this function is intended to be used for validating or
+	// upgrading drumkits via CLI or OSC command. It should always
+	// refer to the latest copy found on disk.
 
 	*bIsCompressed = false;
 	*sTemporaryFolder = "";

src/core/CoreActionController.h

@@ -396,6 +396,30 @@ class CoreActionController : public H2Core::Object<CoreActionController> {
 	 * values and informs the GUI.
 	 */
 	void updatePreferences();
+
+	/**
+	 * Loads the drumkit specified in @a sDrumkitPath.
+	 *
+	 * Methods from within Hydrogen should _never_ call this function
+	 * directly but, instead, use
+	 * #SoundLibrarydatabase::getDrumkit(). It is only exposed
+	 * publicly to be used within the unit tests.
+	 *
+	 * \param sDrumkitPath Can be either an absolute path to a folder
+	 * containing a drumkit file (drumkit.xml), an absolute path to a
+	 * drumkit file itself, or an absolute file to a compressed
+	 * drumkit (.h2drumkit).
+	 * \param bIsCompressed Stores whether the drumkit was provided as
+	 * a compressed .h2drumkit file
+	 * \param sDrumkitDir Stores the folder containing the drumkit
+	 * file. If a compressed drumkit was provided, this will point to
+	 * a temporary folder.
+	 * \param sTemporaryFolder Root path of a temporary folder
+	 * containing the extracted drumkit in case @a sDrumkitPath
+	 * pointed to a compressed .h2drumkit file.
+	 */
+	std::shared_ptr<Drumkit> retrieveDrumkit( const QString& sDrumkitPath, bool* bIsCompressed,
+											  QString* sDrumkitDir, QString* sTemporaryFolder );
 private:
 	bool sendMasterVolumeFeedback();
 	bool sendStripVolumeFeedback( int nStrip );
@@ -428,24 +452,6 @@ class CoreActionController : public H2Core::Object<CoreActionController> {
 		 */
 		bool setSong( std::shared_ptr<Song> pSong, bool bRelinking = true );
 
-	/**
-	 * Loads the drumkit specified in @a sDrumkitPath.
-	 *
-	 * \param sDrumkitPath Can be either an absolute path to a folder
-	 * containing a drumkit file (drumkit.xml), an absolute path to a
-	 * drumkit file itself, or an absolute file to a compressed
-	 * drumkit (.h2drumkit).
-	 * \param bIsCompressed Stores whether the drumkit was provided as
-	 * a compressed .h2drumkit file
-	 * \param sDrumkitDir Stores the folder containing the drumkit
-	 * file. If a compressed drumkit was provided, this will point to
-	 * a temporary folder.
-	 * \param sTemporaryFolder Root path of a temporary folder
-	 * containing the extracted drumkit in case @a sDrumkitPath
-	 * pointed to a compressed .h2drumkit file.
-	 */
-	std::shared_ptr<Drumkit> retrieveDrumkit( const QString& sDrumkitPath, bool* bIsCompressed,
-											  QString* sDrumkitDir, QString* sTemporaryFolder );
 	/**
 	 * Add @a sFilename to the list of recent songs in
 	 * Preferences::m_recentFiles.

src/core/Helpers/Legacy.cpp

@@ -53,18 +53,32 @@ std::shared_ptr<InstrumentComponent> Legacy::loadInstrumentComponent( XMLNode* p
 
 	if ( pNode->firstChildElement( "filename" ).isNull() ) {
 		// not that old but no component yet.
+		auto pCompo = std::make_shared<InstrumentComponent>( 0 );
+
 		XMLNode layerNode = pNode->firstChildElement( "layer" );
-		if ( layerNode.isNull() ) {
+		int nLayer = 0;
+		while ( ! layerNode.isNull() ) {
+			if ( nLayer >= InstrumentComponent::getMaxLayers() ) {
+				ERRORLOG( QString( "Layer #%1 >= m_nMaxLayers (%2). This as well as all further layers will be omitted." )
+						  .arg( nLayer )
+						  .arg( InstrumentComponent::getMaxLayers() ) );
+				break;
+			}
+
+			auto pLayer = InstrumentLayer::load_from( &layerNode, sDrumkitPath,
+													  drumkitLicense, bSilent );
+			if ( pLayer != nullptr ) {
+				pCompo->set_layer( pLayer, nLayer );
+				nLayer++;
+			}
+			layerNode = layerNode.nextSiblingElement( "layer" );
+		}
+
+		if ( nLayer == 0 ) {
 			ERRORLOG( "Unable to load instrument component. Neither 'filename', 'instrumentComponent', nor 'layer' node found. Aborting." );
 			return nullptr;
 		}
 
-		auto pCompo = std::make_shared<InstrumentComponent>( 0 );
-		pCompo->set_layer( InstrumentLayer::load_from( &layerNode,
-													   sDrumkitPath,
-													   drumkitLicense,
-													   bSilent ),
-						   0 );
 		return pCompo;
 	}
 	else {

src/tests/XmlTest.cpp

@@ -38,6 +38,7 @@
 
 #include <QDir>
 #include <QTemporaryDir>
+#include <QTime>
 
 #include <core/Helpers/Filesystem.h>
 #include <core/Helpers/Xml.h>
@@ -244,7 +245,10 @@ void XmlTest::testDrumkitUpgrade() {
 		CPPUNIT_ASSERT( ! pCoreActionController->validateDrumkit( sDrumkitPath, false ) );
 
 		// The number of files within the drumkit has to be constant.
-		QTemporaryDir contentOriginal( H2Core::Filesystem::tmp_dir() + "-XXXXXX" );
+		QTemporaryDir contentOriginal( H2Core::Filesystem::tmp_dir() +
+									   "testDrumkitUpgrade_orig-" +
+									   QTime::currentTime().toString( "hh-mm-ss-zzz" ) +
+									   "-XXXXXX" );
 		contentOriginal.setAutoRemove( false );
 		CPPUNIT_ASSERT( pCoreActionController->extractDrumkit( sDrumkitPath,
 															   contentOriginal.path() ) );
@@ -256,7 +260,10 @@ void XmlTest::testDrumkitUpgrade() {
 		// Upgrade the legacy kit and store the result in a temporary
 		// folder (they will be automatically removed by Qt as soon as
 		// the variable gets out of scope)
-		QTemporaryDir firstUpgrade( H2Core::Filesystem::tmp_dir() + "-XXXXXX" );
+		QTemporaryDir firstUpgrade( H2Core::Filesystem::tmp_dir() +
+									"testDrumkitUpgrade_firstUpgrade-" +
+									QTime::currentTime().toString( "hh-mm-ss-zzz" ) +
+									"-XXXXXX" );
 		firstUpgrade.setAutoRemove( false );
 		CPPUNIT_ASSERT( pCoreActionController->upgradeDrumkit( sDrumkitPath,
 															   firstUpgrade.path() ) );
@@ -269,8 +276,40 @@ void XmlTest::testDrumkitUpgrade() {
 							  upgradeFolder.entryList( QDir::AllEntries |
 													   QDir::NoDotAndDotDot )[ 0 ] );
 		CPPUNIT_ASSERT( pCoreActionController->validateDrumkit( sUpgradedKit, false ) );
+
+		// Check whether the drumkit call be loaded properly.
+		bool b;
+		QString s1, s2;
+		auto pDrumkit =
+			pCoreActionController->retrieveDrumkit( firstUpgrade.path() + "/" + ssFile,
+													&b, &s1, &s2 );
+		CPPUNIT_ASSERT( pDrumkit != nullptr );
+		if ( pDrumkit->get_name() == "Boss DR-110" ) {
+			// For our default kit we put in some prior knowledge to
+			// check whether the upgrade process produce the expected
+			// results.
+			auto pInstrumentList = pDrumkit->get_instruments();
+			CPPUNIT_ASSERT( pInstrumentList != nullptr );
+			CPPUNIT_ASSERT( pInstrumentList->size() == 6 );
+
+			auto pInstrument = pInstrumentList->get( 0 );
+			CPPUNIT_ASSERT( pInstrument != nullptr );
+
+			auto pComponents = pInstrument->get_components();
+			CPPUNIT_ASSERT( pComponents != nullptr );
+			CPPUNIT_ASSERT( pComponents->size() == 1 );
+
+			auto pComponent = pComponents->at( 0 );
+			CPPUNIT_ASSERT( pComponent != nullptr );
+
+			auto pLayers = pComponent->get_layers();
+			CPPUNIT_ASSERT( pLayers.size() == 2 );
+		}
 
-		QTemporaryDir contentUpgraded( H2Core::Filesystem::tmp_dir() + "-XXXXXX" );
+		QTemporaryDir contentUpgraded( H2Core::Filesystem::tmp_dir() +
+									"testDrumkitUpgrade_contentUpgraded-" +
+									QTime::currentTime().toString( "hh-mm-ss-zzz" ) +
+									"-XXXXXX" );
 		contentUpgraded.setAutoRemove( false );
 		CPPUNIT_ASSERT( pCoreActionController->extractDrumkit( sUpgradedKit,
 															   contentUpgraded.path() ) );
@@ -296,7 +335,10 @@ void XmlTest::testDrumkitUpgrade() {
 
 		// Now we upgrade the upgraded drumkit again and bit-compare
 		// the results.
-		QTemporaryDir secondUpgrade( H2Core::Filesystem::tmp_dir() + "-XXXXXX" );
+		QTemporaryDir secondUpgrade( H2Core::Filesystem::tmp_dir() +
+									"testDrumkitUpgrade_secondUpgrade-" +
+									QTime::currentTime().toString( "hh-mm-ss-zzz" ) +
+									 "-XXXXXX" );
 		secondUpgrade.setAutoRemove( false );
 		CPPUNIT_ASSERT( pCoreActionController->upgradeDrumkit( sUpgradedKit,
 															   secondUpgrade.path() ) );
@@ -308,7 +350,10 @@ void XmlTest::testDrumkitUpgrade() {
 								upgradeFolder.entryList( QDir::AllEntries |
 														 QDir::NoDotAndDotDot )[ 0 ] );
 
-		QTemporaryDir contentValidation( H2Core::Filesystem::tmp_dir() + "-XXXXXX" );
+		QTemporaryDir contentValidation( H2Core::Filesystem::tmp_dir() +
+										 "testDrumkitUpgrade_contentValidation-" +
+										 QTime::currentTime().toString( "hh-mm-ss-zzz" ) +
+										 "-XXXXXX" );
 		contentValidation.setAutoRemove( false );
 		CPPUNIT_ASSERT( pCoreActionController->extractDrumkit( sUpgradedKit,
 															   contentValidation.path() ) );