From 81e47b089622d797505ee22303e7c19bc3d9bae4 Mon Sep 17 00:00:00 2001 From: Daniel Molkentin Date: Tue, 26 Nov 2013 02:02:56 +0100 Subject: [PATCH] Folder wizard: sanitize error detection * Wrap text properly * Format multiple warnings as bullet points * Use 'Folder' instead of 'Directory' everywhere * Fix false positives when checking if one directory contains another * Fix logic errors in target folder warning detection Fixes #1201 --- src/mirall/folderwizard.cpp | 102 +++++++++++++++++++-------- src/mirall/folderwizard.h | 9 ++- src/mirall/folderwizardsourcepage.ui | 3 - src/mirall/folderwizardtargetpage.ui | 3 - 4 files changed, 79 insertions(+), 38 deletions(-) diff --git a/src/mirall/folderwizard.cpp b/src/mirall/folderwizard.cpp index 17b3b0e47e..ccb7421981 100644 --- a/src/mirall/folderwizard.cpp +++ b/src/mirall/folderwizard.cpp @@ -36,8 +36,24 @@ namespace Mirall { +QString FormatWarningsWizardPage::formatWarnings(const QStringList &warnings) const +{ + QString ret; + if (warnings.count() == 1) { + ret = tr("Warning: ") + warnings.first(); + } else if (warnings.count() > 1) { + ret = tr("Warning: ") + ""; + } + + return ret; +} + FolderWizardSourcePage::FolderWizardSourcePage() - : QWizardPage() + : FormatWarningsWizardPage() { _ui.setupUi(this); registerField(QLatin1String("sourceFolder*"), _ui.localFolderLineEdit); @@ -45,7 +61,7 @@ FolderWizardSourcePage::FolderWizardSourcePage() _ui.localFolderLineEdit->setText( QDir::toNativeSeparators( defaultPath ) ); registerField(QLatin1String("alias*"), _ui.aliasLineEdit); _ui.aliasLineEdit->setText( Theme::instance()->appNameGUI() ); - + _ui.warnLabel->setTextFormat(Qt::RichText); _ui.warnLabel->hide(); } @@ -69,16 +85,16 @@ bool FolderWizardSourcePage::isComplete() const QFileInfo selFile( QDir::fromNativeSeparators(_ui.localFolderLineEdit->text()) ); QString userInput = selFile.canonicalFilePath(); - QString warnString; + QStringList warnStrings; bool isOk = selFile.isDir(); if( !isOk ) { - warnString = tr("No local folder selected!"); + warnStrings.append(tr("No valid local folder selected!")); } if (isOk && !selFile.isWritable()) { isOk = false; - warnString += tr("You have no permission to write to the selected folder!"); + warnStrings.append(tr("You have no permission to write to the selected folder!")); } // check if the local directory isn't used yet in another ownCloud sync @@ -99,21 +115,40 @@ bool FolderWizardSourcePage::isComplete() const if( ! folderDir.endsWith(QLatin1Char('/')) ) folderDir.append(QLatin1Char('/')); qDebug() << "Checking local path: " << folderDir << " <-> " << userInput; - if( QFileInfo( f->path() ) == userInput ) { + if( QDir::cleanPath(f->path()) == QDir::cleanPath(userInput) && + QDir::cleanPath(QDir(f->path()).canonicalPath()) == QDir(userInput).canonicalPath() ) { isOk = false; - warnString.append( tr("The local path %1 is already an upload folder.
Please pick another one!") + warnStrings.append( tr("The local path %1 is already an upload folder. Please pick another one!") .arg(QDir::toNativeSeparators(userInput)) ); } - if( isOk && folderDir.startsWith( userInput )) { + if( isOk && QDir::cleanPath(folderDir).startsWith(QDir::cleanPath(userInput)+'/') ) { qDebug() << "A already configured folder is child of the current selected"; - warnString.append( tr("An already configured folder is contained in the current entry.")); + warnStrings.append( tr("An already configured folder is contained in the current entry.")); isOk = false; } - if( isOk && userInput.startsWith( folderDir ) ) { + + QString absCleanUserFolder = QDir::cleanPath(QDir(userInput).canonicalPath())+'/'; + if( isOk && QDir::cleanPath(folderDir).startsWith(absCleanUserFolder) ) { + qDebug() << "A already configured folder is child of the current selected"; + warnStrings.append( tr("The selected folder is a symbolic link. An already configured" + "folder is contained in the folder this link is pointing to.")); + isOk = false; + } + + if( isOk && QDir::cleanPath(QString(userInput+'/')).startsWith( QDir::cleanPath(folderDir)) ) { qDebug() << "An already configured folder is parent of the current selected"; - warnString.append( tr("An already configured folder contains the currently entered directory.")); + warnStrings.append( tr("An already configured folder contains the currently entered folder.")); isOk = false; } + if( isOk && absCleanUserFolder.startsWith( QDir::cleanPath(folderDir)) ) { + qDebug() << "The selected folder is a symbolic link. An already configured folder is\n" + "the parent of the current selected contains the folder this link is pointing to."; + warnStrings.append( tr("The selected folder is a symbolic link. An already configured folder " + "is the parent of the current selected contains the folder this link is " + "pointing to.")); + isOk = false; + } + i++; } } @@ -121,7 +156,7 @@ bool FolderWizardSourcePage::isComplete() const // check if the alias is unique. QString alias = _ui.aliasLineEdit->text(); if( alias.isEmpty() ) { - warnString.append( tr("The alias can not be empty. Please provide a descriptive alias word.") ); + warnStrings.append( tr("The alias can not be empty. Please provide a descriptive alias word.") ); isOk = false; } @@ -132,7 +167,7 @@ bool FolderWizardSourcePage::isComplete() const qDebug() << "Checking local alias: " << f->alias(); if( f ) { if( f->alias() == alias ) { - warnString.append( tr("
The alias %1 is already in use. Please pick another alias.").arg(alias) ); + warnStrings.append( tr("The alias %1 is already in use. Please pick another alias.").arg(alias) ); isOk = false; goon = false; } @@ -140,12 +175,14 @@ bool FolderWizardSourcePage::isComplete() const i++; } + _ui.warnLabel->setWordWrap(true); if( isOk ) { _ui.warnLabel->hide(); _ui.warnLabel->setText( QString::null ); } else { _ui.warnLabel->show(); - _ui.warnLabel->setText( warnString ); + QString warnings = formatWarnings(warnStrings); + _ui.warnLabel->setText( warnings ); } return isOk; } @@ -168,7 +205,8 @@ void FolderWizardSourcePage::on_localFolderLineEdit_textChanged() // ================================================================================= FolderWizardTargetPage::FolderWizardTargetPage() -: _warnWasVisible(false) + : FormatWarningsWizardPage() + ,_warnWasVisible(false) { _ui.setupUi(this); _ui.warnFrame->hide(); @@ -223,7 +261,7 @@ void FolderWizardTargetPage::slotCreateRemoteFolderFinished(QNetworkReply::Netwo void FolderWizardTargetPage::slotHandleNetworkError(QNetworkReply *reply) { qDebug() << "** webdav mkdir request failed:" << reply->error(); - showWarn(tr("Failed to create the folder on %1.
Please check manually.") + showWarn(tr("Failed to create the folder on %1. Please check manually.") .arg(Theme::instance()->appNameGUI())); } @@ -311,7 +349,11 @@ bool FolderWizardTargetPage::isComplete() const if (!_ui.folderTreeWidget->currentItem()) return false; + QStringList warnStrings; QString dir = _ui.folderTreeWidget->currentItem()->data(0, Qt::UserRole).toString(); + if (!dir.startsWith(QLatin1Char('/'))) { + dir.prepend(QLatin1Char('/')); + } wizard()->setProperty("targetPath", dir); Folder::Map map = _folderMap; @@ -319,24 +361,24 @@ bool FolderWizardTargetPage::isComplete() const for(i = map.constBegin();i != map.constEnd(); i++ ) { Folder *f = static_cast(i.value()); QString curDir = f->remotePath(); - if (dir == curDir) { - showWarn( tr("This directory is already being synced.") ); - return false; + if (!curDir.startsWith(QLatin1Char('/'))) { + curDir.prepend(QLatin1Char('/')); + } + if (QDir::cleanPath(dir) == QDir::cleanPath(curDir)) { + warnStrings.append(tr("This folder is already being synced.")); } else if (dir.startsWith(curDir + QLatin1Char('/'))) { - if (dir.isEmpty()) dir = QLatin1Char('/'); - if (curDir.isEmpty()) curDir = QLatin1Char('/'); - showWarn( tr("You are already syncing %1, which is a parent folder of %2.").arg(curDir).arg(dir) ); - return false; + warnStrings.append(tr("You are already syncing %1, which is a parent folder of %2.").arg(curDir).arg(dir)); + } + + if (curDir == QLatin1String("/")) { + warnStrings.append(tr("You are already syncing all your files. Syncing another folder is not supported. " + "If you want to sync multiple folders, please remove the currently configured " + "root folder sync.")); } } - if( dir == QLatin1String("/") ) { - showWarn( tr("If you sync the root folder, you can not configure another sync directory.")); - return true; - } else { - showWarn(); - return true; - } + showWarn(formatWarnings(warnStrings)); + return warnStrings.isEmpty(); } void FolderWizardTargetPage::cleanupPage() diff --git a/src/mirall/folderwizard.h b/src/mirall/folderwizard.h index f2e453a9bf..dbefa496c6 100644 --- a/src/mirall/folderwizard.h +++ b/src/mirall/folderwizard.h @@ -28,10 +28,15 @@ namespace Mirall { class ownCloudInfo; +class FormatWarningsWizardPage : public QWizardPage { +protected: + QString formatWarnings(const QStringList &warnings) const; +}; + /** * page to ask for the local source folder */ -class FolderWizardSourcePage : public QWizardPage +class FolderWizardSourcePage : public FormatWarningsWizardPage { Q_OBJECT public: @@ -57,7 +62,7 @@ private: * page to ask for the target folder */ -class FolderWizardTargetPage : public QWizardPage +class FolderWizardTargetPage : public FormatWarningsWizardPage { Q_OBJECT public: diff --git a/src/mirall/folderwizardsourcepage.ui b/src/mirall/folderwizardsourcepage.ui index c583b815c9..5dc05c4fca 100644 --- a/src/mirall/folderwizardsourcepage.ui +++ b/src/mirall/folderwizardsourcepage.ui @@ -145,9 +145,6 @@ Qt::AutoText - - Qt::AlignCenter - 3 diff --git a/src/mirall/folderwizardtargetpage.ui b/src/mirall/folderwizardtargetpage.ui index 3c17f0ae44..462fde0f14 100644 --- a/src/mirall/folderwizardtargetpage.ui +++ b/src/mirall/folderwizardtargetpage.ui @@ -109,9 +109,6 @@ Qt::AutoText - - Qt::AlignCenter - true