Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make factory samples relative #3510

Merged
merged 7 commits into from May 6, 2017
Merged

Make factory samples relative #3510

merged 7 commits into from May 6, 2017

Conversation

@tresf
Copy link
Member

@tresf tresf commented Apr 22, 2017

Factory samples were broken in #1719.

This patch attempts to fix the problem by patching tryToMakeRelative by looping through all valid data:/ search paths and making a sample relative if contained within <path>/samples/.

This DOES NOT have an upgrade routine to relativize existing samples (e.g. in broken RC2 projects). Newly dragged samples should be fixed. Clicking "browse" and reselecting samples will fix existing tracks.

Closes #3491

tresf added 3 commits Apr 22, 2017
@tresf
Copy link
Member Author

@tresf tresf commented May 1, 2017

@lukas-w do you want to review this one before merging? I think by now there are enough projects out there in the wild we'll need an upgrade routine as either part of this PR or as a subsequent PR.

@lukas-w lukas-w self-requested a review May 2, 2017
Copy link
Member

@lukas-w lukas-w left a comment

The way this is implemented makes the function dependent on the actual value of factorySamplesDir(). It's assumed that dataDir() is an infix of factorySamplesDir(). So while it's fine this way for now and fixes the issue, it could become a very hard to find error in case dataDir() or factorySamplesDir() changes.

Ideally, what we'd need is a general-purpose function to query all search paths or files of a given path such as data:/samples/ (something we could use in PluginFactory::discoverPlugins() as well).

As long as we don't have that supposedly ideal solution, I suggest adding a test for tryToMakeRelative so that we know if it breaks.

QString samplesSuffix = ConfigManager::inst()->factorySamplesDir().mid( ConfigManager::inst()->dataDir().length() );

// Iterate over all valid "data:/" searchPaths
foreach ( const QString& path, QDir::searchPaths( "data" ) )

This comment has been minimized.

@lukas-w

lukas-w May 2, 2017
Member

Minor, but foreach is a candidate for removal in Qt, quote:

Since Qt 5.7, the use of this macro is discouraged. It will be removed in a future version of Qt. Please use C++11 range-for, possibly with qAsConst(), as needed.

So this would be the preferred way:

for ( const QString& path : QDir::searchPaths( "data" ) )
@tresf
Copy link
Member Author

@tresf tresf commented May 3, 2017

@lukas-w thanks. All points addressed.

Unfortunately my unit test fails. Can you help me make it work?

tresf added 2 commits May 3, 2017
@tresf
Copy link
Member Author

@tresf tresf commented May 3, 2017

Can you help me make it work?

@lukas-w I'll clarify... the SampleBuffer::tryToMakeRelative(...) needs some paths that can be detected as relative and then compare. I've also written a small test case for SampleBuffer::tryToMakeAbsolute(...). Both are failing a unit test.

I'm not exactly sure how I should construct the path. For example, if I use /home/tresf/lmms/build/data/samples/drums/kick01.ogg it does not work. I believe running from the ~/lmms/build directory, it would use the versions inside the source tree, but I'm unsure how to write a unit test for that. Do I need to do some CMakeCache.txt trickery? Any ideas?

@tresf tresf force-pushed the tresf:stable-1.2 branch from 2077621 to cdb2eef May 4, 2017
@tresf
Copy link
Member Author

@tresf tresf commented May 4, 2017

@lukas-w unit tests fixed. Ready for review.

@lukas-w
Copy link
Member

@lukas-w lukas-w commented May 5, 2017

@tresf Thanks. Somehow the sourceDir test fails for me when I run it from the /tests subdirectory. I'll see if I can find out why, or if I can come up with a test that doesn't need sourceDir to be exposed.

@lukas-w
Copy link
Member

@lukas-w lukas-w commented May 5, 2017

@tresf I'd suggest the following changes:

diff --git a/include/ConfigManager.h b/include/ConfigManager.h
index 7d542d93c..c5ef45b34 100644
--- a/include/ConfigManager.h
+++ b/include/ConfigManager.h
@@ -74,11 +74,6 @@ public:
 		return m_workingDir;
 	}
 
