From 8fc28de18f903e4bc9d9777589edc819ed9ea996 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 20 Aug 2026 18:25:31 +0200 Subject: feat(config): send_command and the [compose] section, item 123 An account's ability to send IS its send_command's presence. Not a separate receive_only key: with one key there is nothing to keep in step and nothing to contradict, and a receive-only account is expressed by omission, which is how one real account here is meant to work. Startup validation follows the startup_query pattern, and is deliberately asymmetric. A default_account that cannot send is warned about, because the user named an account and expects mail to come from it. An installation where NO account can send is not: that is a valid read-only installation, and warning about it would train the user to ignore warnings. Every [compose] key reads through value(key, default) rather than testing contains(), because send_delay_ms = 0 is a real setting meaning 'send at once' that a zero-test would mistake for unset. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015muoUo2GdxmBDSp5vjYcbE --- src/config.cpp | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) (limited to 'src/config.cpp') diff --git a/src/config.cpp b/src/config.cpp index a2d1cec..09869b9 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -23,6 +23,8 @@ // so this reports the same numbers rather than keeping a second copy. #include "messageview.h" +#include + #include #include #include @@ -469,6 +471,12 @@ void Config::load(const QString &path) account.inbox = settings.value(QStringLiteral("inbox")).toString().trimmed(); + // Optional, and its absence IS the receive-only state: see the field + // comment in config.h. Run without a shell, so trimming here is only + // whitespace hygiene, never a quoting concern. + account.sendCommand = + settings.value(QStringLiteral("send_command")).toString().trimmed(); + // Both optional, and both describe this account's chip in the thread // list. An account tag is a different taxonomy from a functional one, // saying which mailbox a thread arrived in rather than what state it @@ -516,6 +524,29 @@ void Config::load(const QString &path) m_accounts.append(account); } + settings.beginGroup(QStringLiteral("compose")); + // value(key, default) throughout rather than testing contains(): an + // absent key and a key set to its default must behave identically, and + // send_delay_ms = 0 is a REAL setting meaning "send at once" that a + // zero-test would mistake for unset. + m_compose.quotePosition = + settings.value(QStringLiteral("quote_position"), QStringLiteral("above")) + .toString().compare(QStringLiteral("below"), Qt::CaseInsensitive) == 0 + ? ComposeSettings::QuotePosition::Below + : ComposeSettings::QuotePosition::Above; + m_compose.sendHtml = + settings.value(QStringLiteral("send_html"), true).toBool(); + m_compose.autosaveIntervalMs = + settings.value(QStringLiteral("autosave_interval_ms"), 30000).toInt(); + m_compose.sendDelayMs = + settings.value(QStringLiteral("send_delay_ms"), 5000).toInt(); + m_compose.defaultAccount = + settings.value(QStringLiteral("default_account")).toString().trimmed(); + m_compose.attachmentWarnBytes = + settings.value(QStringLiteral("attachment_warn_bytes"), qint64(26214400)) + .toLongLong(); + settings.endGroup(); + loadSavedQueries(path, settings); // Checked here rather than where startup_query is read: the saved queries @@ -534,6 +565,48 @@ void Config::load(const QString &path) m_startupAccount.clear(); } + // default_account is validated here, once the accounts are parsed. A + // named account that cannot send is reported: the user named an account + // and expects mail to come from it, unlike an installation where no + // account can send at all, which is a valid read-only setup and not + // warned about below. + if (!m_compose.defaultAccount.isEmpty()) { + const auto named = std::find_if( + m_accounts.cbegin(), m_accounts.cend(), + [this](const Account &a) { return a.key == m_compose.defaultAccount; }); + + if (named == m_accounts.cend()) { + addProblem( + tr("[compose] default_account names '%1', which is not a " + "configured account. A new message will pick a sending " + "account by the usual rules.") + .arg(m_compose.defaultAccount)); + } else if (!named->canSend()) { + addProblem( + tr("[compose] default_account names '%1', which has no " + "send_command and cannot send. A new message will pick a " + "sending account by the usual rules.") + .arg(m_compose.defaultAccount)); + } + } + + for (const Account &account : m_accounts) { + if (!account.canSend()) + continue; + if (account.sent.isEmpty()) { + addProblem( + tr("Account '%1' can send but configures no `sent` folder, so " + "no local copy of sent mail is filed.") + .arg(account.key)); + } + if (account.drafts.isEmpty()) { + addProblem( + tr("Account '%1' can send but configures no `drafts` folder, " + "so the composer runs without draft protection.") + .arg(account.key)); + } + } + // Asks whether the resolved query matched on EITHER a name or a generator, // rather than comparing the name alone. Comparing names warned about a // config that was working: `startup_query = Inbox` resolves through the @@ -948,6 +1021,16 @@ SavedQuery Config::startupSavedQuery() const return builtinFilter(QStringLiteral("unread")); } +QList Config::sendingAccounts() const +{ + QList sending; + for (const Account &account : m_accounts) { + if (account.canSend()) + sending.append(account); + } + return sending; +} + Account Config::account(const QString &key) const { for (const Account &a : m_accounts) { -- cgit v1.2.3 From 90cf14b0b23218f0ca01a280998e855a00b47567 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 20 Aug 2026 18:38:13 +0200 Subject: fix(config): reject garbage numerics instead of silently reading zero, item 123 toInt() and toLongLong() return 0 on failure rather than the default, so a typo in autosave_interval_ms produced a zero-interval timer. That timer is restarted on every keystroke, so it would fire on the next event-loop pass and turn a 30 second debounce into a Maildir write per keystroke, each one uploaded by mbsync: exactly the behaviour the debounce exists to prevent. This file already had the right shape in five places, a checked parse that reports the bad value and keeps the default. The [compose] keys were the only numerics skipping it. The interval is also clamped, since nothing assigns a meaning to a zero or negative autosave. quote_position now warns on an unrecognised value, matching sync_on_exit, language and date_format; the only silent fallbacks in this file are for absent keys rather than malformed ones. And a missing `sent` folder is a notice rather than a problem, because the spec blesses that configuration and a modal on every launch for a permanently correct setup is how users learn to dismiss dialogs unread. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015muoUo2GdxmBDSp5vjYcbE --- src/config.cpp | 109 ++++++++++++++++++++++++++++++++++------ src/types.h | 2 +- tests/test_config.cpp | 91 +++++++++++++++++++++++++++++++++ translations/qtmaildir_it_IT.ts | 16 ++++++ 4 files changed, 201 insertions(+), 17 deletions(-) (limited to 'src/config.cpp') diff --git a/src/config.cpp b/src/config.cpp index 09869b9..c6bedd2 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -525,26 +525,90 @@ void Config::load(const QString &path) } settings.beginGroup(QStringLiteral("compose")); - // value(key, default) throughout rather than testing contains(): an - // absent key and a key set to its default must behave identically, and - // send_delay_ms = 0 is a REAL setting meaning "send at once" that a - // zero-test would mistake for unset. - m_compose.quotePosition = + // Absent keys stay silent (the struct's own default holds), but a + // PRESENT and malformed value is reported: value(key, default) alone + // would happily accept "quote_position = abov" as Above, matching every + // other enum-ish key in this file (sync_on_exit, language, date_format) + // rather than being the one silent exception. + const QString quotePosition = settings.value(QStringLiteral("quote_position"), QStringLiteral("above")) - .toString().compare(QStringLiteral("below"), Qt::CaseInsensitive) == 0 - ? ComposeSettings::QuotePosition::Below - : ComposeSettings::QuotePosition::Above; + .toString().trimmed(); + if (quotePosition.compare(QStringLiteral("above"), Qt::CaseInsensitive) == 0) { + m_compose.quotePosition = ComposeSettings::QuotePosition::Above; + } else if (quotePosition.compare(QStringLiteral("below"), Qt::CaseInsensitive) == 0) { + m_compose.quotePosition = ComposeSettings::QuotePosition::Below; + } else { + addProblem(tr("[compose] quote_position '%1' is not recognised; " + "expected above or below. Using above.") + .arg(quotePosition)); + } + m_compose.sendHtml = settings.value(QStringLiteral("send_html"), true).toBool(); - m_compose.autosaveIntervalMs = - settings.value(QStringLiteral("autosave_interval_ms"), 30000).toInt(); - m_compose.sendDelayMs = - settings.value(QStringLiteral("send_delay_ms"), 5000).toInt(); + + // Three numerics, all following the shape already established at + // message_zoom, toolbar_icon_size, mark_read_delay_ms and + // auto_sync_delay_ms elsewhere in this function: a QVariant, a checked + // toInt()/toLongLong(), and a reported fallback to the struct's own + // default on failure. The bare toInt()/toLongLong() this replaced return + // 0 on a PARSE FAILURE, not the default, which is silently indistinguishable + // from the user writing 0 on purpose. For autosave_interval_ms that 0 + // reaches a QTimer restarted on every keystroke, so it would fire on the + // very next event-loop pass and turn the debounce into a write per + // keystroke, each one uploaded by mbsync. + const QVariant autosave = settings.value(QStringLiteral("autosave_interval_ms")); + if (autosave.isValid()) { + bool ok = false; + const int value = autosave.toString().trimmed().toInt(&ok); + if (ok) { + // Clamped, not merely parsed: nothing in the spec assigns a + // meaning to a zero or negative autosave interval, unlike + // mark_read_delay_ms where negative-means-off is documented + // behaviour. A zero interval here is the same runaway-write + // hazard as the parse failure above, just spelled correctly. + m_compose.autosaveIntervalMs = qMax(1000, value); + } else { + addProblem(tr("[compose] autosave_interval_ms '%1' is not a " + "number; using %2.") + .arg(autosave.toString()) + .arg(m_compose.autosaveIntervalMs)); + } + } + + // Zero is a REAL setting here, meaning "send at once", and must be + // honoured rather than mistaken for unset: that is exactly why this is + // isValid()-then-checked-parse rather than a zero-test. + const QVariant sendDelay = settings.value(QStringLiteral("send_delay_ms")); + if (sendDelay.isValid()) { + bool ok = false; + const int value = sendDelay.toString().trimmed().toInt(&ok); + if (ok) { + m_compose.sendDelayMs = value; + } else { + addProblem(tr("[compose] send_delay_ms '%1' is not a number; " + "using %2.") + .arg(sendDelay.toString()) + .arg(m_compose.sendDelayMs)); + } + } + m_compose.defaultAccount = settings.value(QStringLiteral("default_account")).toString().trimmed(); - m_compose.attachmentWarnBytes = - settings.value(QStringLiteral("attachment_warn_bytes"), qint64(26214400)) - .toLongLong(); + + const QVariant attachmentWarn = + settings.value(QStringLiteral("attachment_warn_bytes")); + if (attachmentWarn.isValid()) { + bool ok = false; + const qint64 value = attachmentWarn.toString().trimmed().toLongLong(&ok); + if (ok) { + m_compose.attachmentWarnBytes = value; + } else { + addProblem(tr("[compose] attachment_warn_bytes '%1' is not a " + "number; using %2.") + .arg(attachmentWarn.toString()) + .arg(m_compose.attachmentWarnBytes)); + } + } settings.endGroup(); loadSavedQueries(path, settings); @@ -570,6 +634,11 @@ void Config::load(const QString &path) // and expects mail to come from it, unlike an installation where no // account can send at all, which is a valid read-only setup and not // warned about below. + // + // Unlike startup_account just above, the bad value is NOT cleared after + // the warning: the composer resolves this through canSend() at the point + // of use, so a value naming an unusable account is simply skipped there + // rather than needing to be blanked here. if (!m_compose.defaultAccount.isEmpty()) { const auto named = std::find_if( m_accounts.cbegin(), m_accounts.cend(), @@ -593,12 +662,20 @@ void Config::load(const QString &path) for (const Account &account : m_accounts) { if (!account.canSend()) continue; + // A notice, not a problem: a provider whose SMTP server files sent + // mail on its own is a legitimate, permanently correct configuration. + // addProblem() here would raise a startup modal on every launch for a + // setup that will never change, which is exactly how a user learns to + // dismiss dialogs unread. if (account.sent.isEmpty()) { - addProblem( + addNotice( tr("Account '%1' can send but configures no `sent` folder, so " "no local copy of sent mail is filed.") .arg(account.key)); } + // Still a problem: unlike a missing sent folder, this is a real loss + // of protection (no draft is saved while composing) rather than a + // deliberate provider-side choice. if (account.drafts.isEmpty()) { addProblem( tr("Account '%1' can send but configures no `drafts` folder, " diff --git a/src/types.h b/src/types.h index 0c6532e..99c271d 100644 --- a/src/types.h +++ b/src/types.h @@ -249,7 +249,7 @@ struct ComposeContext { enum class Kind { New, Reply, ReplyAll, Forward }; - QString accountKey; ///< Which account sends. Resolved by ComposeContext's rules. + QString accountKey; ///< Which account sends. Plain data here; the resolution rules live with whatever builds this context. Kind kind = Kind::New; QString originalPath; ///< The .eml being replied to or forwarded. Empty for New. QString inReplyTo; ///< Message-ID of the original. diff --git a/tests/test_config.cpp b/tests/test_config.cpp index 2df4c40..a902425 100644 --- a/tests/test_config.cpp +++ b/tests/test_config.cpp @@ -126,6 +126,11 @@ private slots: void aZeroSendDelayIsHonouredRatherThanTreatedAsUnset(); void aDefaultAccountThatCannotSendIsWarnedAbout(); void anInstallationWhereNoAccountCanSendIsNotWarnedAbout(); + void garbageAutosaveIntervalIsRejectedNotZero(); + void garbageSendDelayIsRejectedNotZero(); + void garbageAttachmentWarnBytesIsRejectedNotZero(); + void zeroOrNegativeAutosaveIntervalIsClamped(); + void unrecognisedQuotePositionWarnsAndFallsBackToAbove(); }; static QString writeIni(const QTemporaryDir &dir, const QString &body) @@ -2418,5 +2423,91 @@ void TestConfig::anInstallationWhereNoAccountCanSendIsNotWarnedAbout() } } +void TestConfig::garbageAutosaveIntervalIsRejectedNotZero() +{ + // toInt() alone returns 0 on a parse failure, not the default, and 0 + // reaches a QTimer restarted on every keystroke: a typo here would have + // turned the debounce into a write per keystroke, uploaded by mbsync. + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral( + "[compose]\n" + "autosave_interval_ms=oops\n"))); + + QCOMPARE(config.compose().autosaveIntervalMs, 30000); + QVERIFY2(!config.problems().isEmpty(), + "a garbage autosave_interval_ms was accepted silently"); +} + +void TestConfig::garbageSendDelayIsRejectedNotZero() +{ + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral( + "[compose]\n" + "send_delay_ms=soon\n"))); + + QCOMPARE(config.compose().sendDelayMs, 5000); + QVERIFY2(!config.problems().isEmpty(), + "a garbage send_delay_ms was accepted silently"); +} + +void TestConfig::garbageAttachmentWarnBytesIsRejectedNotZero() +{ + // Verified against the actual defect: attachment_warn_bytes=banana gave 0 + // via a bare toLongLong(), which would have warned about every attachment + // no matter how small. + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral( + "[compose]\n" + "attachment_warn_bytes=banana\n"))); + + QCOMPARE(config.compose().attachmentWarnBytes, qint64(26214400)); + QVERIFY2(!config.problems().isEmpty(), + "a garbage attachment_warn_bytes was accepted silently"); +} + +void TestConfig::zeroOrNegativeAutosaveIntervalIsClamped() +{ + // Independent of the parse fix: a value that parses fine but is zero or + // negative must still not reach setInterval(), since nothing assigns a + // meaning to one, unlike mark_read_delay_ms's documented negative-means-off. + QTemporaryDir dir; + Config zero; + zero.load(writeIni(dir, QStringLiteral( + "[compose]\n" + "autosave_interval_ms=0\n"))); + QVERIFY2(zero.compose().autosaveIntervalMs >= 1000, + qPrintable(QStringLiteral("zero autosave interval was not clamped: %1") + .arg(zero.compose().autosaveIntervalMs))); + + QTemporaryDir dir2; + Config negative; + negative.load(writeIni(dir2, QStringLiteral( + "[compose]\n" + "autosave_interval_ms=-500\n"))); + QVERIFY2(negative.compose().autosaveIntervalMs >= 1000, + qPrintable(QStringLiteral("negative autosave interval was not clamped: %1") + .arg(negative.compose().autosaveIntervalMs))); +} + +void TestConfig::unrecognisedQuotePositionWarnsAndFallsBackToAbove() +{ + // Matches the precedent set by sync_on_exit, language and date_format: + // the only silent fallbacks in this file are for ABSENT keys, never for + // malformed ones. + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral( + "[compose]\n" + "quote_position=abov\n"))); + + QVERIFY2(config.compose().quotePosition == ComposeSettings::QuotePosition::Above, + "an unrecognised quote_position must still fall back to Above"); + QVERIFY2(!config.problems().isEmpty(), + "an unrecognised quote_position was accepted silently"); +} + QTEST_MAIN(TestConfig) #include "test_config.moc" diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts index edc9d5e..4dbc62d 100644 --- a/translations/qtmaildir_it_IT.ts +++ b/translations/qtmaildir_it_IT.ts @@ -47,6 +47,22 @@ Account '%1' has no trash folder configured; add a 'trash' key to its section. Delete will not work for this account until it does. L'account '%1' non ha un cestino configurato; aggiungere una chiave 'trash' alla sua sezione. L'eliminazione non funzionerà per questo account finché non verrà fatto. + + [compose] quote_position '%1' is not recognised; expected above or below. Using above. + [compose] quote_position '%1' non è riconosciuto; atteso above o below. Verrà usato above. + + + [compose] autosave_interval_ms '%1' is not a number; using %2. + [compose] autosave_interval_ms '%1' non è un numero; verrà usato %2. + + + [compose] send_delay_ms '%1' is not a number; using %2. + [compose] send_delay_ms '%1' non è un numero; verrà usato %2. + + + [compose] attachment_warn_bytes '%1' is not a number; using %2. + [compose] attachment_warn_bytes '%1' non è un numero; verrà usato %2. + Startup account '%1' is not a configured account; starting on all accounts. L'account iniziale '%1' non è un account configurato; si parte da tutti gli account. -- cgit v1.2.3