From 3ee73d48c9d819b2e186655b7ba86ebc2a961baf Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 3 Aug 2026 09:28:33 +0200 Subject: fix: only interrupt startup for real configuration problems Found while walking the task 13 checklist against real mail. Item 1 ("startup shows no configuration warnings with a valid config") failed: with a perfectly valid config that simply had no [sync] command, every launch opened a blocking modal that had to be dismissed before the window could be used. Config now separates the two cases. A problem is something configured but wrong (a sync command that does not exist, an account with no maildir); those still open a dialog, as does every KeyMap warning, since each one means a binding the user wrote is being ignored. A notice is an optional feature simply not being configured; it reports to the status bar only. Nothing is broken in that case, and a modal on every launch teaches the user to dismiss dialogs unread, which defeats the ones that matter. problems() is a subset of warnings(), so callers wanting everything need only the latter. Co-Authored-By: Claude Opus 5 --- src/config.cpp | 19 +++++++++++-- src/config.h | 18 +++++++++++- src/mainwindow.cpp | 18 +++++++++--- tests/test_config.cpp | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 123 insertions(+), 8 deletions(-) diff --git a/src/config.cpp b/src/config.cpp index b12130d..0253a74 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -19,6 +19,17 @@ QString Config::defaultPath() return base + QStringLiteral("/qtmaildir/qtmaildir.conf"); } +void Config::addProblem(const QString &message) +{ + m_warnings.append(message); + m_problems.append(message); +} + +void Config::addNotice(const QString &message) +{ + m_warnings.append(message); +} + void Config::load(const QString &path) { QSettings settings(path, QSettings::IniFormat); @@ -28,10 +39,12 @@ void Config::load(const QString &path) m_syncCommand = settings.value(QStringLiteral("sync/command")).toString(); if (m_syncCommand.isEmpty()) { - m_warnings.append(QStringLiteral( + // Not a problem: sync is optional, and nothing the user asked for is + // being ignored. A modal here would fire on every launch. + addNotice(QStringLiteral( "No sync command configured ([sync] command); syncing is disabled.")); } else if (!QFileInfo::exists(m_syncCommand.split(QLatin1Char(' ')).first())) { - m_warnings.append( + addProblem( QStringLiteral("Sync command '%1' does not exist; syncing is disabled.") .arg(m_syncCommand)); m_syncCommand.clear(); @@ -61,7 +74,7 @@ void Config::load(const QString &path) settings.endGroup(); if (!account.isValid()) { - m_warnings.append( + addProblem( QStringLiteral("Account '%1' has no maildir; ignoring it.") .arg(account.key)); continue; diff --git a/src/config.h b/src/config.h index f037db5..80f6101 100644 --- a/src/config.h +++ b/src/config.h @@ -50,13 +50,29 @@ public: /// Optional alternate notmuch config file. Empty means "let notmuch decide". QString notmuchConfig() const { return m_notmuchConfig; } - /// Non-fatal problems, shown once in a startup banner. + /// Every non-fatal problem, both kinds below. Shown in the status bar. QStringList warnings() const { return m_warnings; } + /// The subset worth interrupting startup for: something in the config is + /// wrong and the user's stated intent is not being honoured (a malformed + /// account, an unparseable key binding, a sync command that does not + /// exist). An optional setting simply being absent is NOT one of these: + /// nothing is broken, the feature is just off, and a modal on every launch + /// trains the user to dismiss dialogs without reading them. + QStringList problems() const { return m_problems; } + private: + /// Records a problem: something configured but wrong. Also appears in + /// warnings(), so callers that want everything need only that one. + void addProblem(const QString &message); + + /// Records a notice: nothing is wrong, a feature is simply not configured. + void addNotice(const QString &message); + QList m_accounts; QList m_savedQueries; QString m_syncCommand; QString m_notmuchConfig; QStringList m_warnings; + QStringList m_problems; }; diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index ed16010..d6eccf9 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -274,15 +274,25 @@ void MainWindow::wireWorker() void MainWindow::showWarnings() { - QStringList warnings = m_config.warnings() + m_keyMap.warnings(); + const QStringList warnings = m_config.warnings() + m_keyMap.warnings(); if (warnings.isEmpty()) return; // Non-fatal: the app runs degraded rather than refusing to start. m_statusLabel->setText( - tr("%1 configuration warning(s); see Help").arg(warnings.size())); - QMessageBox::warning(this, tr("Configuration warnings"), - warnings.join(QLatin1Char('\n'))); + tr("%n configuration warning(s)", "", warnings.size())); + + // Interrupt startup only for things that are actually wrong. Every KeyMap + // warning qualifies (each one means a binding the user wrote is being + // ignored), but a Config notice such as "no sync command configured" does + // not: nothing is broken, the feature is simply off, and a modal on every + // launch teaches the user to dismiss dialogs unread. + const QStringList problems = m_config.problems() + m_keyMap.warnings(); + if (problems.isEmpty()) + return; + + QMessageBox::warning(this, tr("Configuration problems"), + problems.join(QLatin1Char('\n'))); } void MainWindow::runCurrentQuery() diff --git a/tests/test_config.cpp b/tests/test_config.cpp index 0caa93c..50d7e67 100644 --- a/tests/test_config.cpp +++ b/tests/test_config.cpp @@ -12,6 +12,10 @@ private slots: void missingSyncCommandIsEmpty(); void accountWithoutMaildirIsRejected(); void scopedQueryWrapsCorrectly(); + void absentSyncCommandIsNoticeNotProblem(); + void brokenSyncCommandIsAProblem(); + void malformedAccountIsAProblem(); + void validConfigHasNoProblems(); }; static QString writeIni(const QTemporaryDir &dir, const QString &body) @@ -128,5 +132,77 @@ void TestConfig::scopedQueryWrapsCorrectly() QStringLiteral("path:\"work-mail/**\"")); } +void TestConfig::absentSyncCommandIsNoticeNotProblem() +{ + // An optional feature simply not being configured must not interrupt + // startup: the modal would fire on every launch and train the user to + // dismiss dialogs without reading them. + QTemporaryDir dir; + const QString path = writeIni(dir, QStringLiteral( + "[account.work]\n" + "maildir=work-mail\n")); + + Config config; + config.load(path); + + QCOMPARE(config.warnings().size(), 1); + QVERIFY(config.warnings().first().contains(QStringLiteral("No sync command"))); + QVERIFY(config.problems().isEmpty()); +} + +void TestConfig::brokenSyncCommandIsAProblem() +{ + // Configured but missing is different: the user asked for sync and is not + // getting it, so they need telling. + QTemporaryDir dir; + const QString path = writeIni(dir, QStringLiteral( + "[sync]\n" + "command=/nonexistent/qtmaildir-test/mailsync.sh\n" + "\n" + "[account.work]\n" + "maildir=work-mail\n")); + + Config config; + config.load(path); + + QCOMPARE(config.problems().size(), 1); + QVERIFY(config.problems().first().contains(QStringLiteral("does not exist"))); + // Problems are a subset of warnings, so a caller wanting everything needs + // only warnings(). + QVERIFY(config.warnings().contains(config.problems().first())); +} + +void TestConfig::malformedAccountIsAProblem() +{ + QTemporaryDir dir; + const QString path = writeIni(dir, QStringLiteral( + "[account.broken]\n" + "name=No Maildir Here\n")); + + Config config; + config.load(path); + + QVERIFY(!config.problems().isEmpty()); + QVERIFY(config.problems().first().contains(QStringLiteral("broken"))); +} + +void TestConfig::validConfigHasNoProblems() +{ + QTemporaryDir dir; + const QString path = writeIni(dir, QStringLiteral( + "[sync]\n" + "command=/bin/true\n" + "\n" + "[account.work]\n" + "maildir=work-mail\n" + "address=user@example.org\n")); + + Config config; + config.load(path); + + QVERIFY(config.problems().isEmpty()); + QVERIFY(config.warnings().isEmpty()); +} + QTEST_MAIN(TestConfig) #include "test_config.moc" -- cgit v1.2.3