-	const QString & sourceDir() const
-	{
-		return m_srcDir;
-	}
-
 	QString userProjectsDir() const
 	{
 		return workingDir() + PROJECTS_PATH;
@@ -258,7 +253,6 @@ private:
 
 	QString m_lmmsRcFile;
 	QString m_workingDir;
-	QString m_srcDir;
 	QString m_dataDir;
 	QString m_artworkDir;
 	QString m_vstDir;
diff --git a/src/core/ConfigManager.cpp b/src/core/ConfigManager.cpp
index 38ddbe892..7016acce8 100644
--- a/src/core/ConfigManager.cpp
+++ b/src/core/ConfigManager.cpp
@@ -63,12 +63,12 @@ ConfigManager::ConfigManager() :
 
 	// If we're in development (lmms is not installed) let's get the source and
 	// binary directories by reading the CMake Cache
-	QString appPath = qApp->applicationDirPath();
+	QDir appPath = qApp->applicationDirPath();
 	// If in tests, get parent directory
-	if(appPath.endsWith("/tests")) {
-		appPath = QFileInfo(appPath).dir().currentPath();
+	if(appPath.dirName() == "tests") {
+		appPath.cdUp();
 	}
-	QFile cmakeCache(appPath + "/CMakeCache.txt");
+	QFile cmakeCache(appPath.absoluteFilePath("CMakeCache.txt"));
 	if (cmakeCache.exists()) {
 		cmakeCache.open(QFile::ReadOnly);
 		QTextStream stream(&cmakeCache);
@@ -81,8 +81,8 @@ ConfigManager::ConfigManager() :
 			QString line = stream.readLine();
 
 			if (line.startsWith("lmms_SOURCE_DIR:")) {
-				m_srcDir = line.section('=', -1).trimmed();
-				QDir::addSearchPath("data", m_srcDir + "/data/");
+				QString srcDir = line.section('=', -1).trimmed();
+				QDir::addSearchPath("data", srcDir + "/data/");
 				done++;
 			}
 			if (line.startsWith("lmms_BINARY_DIR:")) {
diff --git a/tests/src/core/RelativePathsTest.cpp b/tests/src/core/RelativePathsTest.cpp
index 72e7ff967..9738f2127 100644
--- a/tests/src/core/RelativePathsTest.cpp
+++ b/tests/src/core/RelativePathsTest.cpp
@@ -36,9 +36,14 @@ private slots:
 	void RelativePathComparisonTests()
 	{
 		// Bail if we can't find the source directory
-		QVERIFY(ConfigManager::inst()->sourceDir() != nullptr);
-		QVERIFY(SampleBuffer::tryToMakeRelative(ConfigManager::inst()->sourceDir() + "/data/samples/drums/kick01.ogg") == "drums/kick01.ogg");
-		QVERIFY(SampleBuffer::tryToMakeAbsolute("drums/kick01.ogg") == ConfigManager::inst()->sourceDir() + "/data/samples/drums/kick01.ogg");
+		QFileInfo fi(ConfigManager::inst()->factorySamplesDir() + "/drums/kick01.ogg");
+		QVERIFY(fi.exists());
+
+		QString absPath = fi.absoluteFilePath();
+		QString relPath = "drums/kick01.ogg";
+
+		QCOMPARE(SampleBuffer::tryToMakeRelative(absPath), relPath);
+		QCOMPARE(SampleBuffer::tryToMakeAbsolute(relPath), absPath);
 	}
 } RelativePathTests;

This fixes the tests sub-dir problem I experienced, and implements the test in a way that removes the sourceDir dependency.

@tresf tresf force-pushed the tresf:stable-1.2 branch 3 times, most recently from 4ff86f8 to 055766c May 6, 2017
@tresf tresf force-pushed the tresf:stable-1.2 branch from 055766c to 9564d81 May 6, 2017
@tresf
Copy link
Member Author

@tresf tresf commented May 6, 2017

@lukas-w thanks. Done via 4ff86f8.

@lukas-w
Copy link
Member

@lukas-w lukas-w commented May 6, 2017

👍

@lukas-w lukas-w merged commit 7e3ee14 into LMMS:stable-1.2 May 6, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
PhysSong added a commit to PhysSong/lmms that referenced this pull request Jul 8, 2017
* Make factory samples relative
Fixes LMMS#3491
Related LMMS#1719
@tresf tresf mentioned this pull request Feb 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.