From cc07dc668fbc9682a6e1f56a117eee759481a544 Mon Sep 17 00:00:00 2001 From: Aaron Colwell <300262+acolwell@users.noreply.github.com> Date: Mon, 3 Jul 2023 11:45:29 -0700 Subject: [PATCH] Fix bugs and add support for UNC paths on Windows. - Added code to detect network share paths on Windows and treat make them behave like drives. - Fixed a bug where paths typed in with lower case drive letters would cause an upper case drive AND a lower case drive entry to appear in the "My Computer" view. A similar thing would happen for network share hostnames. Added logic to upper case the drive letter and hostnames as part of the "path cleanup" logic. This causes the paths to map to single entries in the "My Computer" view. - Fixed an infinite loop during directory restore when a non-existant network share was specified in a previous Open File dialog instance. - Fixed "up" button so that it behaves more consistently and properly handles network share paths. - Fixed invalid memory access for files that start with a '.' - Fixed a few places where code was using a URL path instead of converting the URL to a local path string like the vast majority of the code. This made it possible to properly view the root directory of network shares. - Updated unit tests to reflect changes in path handling behaviour. --- Engine/FileSystemModel.cpp | 175 ++++++++++++++++++++++++++------- Gui/SequenceFileDialog.cpp | 69 ++++++++----- Gui/SequenceFileDialog.h | 1 + Tests/FileSystemModel_Test.cpp | 44 +++++---- 4 files changed, 209 insertions(+), 80 deletions(-) diff --git a/Engine/FileSystemModel.cpp b/Engine/FileSystemModel.cpp index 2c1df6669..7442c6728 100644 --- a/Engine/FileSystemModel.cpp +++ b/Engine/FileSystemModel.cpp @@ -61,6 +61,93 @@ CLANG_DIAG_ON(uninitialized) NATRON_NAMESPACE_ENTER +static int +getDriveNameSize(const QString& name) +{ +#ifdef __NATRON_WIN32__ + if (name.size() < 3) { + return 0; + } + + if (name.at(0).isLetter() && name.at(1) == QLatin1Char(':') && ( name.at(2) == QLatin1Char('/') || name.at(2) == QLatin1Char('\\') ) ) { + // Drive path. (e.g. C:/ or C:\) + return 3; + } + + if (((name.at(0) == QLatin1Char('/') && name.at(1) == QLatin1Char('/')) || + (name.at(0) == QLatin1Char('\\') && name.at(1) == QLatin1Char('\\'))) && name.at(2).isLetterOrNumber()) { + // Possible Network share. (e.g. //SomeHost/ShareName) + + // Search for end of hostname. + int idx = name.indexOf(QLatin1Char('/'), 2); + if (idx == -1) { + idx = name.indexOf(QLatin1Char('\\'), 2); + } + + if (idx == -1) { + // Hostname only without a trailing slash. (e.g. //SomeHost). + // Do not consider this valid, just like "C:" is not considered a valid drive. + return 0; + } + + ++idx; // Move beyond slash. + return idx; + } +#else + if (name.startsWith(QDir::rootPath())) { + return QDir::rootPath().size(); + } +#endif + + return 0; +} + +#ifdef __NATRON_WIN32__ +// Ensures that drive names and network share hostnames are always upper case. +static QString +ensureCanonicalDriveAndShareNames(const QString& path) +{ + if (path.isEmpty() || !FileSystemModel::startsWithDriveName(path)) { + return path; + } + + if (path.size() < 3) { + return path; + } + + // Assume that any path that reaches this point has already had backslashes converted + // to forward slashes. + assert(path.indexOf(QLatin1Char('\\')) == -1); + + QString ret = path; + if (path[0] == QLatin1Char('/') && path[1] == QLatin1Char('/') && path[2].isLetterOrNumber()) { + // Network share name. + + int idx = path.indexOf(QLatin1Char('/'), 2); + ret.truncate(2); // keep starting slashes. + if (idx == -1) { + // No trailing slash. Hostname only case. + // Change the hostname to upper case and add the trailing slash. + ret.append(path.mid(2).toUpper()); + ret.append(QLatin1Char('/')); // Add a trailing slash. + } else { + // Hostname with slash and possible path components. + const QString hostname = path.mid(2, idx - 2); + + // Change hostname to upper case and append any path components that may remain. + ret.append(hostname.toUpper()); + ret.append(path.mid(idx)); + } + } else if (path[0].isLetter() && path[1] == QLatin1Char(':') && path[2] == QLatin1Char('/')) { + // Drive name. + // Force drive letter to be upper case. + ret[0] = ret[0].toUpper(); + } + + return ret; +} +#endif // __NATRON_WIN32__ + QStringList FileSystemModel::getSplitPath(const QString& path) { @@ -68,36 +155,24 @@ FileSystemModel::getSplitPath(const QString& path) return QStringList(); } QString pathCpy = path; - bool isdriveOrRoot; -#ifdef __NATRON_WIN32__ - QString startPath = pathCpy.mid(0, 3); - isdriveOrRoot = FileSystemModel::isDriveName(startPath); - if (isdriveOrRoot) { - pathCpy = pathCpy.remove(0, 3); - } - if ( (pathCpy.size() > 0) && ( pathCpy[pathCpy.size() - 1] == QChar::fromLatin1('/') ) ) { - pathCpy = pathCpy.mid(0, pathCpy.size() - 1); - } - QStringList splitPath = pathCpy.split( QChar::fromLatin1('/') ); - if (isdriveOrRoot) { - splitPath.prepend( startPath.mid(0, 3) ); - } -#else - isdriveOrRoot = pathCpy.startsWith( QChar::fromLatin1('/') ); - if (isdriveOrRoot) { - pathCpy = pathCpy.remove(0, 1); + int driveNameSize = getDriveNameSize(path); + if (driveNameSize > 0) { + // Remove the drive name from pathCpy. + pathCpy = pathCpy.remove(0, driveNameSize); } + + // Remove trailing slash. if ( (pathCpy.size() > 0) && ( pathCpy[pathCpy.size() - 1] == QChar::fromLatin1('/') ) ) { - pathCpy = pathCpy.mid(0, pathCpy.size() - 1); + pathCpy = pathCpy.mid(0, pathCpy.size() - 1); } + QStringList splitPath = pathCpy.split( QChar::fromLatin1('/') ); - if (isdriveOrRoot) { - splitPath.prepend( QChar::fromLatin1('/') ); + if (driveNameSize > 0) { + // Put the drive name at the beginning of the list. + splitPath.prepend(path.mid(0, driveNameSize)); } -#endif - return splitPath; } @@ -613,29 +688,57 @@ FileSystemModel::getSharedItemPtr(FileSystemItem* item) const bool FileSystemModel::isDriveName(const QString& name) { -#ifdef __NATRON_WIN32__ - - return name.size() == 3 && name.at(0).isLetter() && name.at(1) == QLatin1Char(':') && ( name.at(2) == QLatin1Char('/') || name.at(2) == QLatin1Char('\\') ); -#else - - return name == QDir::rootPath(); -#endif + return !name.isEmpty() && getDriveNameSize(name) == name.size(); } bool FileSystemModel::startsWithDriveName(const QString& name) { + return getDriveNameSize(name) > 0; +} + +QString FileSystemModel::cleanPath(const QString& path) { + if (path.isEmpty()) { + return path; + } + + QString newPath = path; + #ifdef __NATRON_WIN32__ + // Replace backslashes with slashes. + newPath.replace( QLatin1Char('\\'), QLatin1Char('/') ); +#endif - return name.size() >= 3 && name.at(0).isLetter() && name.at(1) == QLatin1Char(':') && ( name.at(2) == QLatin1Char('/') || name.at(2) == QLatin1Char('\\') ); -#else + bool startedWithDriveName = startsWithDriveName(newPath); - return name.startsWith( QDir::rootPath() ); + // Resolve and remove '.' and "..". + newPath = QDir::cleanPath(newPath); + + if (newPath.isEmpty()) { + return newPath; + } + + // QDir::cleanPath() strips the trailing slash for network shares + // (e.g. //SomeHost/) , but not for drives (e.g. C:/). The trailing + // slash is required for network shares to behave the same way as + // drives elsewhere in the code so we need to put back the trailing + // slash. + if (startedWithDriveName && !startsWithDriveName(newPath)) { + // The path no longer starts with a drive name after cleaning. + // See if appending a trailing slash fixes it. + const QString newPathWithTrailingSlash = newPath + QLatin1Char('/'); + if (startsWithDriveName(newPathWithTrailingSlash)) { + newPath = newPathWithTrailingSlash; + } + } + +#ifdef __NATRON_WIN32__ + // Make sure drive letters and share hostnames are upper case so they get properly + // collapsed together in the UI and other path matching. + newPath = ensureCanonicalDriveAndShareNames(newPath); #endif -} -QString FileSystemModel::cleanPath(const QString& path) { - return QDir::cleanPath(path); + return newPath; } QVariant diff --git a/Gui/SequenceFileDialog.cpp b/Gui/SequenceFileDialog.cpp index 9f1ea3dcb..e55b165dc 100644 --- a/Gui/SequenceFileDialog.cpp +++ b/Gui/SequenceFileDialog.cpp @@ -540,10 +540,12 @@ SequenceFileDialog::SequenceFileDialog( QWidget* parent, // necessary to transmi } if (!hasRestoredDir) { - // try to go up until we find a directory that exists. + // try to go up until we find a directory that exists or we have reached a drive name. + // Stopping at the drive name is important because cleanPath() does not guarantee that + // applying a "/.." to all drive names will return an empty string. (e.g. network shares //somehost/) QString dir = QString::fromUtf8( directoryArgs.c_str() ); //qDebug() << "dir=" << dir; - while (!dir.isEmpty() && !hasRestoredDir) { + while (!dir.isEmpty() && !FileSystemModel::isDriveName(dir) && !hasRestoredDir) { dir = FileSystemModel::cleanPath(dir + QString::fromUtf8("/..")); if ( isDirectory(dir) ) { hasRestoredDir = setDirectory(dir); @@ -737,7 +739,10 @@ SequenceFileDialog::setFileExtensionOnLineEdit(const QString & ext) return; } else { int pos = str.lastIndexOf( QLatin1Char('.') ); - if (pos != -1) { + if (pos == 0) { + // Do not modify filenames that start with a period. + return; + } else if (pos != -1) { if ( str.at(pos - 1) == QLatin1Char('#') ) { --pos; } @@ -1079,7 +1084,7 @@ SequenceDialogView::dropEvent(QDropEvent* e) QList urls = e->mimeData()->urls(); QString path; if (urls.size() > 0) { - path = QtCompat::toLocalFileUrlFixed( urls.at(0) ).path(); + path = urlToPathString( urls.at(0) ); } if ( !path.isEmpty() ) { _fd->setDirectory(path); @@ -1282,26 +1287,19 @@ SequenceFileDialog::parentFolder() QDir dir(rootPath); dir.cdUp(); newDir = dir.absolutePath(); - } - -#ifdef __NATRON_WIN32__ - if ( FileSystemModel::isDriveName(newDir) ) { - _upButton->setEnabled(false); - } else { - _upButton->setEnabled(true); - } -#else - if ( FileSystemModel::isDriveName(newDir) || newDir.isEmpty() ) { - _upButton->setEnabled(false); + if (!FileSystemModel::startsWithDriveName(newDir)) { + // QDir always provides a trailing slash to drives, but + // leaves it off for network shares. If the new + // directory does not start with a drive name anymore + // assume it just needs a trailing slash appended. + StrUtils::ensureLastPathSeparator(newDir); + } - return; - } else { - _upButton->setEnabled(true); + assert(FileSystemModel::startsWithDriveName(newDir)); } -#endif - + updateUpButton(newDir); setDirectory(newDir); } @@ -1617,11 +1615,7 @@ SequenceFileDialog::getEnvironmentVariable(const QString &string) void SequenceFileDialog::pathChanged(const QString &newPath) { - if ( newPath.isEmpty() ) { - _upButton->setEnabled(false); - } else { - _upButton->setEnabled(true); - } + updateUpButton(newPath); _favoriteView->selectUrl( QUrl::fromLocalFile(newPath) ); setHistory( _lookInCombobox->history() ); @@ -1638,6 +1632,27 @@ SequenceFileDialog::pathChanged(const QString &newPath) _previousButton->setEnabled(_currentHistoryLocation > 0); } +void +SequenceFileDialog::updateUpButton(const QString &newPath) { + if (newPath.isEmpty()) { + _upButton->setEnabled(false); + return; + } + +#ifndef __NATRON_WIN32__ + // On non-Windows platforms the up button should be disabled for the rootPath (i.e. "/"). + // On Windows we don't want to stop here because the rootPath is typically "C:/" and + // we want to be able to "go up one more" so that the drives and network shares can + // be seen. + if (newPath == QDir::rootPath()) { + _upButton->setEnabled(false); + return; + } +#endif + + _upButton->setEnabled(true); +} + void SequenceFileDialog::setHistory(const QStringList &paths) { @@ -1910,7 +1925,7 @@ UrlModel::setUrl(const QModelIndex &index, const QModelIndex &dirIndex) { setData(index, url, UrlRole); - if ( url.path().isEmpty() ) { + if ( urlToPathString(url).isEmpty() ) { const char* myComputer = "Computer"; setData(index, tr(myComputer)); setData(index, tr(myComputer), Qt::DecorationRole); @@ -2130,7 +2145,7 @@ UrlModel::changed(const QString &path) void UrlModel::removeRowIndex(const QModelIndex& index) { - QString urlPath = index.data(UrlRole).toUrl().path(); + QString urlPath = urlToPathString(index.data(UrlRole).toUrl()); if ( !urlPath.isEmpty() ) { removeRow( index.row() ); diff --git a/Gui/SequenceFileDialog.h b/Gui/SequenceFileDialog.h index de936dbeb..eecdc6437 100644 --- a/Gui/SequenceFileDialog.h +++ b/Gui/SequenceFileDialog.h @@ -543,6 +543,7 @@ public Q_SLOTS: void getSequenceFromFilesForFole(const QString & file, SequenceParsing::SequenceFromFiles* sequence) const; void updateFileExtensionCombo(const QString& extension); + void updateUpButton(const QString &newPath); private: // FIXME: PIMPL diff --git a/Tests/FileSystemModel_Test.cpp b/Tests/FileSystemModel_Test.cpp index 3ff940c6d..9ea83ab40 100644 --- a/Tests/FileSystemModel_Test.cpp +++ b/Tests/FileSystemModel_Test.cpp @@ -81,9 +81,9 @@ TEST(FileSystemModelTest, DriveName) { {"c:\\somedir\\filename.txt", false, true, false, false}, {"//", false, false, false, true}, {"//somehost", false, false, false, true}, - {"//somehost/", false, false, false, true}, - {"//somehost/somedir/", false, false, false, true}, - {"//somehost/somedir/filename.txt", false, false, false, true}, + {"//somehost/", true, true, false, true}, + {"//somehost/somedir/", false, true, false, true}, + {"//somehost/somedir/filename.txt", false, true, false, true}, }); for (const auto& testCase : testCases) { @@ -146,12 +146,12 @@ TEST(FileSystemModelTest, GetSplitPath) { {"", "", "somehost"}, // Not considered a network path because // of missing slash after hostname. {"/", "", "somehost"}}, - {"//somehost/", {"", "", "somehost"}, {"/", "", "somehost"}}, + {"//somehost/", {"//somehost/", ""}, {"/", "", "somehost"}}, {"//somehost/somedir/", - {"", "", "somehost", "somedir"}, + {"//somehost/", "somedir"}, {"/", "", "somehost", "somedir"}}, {"//somehost/somedir/filename.txt", - {"", "", "somehost", "somedir", "filename.txt"}, + {"//somehost/", "somedir", "filename.txt"}, {"/", "", "somehost", "somedir", "filename.txt"}}, }); @@ -202,21 +202,31 @@ TEST(FileSystemModelTest, CleanPath) { {"/somedir/filename.txt", nullptr, nullptr}, {"c:", nullptr, nullptr}, // Platform specific path behaviors. - {"c:/", "c:/", "c:"}, - {"c:\\", "c:/", "c:\\"}, - {"c:/filename.txt", "c:/filename.txt", "c:/filename.txt"}, - {"c:\\filename.txt", "c:/filename.txt", "c:\\filename.txt"}, - {"c:/somedir/", "c:/somedir", nullptr}, - {"c:/somedir/filename.txt", "c:/somedir/filename.txt", nullptr}, - {"c:\\somedir\\", "c:/somedir", "c:\\somedir\\"}, - {"c:\\somedir\\filename.txt", "c:/somedir/filename.txt", + // Paths with Windows drive letters and backslashes. + // - Verify drive letters become capitalized on Windows + // - Verify drives keep their trailing slash. + // - Verify backslashes are converted to forward slashes on Windows. + // - Verify Unix platforms do not treat forward slashes and drive + // letters in special way. + {"c:/", "C:/", "c:"}, + {"c:\\", "C:/", "c:\\"}, + {"c:/filename.txt", "C:/filename.txt", "c:/filename.txt"}, + {"c:\\filename.txt", "C:/filename.txt", "c:\\filename.txt"}, + {"c:/somedir/", "C:/somedir", "c:/somedir"}, + {"c:/somedir/filename.txt", "C:/somedir/filename.txt", "c:/somedir/filename.txt"}, + {"c:\\somedir\\", "C:/somedir", "c:\\somedir\\"}, + {"c:\\somedir\\filename.txt", "C:/somedir/filename.txt", "c:\\somedir\\filename.txt"}, + // Windows Network Shares + // - Verify that hostname is capitalized on Windows as long as it + // is followed by a /. + // - Verify Unix just collapses the '//' to a single slash. {"//", "/", nullptr}, {"//somehost", "//somehost", "/somehost"}, - {"//somehost/", "//somehost", "/somehost"}, - {"//somehost/somedir/", "//somehost/somedir", "/somehost/somedir"}, + {"//somehost/", "//SOMEHOST/", "/somehost"}, + {"//somehost/somedir/", "//SOMEHOST/somedir", "/somehost/somedir"}, {"//somehost/somedir/filename.txt", - "//somehost/somedir/filename.txt", + "//SOMEHOST/somedir/filename.txt", "/somehost/somedir/filename.txt"}, });