From 351b5b3e9410e5957c4cb74fcaf24a3aff1dbc8f Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Mon, 28 Oct 2024 18:06:48 +0100 Subject: [PATCH] QFileSystemModel on Windows: consistently trim spaces and dots On Windows, file- and directory-names that end with a space (or a dot) are invalid, even though it's possible to create them. Same for names that start with a space (but starting with dots is ok). Even Explorer chokes when trying to then delete or rename such a file or directory, and often the only way out of the mess is to operate on the 8.3 path. In Qt, we only sometimes fixed such paths by removing trailing spaces and dots. This still made it possible to create a "bad " directory in a non-native file dialog, which could then not be deleted. The node tree even ended up with both "bad " and "bad", neither of which was handled correctly. Fix this by always chopping trailing space and dots when adding or modifying a node, amending 3693d3d2a1e76f6e6d41db340505e00c6ddaeda1. Fortunately, this can be reproduced with QFileSystemModel, so add a test that exercises it in the same way as QFileDialog does, e.g. both by creating the misnamed directory directly, and by renaming a directory to an invalid name. Given that this is very broken on Windows Explorer as well, don't cherry-pick this back into branches under strict change management. Fixes: QTBUG-129028 Change-Id: Iec1edf9fd2548dda4567134a9b5cf3e298589c19 Reviewed-by: Wladimir Leuschner Reviewed-by: Oliver Wolff (cherry picked from commit 53df22b42d73dbb48bc25ad05500993df56b928c) Reviewed-by: Qt Cherry-pick Bot --- src/gui/itemmodels/qfilesystemmodel.cpp | 57 ++++++++++---- .../qfilesystemmodel/tst_qfilesystemmodel.cpp | 75 +++++++++++++++++++ 2 files changed, 119 insertions(+), 13 deletions(-) diff --git a/src/gui/itemmodels/qfilesystemmodel.cpp b/src/gui/itemmodels/qfilesystemmodel.cpp index 290891322f0..92feabdfebd 100644 --- a/src/gui/itemmodels/qfilesystemmodel.cpp +++ b/src/gui/itemmodels/qfilesystemmodel.cpp @@ -310,6 +310,24 @@ static QString qt_GetLongPathName(const QString &strShortPath) return QDir::fromNativeSeparators(strShortPath); } } + +static inline void chopSpaceAndDot(QString &element) +{ + if (element == "."_L1 || element == ".."_L1) + return; + // On Windows, "filename " and "filename" are equivalent and + // "filename . " and "filename" are equivalent + // "filename......." and "filename" are equivalent Task #133928 + // whereas "filename .txt" is still "filename .txt" + while (element.endsWith(u'.') || element.endsWith(u' ')) + element.chop(1); + + // If a file is saved as ' Foo.txt', where the leading character(s) + // is an ASCII Space (0x20), it will be saved to the file system as 'Foo.txt'. + while (element.startsWith(u' ')) + element.remove(0, 1); +} + #endif /*! @@ -403,15 +421,10 @@ QFileSystemModelPrivate::QFileSystemNode *QFileSystemModelPrivate::node(const QS if (i == pathElements.size() - 1) elementPath.append(trailingSeparator); #ifdef Q_OS_WIN - // On Windows, "filename " and "filename" are equivalent and - // "filename . " and "filename" are equivalent - // "filename......." and "filename" are equivalent Task #133928 - // whereas "filename .txt" is still "filename .txt" // If after stripping the characters there is nothing left then we // just return the parent directory as it is assumed that the path - // is referring to the parent - while (element.endsWith(u'.') || element.endsWith(u' ')) - element.chop(1); + // is referring to the parent. + chopSpaceAndDot(element); // Only filenames that can't possibly exist will be end up being empty if (element.isEmpty()) return parent; @@ -866,6 +879,12 @@ bool QFileSystemModel::setData(const QModelIndex &idx, const QVariant &value, in } QString newName = value.toString(); +#ifdef Q_OS_WIN + chopSpaceAndDot(newName); + if (newName.isEmpty()) + return false; +#endif + QString oldName = idx.data().toString(); if (newName == oldName) return true; @@ -1428,17 +1447,24 @@ QModelIndex QFileSystemModel::mkdir(const QModelIndex &parent, const QString &na if (!parent.isValid()) return parent; + QString fileName = name; +#ifdef Q_OS_WIN + chopSpaceAndDot(fileName); + if (fileName.isEmpty()) + return QModelIndex(); +#endif + QDir dir(filePath(parent)); - if (!dir.mkdir(name)) + if (!dir.mkdir(fileName)) return QModelIndex(); QFileSystemModelPrivate::QFileSystemNode *parentNode = d->node(parent); - d->addNode(parentNode, name, QFileInfo()); - Q_ASSERT(parentNode->children.contains(name)); - QFileSystemModelPrivate::QFileSystemNode *node = parentNode->children[name]; + d->addNode(parentNode, fileName, QFileInfo()); + Q_ASSERT(parentNode->children.contains(fileName)); + QFileSystemModelPrivate::QFileSystemNode *node = parentNode->children[fileName]; #if QT_CONFIG(filesystemwatcher) - node->populate(d->fileInfoGatherer->getInfo(QFileInfo(dir.absolutePath() + QDir::separator() + name))); + node->populate(d->fileInfoGatherer->getInfo(QFileInfo(dir.absolutePath() + QDir::separator() + fileName))); #endif - d->addVisibleFiles(parentNode, QStringList(name)); + d->addVisibleFiles(parentNode, QStringList(fileName)); return d->index(node); } @@ -1932,6 +1958,11 @@ void QFileSystemModelPrivate::fileSystemChanged(const QString &path, QExtendedInformation info = fileInfoGatherer->getInfo(update.second); bool previouslyHere = parentNode->children.contains(fileName); if (!previouslyHere) { +#ifdef Q_OS_WIN + chopSpaceAndDot(fileName); + if (fileName.isEmpty()) + continue; +#endif addNode(parentNode, fileName, info.fileInfo()); } QFileSystemModelPrivate::QFileSystemNode * node = parentNode->children.value(fileName); diff --git a/tests/auto/gui/itemmodels/qfilesystemmodel/tst_qfilesystemmodel.cpp b/tests/auto/gui/itemmodels/qfilesystemmodel/tst_qfilesystemmodel.cpp index f75ff57b010..c26e4ecab2d 100644 --- a/tests/auto/gui/itemmodels/qfilesystemmodel/tst_qfilesystemmodel.cpp +++ b/tests/auto/gui/itemmodels/qfilesystemmodel/tst_qfilesystemmodel.cpp @@ -114,6 +114,9 @@ private slots: void fileInfo(); + void pathWithTrailingSpace_data(); + void pathWithTrailingSpace(); + protected: bool createFiles(QFileSystemModel *model, const QString &test_path, const QStringList &initial_files, int existingFileCount = 0, @@ -1296,6 +1299,78 @@ void tst_QFileSystemModel::fileInfo() QCOMPARE(model.fileInfo(idx), QFileInfo(dirPath)); } +void tst_QFileSystemModel::pathWithTrailingSpace_data() +{ + QTest::addColumn("input"); + QTest::addColumn("foundAs"); + + static constexpr bool windows = +#ifdef Q_OS_WIN + true; +#else + false; +#endif + // Cover the various cases as documented at + // https://learn.microsoft.com/en-us/troubleshoot/windows-client/shell-experience/file-folder-name-whitespace-characters + + QTest::addRow("trailing space") << "space " + << (windows ? "space" : "space "); + QTest::addRow("leading space") << " leadingspace" + << (windows ? "leadingspace" : " leadingspace"); + QTest::addRow("trailing dot") << "dot." + << (windows ? "dot" : "dot."); + QTest::addRow("leading dot") << ".dot" << ".dot"; + + QTest::addRow("spacedot .") << "spacedot ." + << (windows ? "spacedot" : "spacedot ."); + QTest::addRow("dotspace. ") << "dotspace. " + << (windows ? "dotspace" : "dotspace. "); + QTest::addRow("invisible") << " " + << (windows ? "" : " "); + + QTest::addRow("leading 0x3000") << "\u3000-x3000" << "\u3000-x3000"; + QTest::addRow("trailing 0x3000") << "x3000-\u3000" << "x3000-\u3000"; +} + +void tst_QFileSystemModel::pathWithTrailingSpace() +{ + QFETCH(const QString, input); + QFETCH(const QString, foundAs); + + const bool validInput = !foundAs.isEmpty(); + + QTemporaryDir temp; + + QFileSystemModel model; + model.setReadOnly(false); + const QModelIndex rootIndex = model.setRootPath(temp.path()); + const QDir rootDir = model.rootDirectory(); + const QString absoluteInput = rootDir.absoluteFilePath(input); + const QString absoluteFoundAs = validInput ? rootDir.absoluteFilePath(foundAs) + : rootDir.absolutePath(); + + const QModelIndex createdIndex = model.mkdir(rootIndex, input); + QCOMPARE(createdIndex.isValid(), validInput); + if (createdIndex.isValid()) + QCOMPARE(model.filePath(createdIndex), absoluteFoundAs); + const QModelIndex foundIndex = model.index(absoluteInput); + QVERIFY(foundIndex.isValid()); + QCOMPARE(model.filePath(foundIndex), absoluteFoundAs); + + if (createdIndex.isValid()) + QVERIFY(model.rmdir(createdIndex)); + + const QPersistentModelIndex newFolder = model.mkdir(rootIndex, "New Folder"); + QVERIFY(newFolder.isValid()); + QVERIFY(model.flags(newFolder) & Qt::ItemIsEditable); + QCOMPARE(model.setData(newFolder, input, Qt::EditRole), validInput); + if (validInput) { + const QModelIndex renamedIndex = model.index(absoluteInput); + QVERIFY(renamedIndex.isValid()); + } +} + + QTEST_MAIN(tst_QFileSystemModel) #include "tst_qfilesystemmodel.moc"