feat(gui): improve CLI help output and argument validation

src/gui/application.cpp

@@ -1,3 +1,3 @@
 /*
  * SPDX-FileCopyrightText: 2017 Nextcloud GmbH and Nextcloud contributors
  * SPDX-FileCopyrightText: 2014 ownCloud GmbH
@@ -70,32 +70,56 @@
 
     static const char optionsC[] =
         "Options:\n"
+        "\n"
+        "General options:\n"
         "  --help, -h                 : show this help screen.\n"
-        "  --version, -v              : show version information.\n"
-        "  -q --quit                  : quit the running instance\n"
+        "  --version, -v              : show client version/build details.\n"
+        "  --quit, -q                 : quit the running instance.\n"
+        "  --background               : launch the client in the background.\n"
+        "  --reverse                  : use a reverse layout direction.\n"
+        "\n"
+        "Logging options:\n"
         "  --logwindow, -l            : open a window to show log output.\n"
-        "  --logfile <filename>       : write log output to file <filename>.\n"
-        "  --logdir <name>            : write each sync log output in a new file\n"
-        "                               in folder <name>.\n"
-        "  --logexpire <hours>        : removes logs older than <hours> hours.\n"
-        "                               (to be used with --logdir)\n"
-        "  --logflush                 : flush the log file after every write.\n"
+        "  --logfile <filename>       : write sync logs to the specified filename.\n"
+        "  --logdir <dir>             : write sync logs for each run to a new file\n"
+        "                               in the specified directory.\n"
+        "  --logexpire <hours>        : delete sync logs older than the specified\n"
+        "                               number of hours (to be used with --logdir).\n"
+        "                               Default: 24 hours.\n"
+        "  --logflush                 : flush the sync log file after every write.\n"
         "  --logdebug                 : also output debug-level messages in the log.\n"
-        "  --confdir <dirname>        : Use the given configuration folder.\n"
-        "  --background               : launch the application in the background.\n"
-        "  --overrideserverurl        : specify a server URL to use for the force override to be used in the account setup wizard.\n"
-        "  --overridelocaldir         : specify a local dir to be used in the account setup wizard.\n"
-        "  --userid                   : userId (username as on the server) to pass when creating an account via command-line.\n"
-        "  --apppassword              : appPassword to pass when creating an account via command-line.\n"
-        "  --set-language <lang>      : specify a language to use for the client, regardless of the OS language.\n"
-        "  --localdirpath             : (optional) path where to create a local sync folder when creating an account via command-line.\n"
-        "  --isvfsenabled             : whether to set a VFS or non-VFS folder (1 for 'yes' or 0 for 'no') when creating an account via command-line.\n"
-        "  --remotedirpath            : (optional) path to a remote subfolder when creating an account via command-line.\n"
-        "  --serverurl                : a server URL to use when creating an account via command-line.\n"
+        "\n"
+        "Configuration options:\n"
+        "  --confdir <dir>            : override the default client configuration\n"
+        "                               directory.\n"
+        "  --set-language <lang>      : use this language for the client, regardless\n"
+        "                               of the OS language.\n"
+        "  --debug                    : enable debug mode and debug-level logging.\n"
 #if !DISABLE_ACCOUNT_MIGRATION
-        "  --forcelegacyconfigimport  : forcefully import account configurations from legacy clients (if available).\n"
+        "  --forcelegacyconfigimport  : forcefully import account configurations from\n"
+        "                               legacy clients (if available).\n"
 #endif
-        "  --reverse            : use a reverse layout direction.\n";
+        "\n"
+        "Account setup wizard options:\n"
+        "  --overrideserverurl <url>  : server URL used by the interactive account\n"
+        "                               setup wizard.\n"
+        "  --overridelocaldir <dir>   : local directory used by the interactive\n"
+        "                               account setup wizard.\n"
+        "\n"
+        "Non-interactive account provisioning options:\n"
+        "  --serverurl <url>          : server URL to use during non-interactive\n"
+        "                               account provisioning.\n"
+        "  --userid <userid>          : server-side user ID to use during\n"
+        "                               non-interactive account provisioning.\n"
+        "  --apppassword <pass>       : server-side app password to use during\n"
+        "                               non-interactive account provisioning.\n"
+        "  --localdirpath <dir>       : local sync directory to use during\n"
+        "                               non-interactive account provisioning.\n"
+        "  --remotedirpath <dir>      : server-side directory to set up for syncing\n"
+        "                               during non-interactive account provisioning.\n"
+        "  --isvfsenabled <0|1>       : toggle virtual files support (1 = yes,\n"
+        "                               0 = no) during non-interactive account\n"
+        "                               provisioning.\n";
 
     QString applicationTrPath()
     {
@@ -125,7 +149,7 @@
 {
     ConfigFile configFile;
     const auto shouldTryToMigrate = configFile.shouldTryToMigrate();
     if (!shouldTryToMigrate) {
         qCInfo(lcApplication) << "This is not an upgrade/downgrade/migration. Proceed to read current application config file.";
         configFile.setMigrationPhase(ConfigFile::MigrationPhase::Done);
         return false;
@@ -301,7 +325,7 @@
                 qCDebug(lcApplication) << "Failed to move the old config directory to its new location (" << legacyDir << "to" << confDir << ")";
 
                 // Try to move the files one by one
                 if (QFileInfo(confDir).isDir() || QDir().mkdir(confDir)) {
                     const QStringList filesList = QDir(legacyDir).entryList(QDir::Files);
                     qCDebug(lcApplication) << "Will move the individual files" << filesList;
                     for (const auto &name : filesList) {
@@ -313,7 +337,7 @@
             } else {
 #ifndef Q_OS_WIN
                 // Create a symbolic link so a downgrade of the client would still find the config.
                 QFile::link(confDir, legacyDir);
 #endif
             }
         }
@@ -321,7 +345,7 @@
         setupConfigFile();
     }
 
     if (_theme->doNotUseProxy()) {
         ConfigFile().setProxyType(QNetworkProxy::NoProxy);
         const auto &allAccounts = AccountManager::instance()->accounts();
         for (const auto &accountState : allAccounts) {
@@ -419,7 +443,7 @@
     _gui->setupCloudProviders();
 #endif
 
     if (_theme->doNotUseProxy()) {
         ConfigFile().setProxyType(QNetworkProxy::NoProxy);
         const auto &allAccounts = AccountManager::instance()->accounts();
         for (const auto &accountState : allAccounts) {
@@ -609,7 +633,7 @@
         const auto foldersRestoreMessage = foldersListSize > 1
             ? tr("%1 folders", "number of folders imported").arg(QString::number(foldersListSize))
             : tr("1 folder");
         const auto messageBox = new QMessageBox(QMessageBox::Information,
                                                 tr("Legacy import"),
                                                 tr("Imported %1 and %2 from a legacy desktop client.\n%3",
                                                    "number of accounts and folders imported. list of users.")
@@ -626,7 +650,7 @@
     qCWarning(lcApplication) << accountsListSize << "account(s) migrated:" << prettyNamesList(accounts);
 }
 
 void Application::setupConfigFile()
 {
     // Migrate from version <= 2.4
     setApplicationName(_theme->appNameGUI());
@@ -661,11 +685,11 @@
         qCDebug(lcApplication) << "Failed to move the old config directory to its new location (" << oldDir << "to" << confDir << ")";
 
         // Try to move the files one by one
         if (QFileInfo(confDir).isDir() || QDir().mkdir(confDir)) {
             const QStringList filesList = QDir(oldDir).entryList(QDir::Files);
             qCDebug(lcApplication) << "Will move the individual files" << filesList;
             for (const auto &name : filesList) {
                 if (!QFile::rename(oldDir + "/" + name,  confDir + "/" + name)) {
                     qCDebug(lcApplication) << "Fallback move of " << name << "also failed";
                 }
             }
@@ -678,9 +702,9 @@
     }
 }
 
 AccountManager::AccountsRestoreResult Application::restoreLegacyAccount()
 {
     ConfigFile cfg;
     const auto tryMigrate = cfg.overrideServerUrl().isEmpty();
     auto accountsRestoreResult = AccountManager::AccountsRestoreFailure;
     if (accountsRestoreResult = AccountManager::instance()->restore(tryMigrate);
@@ -757,7 +781,7 @@
 // FIXME: This is not ideal yet since a ConnectionValidator might already be running and is in
 // progress of timing out in some seconds.
 // Maybe we need 2 validators, one triggered by timer, one by network configuration changes?
 void Application::slotSystemOnlineConfigurationChanged()
 {
     if (QNetworkInformation::instance()->reachability() == QNetworkInformation::Reachability::Site ||
             QNetworkInformation::instance()->reachability() == QNetworkInformation::Reachability::Online) {
@@ -880,119 +904,138 @@
 void Application::parseOptions(const QStringList &options)
 {
     QStringListIterator it(options);
-    // skip file name;
+    // skip file name
     if (it.hasNext()) {
         it.next();
     }
 
-    bool shouldExit = false;
+    const auto hasNextValue = [&it]() {
+        return it.hasNext() && !it.peekNext().startsWith(QLatin1String("--"));
+    };
 
-    //parse options; if help or bad option exit
+    const auto isOption = [](const QString &option, const char *name, const char *shortName = nullptr) {
+        return option == QLatin1String(name)
+            || (shortName && option == QLatin1String(shortName));
+    };
+
+    // parse options; if help or bad option exit
     while (it.hasNext()) {
         QString option = it.next();
-        if (option == QLatin1String("--help") || option == QLatin1String("-h")) {
+
+        // Help / version / exit-related
+        if (isOption(option, "--help", "-h")) {
             setHelp();
             break;
-        } else if (option == QLatin1String("--quit") || option == QLatin1String("-q")) {
+        } else if (isOption(option, "--version", "-v")) {
+            _versionOnly = true;
+        } else if (isOption(option, "--quit", "-q")) {
             _quitInstance = true;
-        } else if (option == QLatin1String("--logwindow") || option == QLatin1String("-l")) {
+
+        // Simple boolean toggles
+        } else if (isOption(option, "--logwindow", "-l")) {
             _showLogWindow = true;
-        } else if (option == QLatin1String("--logfile")) {
-            if (it.hasNext() && !it.peekNext().startsWith(QLatin1String("--"))) {
+        } else if (isOption(option, "--logflush")) {
+            _logFlush = true;
+        } else if (isOption(option, "--logdebug")) {
+            _logDebug = true;
+        } else if (isOption(option, "--background")) {
+            _backgroundMode = true;
+        } else if (isOption(option, "--reverse")) {
+            // FIXME: This is current implemented as a toggle, but help text suggests it should set RTL
+            setLayoutDirection(layoutDirection() == Qt::LeftToRight ? Qt::RightToLeft : Qt::LeftToRight);
+        } else if (isOption(option, "--debug")) {
+            _logDebug = true;
+            _debugMode = true;
+
+        // Value options
+        } else if (isOption(option, "--logfile")) {
+            if (hasNextValue()) {
                 _logFile = it.next();
             } else {
-                showHint("Log file not specified");
+                showHint("Missing value for --logfile");
             }
-        } else if (option == QLatin1String("--logdir")) {
-            if (it.hasNext() && !it.peekNext().startsWith(QLatin1String("--"))) {
+        } else if (isOption(option, "--logdir")) {
+            if (hasNextValue()) {
                 _logDir = it.next();
             } else {
-                showHint("Log dir not specified");
+                showHint("Missing value for --logdir");
             }
-        } else if (option == QLatin1String("--logexpire")) {
-            if (it.hasNext() && !it.peekNext().startsWith(QLatin1String("--"))) {
-                _logExpire = it.next().toInt();
+        } else if (isOption(option, "--logexpire")) {
+            if (hasNextValue()) {
+                bool ok = false;
+                const auto logExpire = it.next().toInt(&ok);
+                if (!ok) {
+                    showHint("Invalid value passed to --logexpire");
+                }
+                _logExpire = logExpire;
             } else {
-                showHint("Log expiration not specified");
+                showHint("Missing value for --logexpire");
             }
-        } else if (option == QLatin1String("--logflush")) {
-            _logFlush = true;
-        } else if (option == QLatin1String("--logdebug")) {
-            _logDebug = true;
-        } else if (option == QLatin1String("--confdir")) {
-            if (it.hasNext() && !it.peekNext().startsWith(QLatin1String("--"))) {
+        } else if (isOption(option, "--confdir")) {
+            if (hasNextValue()) {
                 QString confDir = it.next();
                 if (!ConfigFile::setConfDir(confDir)) {
                     showHint("Invalid path passed to --confdir");
                 }
             } else {
-                showHint("Path for confdir not specified");
-            }
-        } else if (option == QLatin1String("--debug")) {
-            _logDebug = true;
-            _debugMode = true;
-        } else if (option == QLatin1String("--background")) {
-            _backgroundMode = true;
-        } else if (option == QLatin1String("--version") || option == QLatin1String("-v")) {
-            _versionOnly = true;
-        } else if (option == QLatin1String("--reverse")) {
-            setLayoutDirection(layoutDirection() == Qt::LeftToRight ? Qt::RightToLeft : Qt::LeftToRight);
-        } else if (option.endsWith(QStringLiteral(APPLICATION_DOTVIRTUALFILE_SUFFIX))) {
-            // virtual file, open it after the Folder were created (if the app is not terminated)
-            QTimer::singleShot(0, this, [this, option] { openVirtualFile(option); });
-        } else if (option.startsWith(QStringLiteral(APPLICATION_URI_HANDLER_SCHEME "://open"))) {
-            // see the section Local file editing of the Architecture page of the user documentation
-            _editFileLocallyUrl = QUrl::fromUserInput(option);
-            if (!_editFileLocallyUrl.isValid()) {
-                _editFileLocallyUrl.clear();
-                const auto errorParsingLocalFileEditingUrl = QStringLiteral("The supplied url for local file editing '%1' is invalid!").arg(option);
-                qCInfo(lcApplication) << errorParsingLocalFileEditingUrl;
-                showHint(errorParsingLocalFileEditingUrl.toStdString());
+                showHint("Missing value for --confdir");
             }
-        } else if (option == QStringLiteral("--overrideserverurl")) {
-            if (it.hasNext() && !it.peekNext().startsWith(QLatin1String("--"))) {
+        } else if (isOption(option, "--overrideserverurl")) {
+            if (hasNextValue()) {
                 const auto overrideUrl = it.next();
-                const auto isUrlValid = (overrideUrl.startsWith(QStringLiteral("http://")) || overrideUrl.startsWith(QStringLiteral("https://")))
+                const auto isUrlValid =
+                    (overrideUrl.startsWith(QStringLiteral("http://"))
+                     || overrideUrl.startsWith(QStringLiteral("https://")))
                     && QUrl::fromUserInput(overrideUrl).isValid();
                 if (!isUrlValid) {
                     showHint("Invalid URL passed to --overrideserverurl");
-                    shouldExit = true;
-                } else {
-                    _overrideServerUrl = overrideUrl;
                 }
+                _overrideServerUrl = overrideUrl;
             } else {
-                showHint("Invalid URL passed to --overrideserverurl");
+                showHint("Missing value for --overrideserverurl");
             }
-        } else if (option == QStringLiteral("--overridelocaldir")) {
-            if (it.hasNext() && !it.peekNext().startsWith(QLatin1String("--"))) {
+        } else if (isOption(option, "--overridelocaldir")) {
+            if (hasNextValue()) {
+                // FIXME: validate specified path
                 _overrideLocalDir = it.next();
             } else {
-                showHint("Invalid URL passed to --overridelocaldir");
+                showHint("Missing value for --overridelocaldir");
             }
-        } else if (option == QStringLiteral("--set-language")) {
-            if (it.hasNext() && !it.peekNext().startsWith(QLatin1String("--"))) {
+        } else if (isOption(option, "--set-language")) {
+            if (hasNextValue()) {
                 _setLanguage = it.next();
+            } else {
+                showHint("Missing value for --set-language");
+            }
+
+        // Special startup handlers
+        } else if (option.endsWith(QStringLiteral(APPLICATION_DOTVIRTUALFILE_SUFFIX))) {
+            // virtual file, open it after the Folder were created (if the app is not terminated)
+            QTimer::singleShot(0, this, [this, option] { openVirtualFile(option); });
+        } else if (option.startsWith(QStringLiteral(APPLICATION_URI_HANDLER_SCHEME "://open"))) {
+            // see the section Local file editing of the Architecture page of the user documentation
+            _editFileLocallyUrl = QUrl::fromUserInput(option);
+            if (!_editFileLocallyUrl.isValid()) {
+                _editFileLocallyUrl.clear();
+                const auto errorParsingLocalFileEditingUrl =
+                    QStringLiteral("The supplied url for local file editing '%1' is invalid!").arg(option);
+                qCInfo(lcApplication) << errorParsingLocalFileEditingUrl;
+                showHint(errorParsingLocalFileEditingUrl.toStdString());
             }
-        }
 #if !DISABLE_ACCOUNT_MIGRATION
-        else if (option == QStringLiteral("--forcelegacyconfigimport")) {
+        } else if (isOption(option, "--forcelegacyconfigimport")) {
             AccountManager::instance()->setForceLegacyImport(true);
-        }
 #endif
-        else {
+        } else {
             QString errorMessage;
             if (!AccountSetupCommandLineManager::instance()->parseCommandlineOption(option, it, errorMessage)) {
                 if (!errorMessage.isEmpty()) {
                     showHint(errorMessage.toStdString());
-                    return;
                 }
                 showHint("Unrecognized option '" + option.toStdString() + "'");
             }
         }
     }
-    if (shouldExit) {
-        std::exit(0);
-    }
 }
 
 // Helpers for displaying messages. Note that there is no console on Windows.
@@ -1094,7 +1137,7 @@
 {
     const ConfigFile cfg;
     const auto configLanguage = cfg.language();
     if (!configLanguage.isEmpty()) {
         // always prefer value from configuration
         return configLanguage;
     }
@@ -1146,8 +1189,8 @@
         const QString qtBaseTrFile = QLatin1String("qtbase_") + choosenLanguage;
         if (!qtTranslator->load(qtTrFile, qtTrPath)) {
             if (!qtTranslator->load(qtTrFile, trPath)) {
                 if (!qtTranslator->load(qtBaseTrFile, qtTrPath)) {
                     if (!qtTranslator->load(qtBaseTrFile, trPath)) {
                         qCDebug(lcApplication()) << "impossible to load Qt translation catalog" << qtBaseTrFile;
                     }
                 }
@@ -1155,7 +1198,7 @@
         }
         const QString qtkeychainTrFile = QLatin1String("qtkeychain_") + choosenLanguage;
         if (!qtkeychainTranslator->load(qtkeychainTrFile, qtTrPath)) {
             if (!qtkeychainTranslator->load(qtkeychainTrFile, trPath)) {
                 qCDebug(lcApplication()) << "impossible to load QtKeychain translation catalog" << qtkeychainTrFile;
             }
         }
@@ -1205,7 +1248,7 @@
 
 void Application::openVirtualFile(const QString &filename)
 {
     QString virtualFileExt = QStringLiteral(APPLICATION_DOTVIRTUALFILE_SUFFIX);
     if (!filename.endsWith(virtualFileExt)) {
         qWarning(lcApplication) << "Can only handle file ending in .owncloud. Unable to open" << filename;
         return;