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

Allow relative paths on non-existent directories #4271

Merged
merged 5 commits into from Mar 27, 2018
Merged

Conversation

@tresf
Copy link
Member

@tresf tresf commented Mar 22, 2018

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

canonicalFilePath has several limitations including:

  1. Failure to handle a non-existant path (#4267)
  2. Possible undesired symlink handling
@tresf tresf requested a review from PhysSong Mar 22, 2018
@Wallacoloo Wallacoloo self-requested a review Mar 23, 2018
@@ -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(), '/' ) );

This comment has been minimized.

@PhysSong

PhysSong Mar 23, 2018
Member

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

This comment has been minimized.

@tresf

tresf Mar 23, 2018
Author Member

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?

This comment has been minimized.

This comment has been minimized.

@tresf

tresf Mar 23, 2018
Author Member

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.

This comment has been minimized.

@tresf

tresf Mar 23, 2018
Author Member

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. :)

This comment has been minimized.

@tresf

tresf Mar 23, 2018
Author Member

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. :)

This comment has been minimized.

@Wallacoloo

Wallacoloo Mar 23, 2018
Member

@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?

This comment has been minimized.

@PhysSong

PhysSong Mar 23, 2018
Member

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

This comment has been minimized.

@tresf

tresf Mar 23, 2018
Author Member

dir_separator, got it! Missed that one. Thanks!

So we can safely remove that.

Agreed. Will do.

@tresf
Copy link
Member Author

@tresf tresf commented Mar 23, 2018

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
Copy link
Member Author

@tresf tresf commented Mar 23, 2018

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
Copy link
Member

@PhysSong PhysSong commented Mar 25, 2018

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

@Wallacoloo
Copy link
Member

@Wallacoloo Wallacoloo commented Mar 25, 2018

@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
Copy link
Member Author

@tresf tresf commented Mar 25, 2018

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

@PhysSong
Copy link
Member

@PhysSong PhysSong commented Mar 26, 2018

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
Copy link
Member Author

@tresf tresf commented Mar 26, 2018

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

@@ -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 ) + "/";

This comment has been minimized.

@tresf

tresf Mar 27, 2018
Author Member

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

This comment has been minimized.

@PhysSong

PhysSong Mar 27, 2018
Member

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
Copy link
Member Author

@tresf tresf commented Mar 27, 2018

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

@tresf tresf merged commit 3673e84 into LMMS:stable-1.2 Mar 27, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tresf tresf deleted the tresf:relative branch Mar 27, 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.

None yet

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