Allow relative paths on non-existent directories #4271

tresf · 2018-03-22T17:28:04Z

Switch SampleBuffer::tryToMakeRelative(...) to use cleanPath instead of canonicalFilePath.

canonicalFilePath has several limitations including:

Failure to handle a non-existant path (Working directory gets reset when the path doesn't exist #4267)
Possible undesired symlink handling

PhysSong · 2018-03-23T05:34:49Z

src/core/SampleBuffer.cpp

@@ -1416,7 +1416,7 @@ QString SampleBuffer::tryToMakeRelative( const QString & file )
 	if( QFileInfo( file ).isRelative() == false )
 	{
 		// Normalize the path
-		QString f = QFileInfo( file ).canonicalFilePath().replace( QDir::separator(), '/' );
+		QString f( QDir::cleanPath( file ).replace( QDir::separator(), '/' ) );


Just a reminder, .replace( QDir::separator(), '/' ) can be removed when Qt4 support is completely dropped later.

Just a reminder, .replace( QDir::separator(), '/' ) can be removed when Qt4 support is completely dropped later.

Good to know... can you elaborate? Is the default behavior to use / in Qt5? If so where is this documented and is this across all Qt functions?

Documented in http://doc.qt.io/qt-5/qdir.html#cleanPath

Documented in http://doc.qt.io/qt-5/qdir.html#cleanPath

Yeah that doesn't have any "since qt5" warnings, so I just thought it was an example. Thanks for clarification.

For comparison:

4.8:

Removes all multiple directory separators "/" and resolves any "."s or ".."s found in the path, path.

5.x:

Returns path with directory separators normalized (converted to "/") and redundant ones removed, and "."s and ".."s resolved (as far as possible).

So I hope you understand my hesitation. :)

Hmm.... Here's the code for posterity...

static QString qt_cleanPath(const QString &path, bool *ok) -- { if (path.isEmpty()) return path; QString name = path; QChar dir_separator = QDir::separator(); if (dir_separator != QLatin1Char('/')) name.replace(dir_separator, QLatin1Char('/')); QString ret = qt_normalizePathSegments(name, OSSupportsUncPaths, ok); // Strip away last slash except for root directories if (ret.length() > 1 && ret.endsWith(QLatin1Char('/'))) { #if defined (Q_OS_WIN) # if defined(Q_OS_WINRT) if (!((ret.length() == 3 \|\| ret.length() == QDir::rootPath().length()) && ret.at(1) == QLatin1Char(':'))) # else if (!(ret.length() == 3 && ret.at(1) == QLatin1Char(':'))) # endif #endif ret.chop(1); } return ret; }

The only function that seems like it would touch \\ would be the qt_normalizePathSegments.cpp, but that specifically says in the source

// This method is shared with QUrl, so it doesn't deal with QDir::separator() ...

I guess I'll take your word for it. :)

@tresf it's about a third of the way down in that qt_cleanPath function:

name.replace(dir_separator, QLatin1Char('/'));

How do you feel about adding a short code comment explaining why we run .replace( QDir::separator(), '/' ) (maybe just linking to #4271 ) so that nobody else accidentally removes it, forgetting about Qt4 support?

Looking into Qt4's code, I found that even Qt 4.0 has the replacement logic. So we can safely remove that.

dir_separator, got it! Missed that one. Thanks!

So we can safely remove that.

Agreed. Will do.

tresf · 2018-03-23T05:39:42Z

I haven't actually tested any of this code myself (e.g for regressions to #4267). For starters, I was confused that QDir::cleanPath(...) actually returns a file path and not the parent folder. Fortunately the unit tests caught my mistake.

@PhysSong I'll let you merge this one since you're the bug submitter and the most likely to have tested this. :)

tresf · 2018-03-23T15:11:48Z

Seems like a lot of chatter over a string replacement, but this knowledge is only going to benefit the project for years to come. Thanks for the digging.

PhysSong · 2018-03-25T06:53:40Z

I found some more places where cleanPath can be used. Should I replace them in a separate PR?

Wallacoloo · 2018-03-25T19:49:08Z

@PhysSong Given this PR is from a tresf branch, I don't think you can easily add your own changes on top of this PR. I would say merge this one and then create a new PR for your changes.

tresf · 2018-03-25T19:57:48Z

Actually GitHub allows PRs to be modified now. It's a great feature especially for abandoned ones. we do it fairly often now. :)

PhysSong · 2018-03-26T06:51:07Z

What I'd like to do is using QDir::cleanPath to normalize data:/ search paths.

index 141085dd2..62bebff7c 100644
--- a/src/core/SampleBuffer.cpp
+++ b/src/core/SampleBuffer.cpp
@@ -1425,7 +1425,7 @@ QString SampleBuffer::tryToMakeRelative( const QString & file )
                // Iterate over all valid "data:/" searchPaths
                for ( const QString & path : QDir::searchPaths( "data" ) )
                {
-                       QString samplesPath = QString( path + samplesSuffix ).replace( QDir::separator(), '/' );
+                       QString samplesPath = QDir::cleanPath( path + samplesSuffix ) + "/";
                        if ( f.startsWith( samplesPath ) )
                        {
                                return QString( f ).mid( samplesPath.length() );

tresf · 2018-03-26T12:54:24Z

Not sure how well the data: portion is handled, but if it works, we can add it here.

tresf · 2018-03-27T00:34:48Z

src/core/SampleBuffer.cpp

@@ -1425,7 +1425,7 @@ QString SampleBuffer::tryToMakeRelative( const QString & file )
 		// Iterate over all valid "data:/" searchPaths
 		for ( const QString & path : QDir::searchPaths( "data" ) )
 		{
-			QString samplesPath = QString( path + samplesSuffix ).replace( QDir::separator(), '/' );
+			QString samplesPath = QDir::cleanPath( path + samplesSuffix ) + "/";


I believe ensureTrailingSlash may be better here. The previous logic doesn't add any slashes.

Maybe, but don't have to be. QDir::cleanPath always strips trailing slash and previous logic always gives a path with trailing slash.
BTW, ensureTrailingSlash is defined in ConfigManager.cpp. If you want to use that function, you should move that into an header first.

tresf · 2018-03-27T01:09:15Z

Didn't know about trailing slash strip. That's nice. Looks good then. Merge?

* Use cleanPath for calculating relative directories Closes LMMS#4267

tresf added 2 commits March 22, 2018 13:24

Allow relative paths on non-existent directories

68b9dac

Closes LMMS#4267

Remove unused var

99711de

tresf requested a review from PhysSong March 22, 2018 17:28

Remove redundant fileName

259bf0b

Wallacoloo self-requested a review March 23, 2018 01:41

Wallacoloo approved these changes Mar 23, 2018

View reviewed changes

PhysSong reviewed Mar 23, 2018

View reviewed changes

PhysSong approved these changes Mar 23, 2018

View reviewed changes

Remove redundant replace

3a87e57

Use normalized search paths in tryToMakeRelative

d2e94f6

tresf commented Mar 27, 2018

View reviewed changes

tresf merged commit 3673e84 into LMMS:stable-1.2 Mar 27, 2018

tresf deleted the relative branch March 27, 2018 01:51

sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022

Allow relative paths on non-existent directories (LMMS#4271)

fc4ddd5

* Use cleanPath for calculating relative directories Closes LMMS#4267

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow relative paths on non-existent directories #4271

Allow relative paths on non-existent directories #4271

tresf commented Mar 22, 2018 •

edited

Loading

PhysSong Mar 23, 2018

tresf Mar 23, 2018

PhysSong Mar 23, 2018

tresf Mar 23, 2018

tresf Mar 23, 2018 •

edited

Loading

tresf Mar 23, 2018

Wallacoloo Mar 23, 2018

PhysSong Mar 23, 2018

tresf Mar 23, 2018

tresf commented Mar 23, 2018

tresf commented Mar 23, 2018

PhysSong commented Mar 25, 2018

Wallacoloo commented Mar 25, 2018

tresf commented Mar 25, 2018

PhysSong commented Mar 26, 2018

tresf commented Mar 26, 2018

tresf Mar 27, 2018

PhysSong Mar 27, 2018

tresf commented Mar 27, 2018

Allow relative paths on non-existent directories #4271

Allow relative paths on non-existent directories #4271

Conversation

tresf commented Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tresf Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tresf commented Mar 23, 2018

tresf commented Mar 23, 2018

PhysSong commented Mar 25, 2018

Wallacoloo commented Mar 25, 2018

tresf commented Mar 25, 2018

PhysSong commented Mar 26, 2018

tresf commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tresf commented Mar 27, 2018

tresf commented Mar 22, 2018 •

edited

Loading

tresf Mar 23, 2018 •

edited

Loading