diff options
| -rw-r--r-- | src/config.cpp | 185 | ||||
| -rw-r--r-- | src/config.h | 65 | ||||
| -rw-r--r-- | tests/test_config.cpp | 331 |
3 files changed, 566 insertions, 15 deletions
diff --git a/src/config.cpp b/src/config.cpp index b1f730c..8cd5e56 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -24,8 +24,14 @@ #include "messageview.h" #include <QDateTime> +#include <QDir> +#include <QFile> #include <QFileInfo> +#include <QJsonArray> +#include <QJsonDocument> +#include <QJsonObject> #include <QLocale> +#include <QSaveFile> #include <QSettings> #include <QStandardPaths> @@ -37,6 +43,14 @@ namespace { constexpr int kMinToolbarIconSize = 16; constexpr int kMaxToolbarIconSize = 64; +/// queries.json format version. Bump only for a BREAKING change: an optional +/// field needs no bump, because an older build preserves what it does not +/// understand rather than dropping it. +/// +/// Unlike rules.json this file has ONE implementation, so a bump here is not a +/// two-repo change and no hook stops tagging if it is half-deployed. +constexpr int kQueriesFormatVersion = 1; + } // namespace QString Account::scopedQuery(const QString &query) const @@ -397,17 +411,10 @@ void Config::load(const QString &path) m_accounts.append(account); } - settings.beginGroup(QStringLiteral("queries")); - // QSettings::childKeys() returns keys in alphabetical order, not file - // order, so the saved-query button order in the UI is alphabetical too. - // A hand-rolled parser would be needed to preserve file order; not - // needed in v1. - for (const QString &name : settings.childKeys()) - m_savedQueries.append({ name, settings.value(name).toString() }); - settings.endGroup(); - - // Checked here rather than where startup_query is read: [queries] is not - // parsed until now. Only a name the user actually wrote is worth a + loadSavedQueries(path, settings); + + // Checked here rather than where startup_query is read: the saved queries + // are not parsed until now. Only a name the user actually wrote is worth a // problem; the built-in default naming a query they never created is not // something they got wrong. if (m_startupQueryWasSet && !m_savedQueries.isEmpty() @@ -419,6 +426,162 @@ void Config::load(const QString &path) } } +QString Config::queriesPath(const QString &configPath) +{ + return QFileInfo(configPath).absolutePath() + + QStringLiteral("/queries.json"); +} + +void Config::loadSavedQueries(const QString &configPath, QSettings &settings) +{ + m_queriesPath = queriesPath(configPath); + + QFile file(m_queriesPath); + if (!file.exists()) { + // Migration. Read [queries] once, write the JSON, and leave the INI + // section alone: stripping it would mean rewriting a hand-edited file + // with QSettings, which drops comments and key order across the WHOLE + // file. A few stale lines the user can delete by hand is the cheaper + // loss, and it keeps a downgrade working. + settings.beginGroup(QStringLiteral("queries")); + const QStringList names = settings.childKeys(); + for (const QString &name : names) { + SavedQuery query; + query.name = name; + query.query = settings.value(name).toString(); + // Pinned, because these are buttons today. A migration that left + // them unpinned would empty the query row on the first launch + // after an upgrade, which reads as data loss. + query.pinned = true; + m_savedQueries.append(query); + } + settings.endGroup(); + + // Order is alphabetical here because childKeys() is genuinely all the + // INI knows. The user reorders once and it sticks from then on. + if (!names.isEmpty() && !saveSavedQueries()) { + addProblem(QStringLiteral("Could not write saved queries to %1.") + .arg(m_queriesPath)); + } + return; + } + + if (!file.open(QIODevice::ReadOnly)) { + addProblem(QStringLiteral("Could not read %1: %2.") + .arg(m_queriesPath, file.errorString())); + m_queriesRefused = true; + return; + } + + QJsonParseError error; + const QJsonDocument document = + QJsonDocument::fromJson(file.readAll(), &error); + file.close(); + + if (error.error != QJsonParseError::NoError || !document.isObject()) { + addProblem(QStringLiteral("%1 is not valid JSON: %2.") + .arg(m_queriesPath, error.errorString())); + m_queriesRefused = true; + return; + } + + const QJsonObject root = document.object(); + const int version = + root.value(QStringLiteral("version")).toInt(kQueriesFormatVersion); + if (version != kQueriesFormatVersion) { + // Refused rather than guessed at, and the refusal blocks the save: + // rewriting a newer document with this build's reading of it would + // destroy whatever the newer build stored. + addProblem(QStringLiteral("%1 has format version %2; this build " + "understands %3. Saved queries were not " + "loaded.") + .arg(m_queriesPath) + .arg(version) + .arg(kQueriesFormatVersion)); + m_queriesRefused = true; + return; + } + + for (auto it = root.begin(); it != root.end(); ++it) { + if (it.key() != QStringLiteral("version") + && it.key() != QStringLiteral("queries")) + m_queriesUnknown.insert(it.key(), it.value()); + } + + const QJsonArray array = root.value(QStringLiteral("queries")).toArray(); + for (const QJsonValue &value : array) { + const QJsonObject object = value.toObject(); + SavedQuery query; + query.name = object.value(QStringLiteral("name")).toString(); + query.query = object.value(QStringLiteral("query")).toString(); + query.pinned = object.value(QStringLiteral("pinned")).toBool(false); + query.account = object.value(QStringLiteral("account")).toString(); + + if (query.name.isEmpty()) { + addProblem(QStringLiteral("A saved query in %1 has no name and was " + "skipped.").arg(m_queriesPath)); + continue; + } + + for (auto it = object.begin(); it != object.end(); ++it) { + static const QStringList known = { + QStringLiteral("name"), QStringLiteral("query"), + QStringLiteral("pinned"), QStringLiteral("account") + }; + if (!known.contains(it.key())) + query.unknown.insert(it.key(), it.value()); + } + + m_savedQueries.append(query); + } +} + +bool Config::saveSavedQueries() const +{ + if (m_queriesPath.isEmpty() || m_queriesRefused) + return false; + + QJsonArray array; + for (const SavedQuery &query : m_savedQueries) { + QJsonObject object; + object.insert(QStringLiteral("name"), query.name); + object.insert(QStringLiteral("query"), query.query); + if (query.pinned) + object.insert(QStringLiteral("pinned"), true); + if (!query.account.isEmpty()) + object.insert(QStringLiteral("account"), query.account); + for (auto it = query.unknown.begin(); it != query.unknown.end(); ++it) + object.insert(it.key(), it.value()); + array.append(object); + } + + QJsonObject root = m_queriesUnknown; + root.insert(QStringLiteral("version"), kQueriesFormatVersion); + root.insert(QStringLiteral("queries"), array); + + QDir().mkpath(QFileInfo(m_queriesPath).absolutePath()); + + // QSaveFile writes a temporary and renames on commit, so an interrupted + // write cannot leave a half-written file where the queries used to be. + QSaveFile file(m_queriesPath); + if (!file.open(QIODevice::WriteOnly)) + return false; + file.write(QJsonDocument(root).toJson(QJsonDocument::Indented)); + return file.commit(); +} + +QString Config::resolvedQuery(const SavedQuery &query) const +{ + if (query.account.isEmpty()) + return query.query; + + const Account scope = account(query.account); + if (!scope.isValid()) + return query.query; + + return scope.scopedQuery(query.query); +} + SavedQuery Config::startupSavedQuery() const { if (m_savedQueries.isEmpty()) diff --git a/src/config.h b/src/config.h index ea0b055..4211b0c 100644 --- a/src/config.h +++ b/src/config.h @@ -19,12 +19,15 @@ #pragma once #include <QColor> +#include <QJsonObject> #include <QList> #include <QString> #include <QStringList> #include "completionentry.h" +class QSettings; + /// One mail account. notmuch has no concept of accounts; it sees a single flat /// tree. An account is therefore a path prefix within that tree plus an /// identity. @@ -96,10 +99,32 @@ struct Account QString draftsQuery() const; }; +/// A named query, stored in queries.json. +/// +/// Stored as an ORDERED array, which is the whole reason the storage moved out +/// of `[queries]`: QSettings reads a section through childKeys(), which sorts +/// alphabetically and cannot express the order the buttons appear in. struct SavedQuery { QString name; QString query; + + /// Renders as a button in the query row; otherwise it lives in the menu. + /// The two tiers are the point of the flag: a row that shows every saved + /// query does not scale past a handful. + bool pinned = false; + + /// Account KEY, the INI group suffix ("work" from [account.work]), and + /// empty for a query that spans every account. + /// + /// A key rather than a maildir path: the path already lives in the account + /// section, and storing a second copy here would go stale the moment the + /// user edits it. Resolve through Config::resolvedQuery(). + QString account; + + /// Keys this build does not understand, preserved verbatim so a file + /// written by a later version survives a save from this one. + QJsonObject unknown; }; /// Reads ~/.config/qtmaildir/qtmaildir.conf. @@ -117,7 +142,32 @@ public: QList<Account> accounts() const { return m_accounts; } Account account(const QString &key) const; + + /// In document order, which IS the display order. Never sort this. QList<SavedQuery> savedQueries() const { return m_savedQueries; } + void setSavedQueries(const QList<SavedQuery> &queries) + { + m_savedQueries = queries; + } + + /// Path of queries.json, derived from the config file's own directory so a + /// test can point load() at a temporary tree and get both files there. + static QString queriesPath(const QString &configPath); + + /// Writes queries.json. False when the file could not be written, or when + /// the loaded file had a version this build refuses: overwriting a + /// newer-format file with a lossy reading of it is the one outcome worth + /// preventing outright. + bool saveSavedQueries() const; + + /// The query as it should be run: scoped to its account when it names one. + /// + /// Composes through Account::scopedQuery(), whose parentheses are + /// load-bearing. `path:... and a or b` binds as `(path:... and a) or b`, so + /// an unparenthesised disjunction escapes its scope and matches every + /// account. An account key naming nothing returns the bare query rather + /// than a scope built from an empty maildir, which would be path:"/**". + QString resolvedQuery(const SavedQuery &query) const; /// Empty when unset; the caller disables the Sync button in that case. QString syncCommand() const { return m_syncCommand; } @@ -254,8 +304,23 @@ private: /// Records a notice: nothing is wrong, a feature is simply not configured. void addNotice(const QString &message); + /// Reads queries.json, or migrates [queries] when it is absent. Called by + /// load(), which has already parsed the INI by then. + void loadSavedQueries(const QString &configPath, QSettings &settings); + QList<Account> m_accounts; QList<SavedQuery> m_savedQueries; + + /// Where saveSavedQueries() writes, remembered from load(). + QString m_queriesPath; + + /// Top-level keys of queries.json this build does not understand. + QJsonObject m_queriesUnknown; + + /// Set when the file was refused for its version. Blocks the save, so a + /// document from a newer build is never overwritten with less than it held. + bool m_queriesRefused = false; + QString m_syncCommand; QString m_syncLog; int m_toolbarIconSize = 24; diff --git a/tests/test_config.cpp b/tests/test_config.cpp index 95398e6..8024728 100644 --- a/tests/test_config.cpp +++ b/tests/test_config.cpp @@ -19,6 +19,9 @@ #include <QtTest> #include <QTemporaryDir> #include <QSettings> +#include <QJsonArray> +#include <QJsonDocument> +#include <QJsonObject> #include "config.h" #include "mailsync.h" @@ -48,6 +51,17 @@ private slots: void startupQueryDefaultsToUnread(); void startupQueryHonoursTheConfiguredName(); void unknownStartupQueryFallsBackAndReports(); + void savedQueriesKeepTheirDocumentOrder(); + void savedQueryFieldsAreRead(); + void unknownFieldsSurviveARoundTrip(); + void savedQueriesRoundTripUnchanged(); + void migrationWritesJsonAndLeavesTheIniByteIdentical(); + void migrationPinsEveryEntry(); + void jsonWinsOnceItExists(); + void malformedQueriesFileIsAProblemNotACrash(); + void futureVersionIsRefusedAndReported(); + void startupQueryFallsBackToDocumentOrder(); + void scopedSavedQueryParenthesisesADisjunction(); void generalSectionKeysAreActuallyRead(); void messageZoomDefaultsAndValidates(); void messageZoomOutOfRangeIsReported(); @@ -139,10 +153,10 @@ void TestConfig::parsesSavedQueries() const QList<SavedQuery> queries = config.savedQueries(); QCOMPARE(queries.size(), 2); - // QSettings::childKeys() returns keys alphabetically, not in file order, - // so the UI button order is alphabetical. This assertion happens to hold - // either way since "Inbox" < "Unread", but the ordering guarantee is - // alphabetical, not "follows the file". + // Reached through migration since item 23: [queries] is read once, when + // queries.json is absent, and childKeys() is alphabetical, so the migrated + // order is too. From then on the JSON's own order wins, which is what + // savedQueriesKeepTheirDocumentOrder() covers. QCOMPARE(queries.at(0).name, QStringLiteral("Inbox")); QCOMPARE(queries.at(0).query, QStringLiteral("tag:inbox")); } @@ -1093,5 +1107,314 @@ void TestConfig::allDraftsQueryIsIndependentOfSent() QVERIFY(!sent.contains(QStringLiteral("provider-b"))); } +// --------------------------------------------------------------------------- +// queries.json (item 23) +// --------------------------------------------------------------------------- + +static QString writeQueries(const QTemporaryDir &dir, const QString &body) +{ + const QString path = dir.filePath(QStringLiteral("queries.json")); + QFile f(path); + f.open(QIODevice::WriteOnly); + f.write(body.toUtf8()); + f.close(); + return path; +} + +/// The property the INI could not provide. "Zebra" is written first and must +/// STAY first: alphabetical order would put it last, so this fails against any +/// implementation that sorts, including the one being replaced. +void TestConfig::savedQueriesKeepTheirDocumentOrder() +{ + QTemporaryDir dir; + const QString path = writeIni(dir, QString()); + writeQueries(dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Zebra", "query": "tag:zebra" }, + { "name": "Apple", "query": "tag:apple" }, + { "name": "Middle", "query": "tag:middle" } + ] + })")); + + Config config; + config.load(path); + + const QList<SavedQuery> queries = config.savedQueries(); + QCOMPARE(queries.size(), 3); + QCOMPARE(queries.at(0).name, QStringLiteral("Zebra")); + QCOMPARE(queries.at(1).name, QStringLiteral("Apple")); + QCOMPARE(queries.at(2).name, QStringLiteral("Middle")); +} + +void TestConfig::savedQueryFieldsAreRead() +{ + QTemporaryDir dir; + const QString path = writeIni(dir, QString()); + writeQueries(dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Inbox", "query": "tag:inbox", "pinned": true }, + { "name": "Billing", "query": "from:billing", "account": "work" } + ] + })")); + + Config config; + config.load(path); + + const QList<SavedQuery> queries = config.savedQueries(); + QCOMPARE(queries.size(), 2); + + QCOMPARE(queries.at(0).name, QStringLiteral("Inbox")); + QCOMPARE(queries.at(0).query, QStringLiteral("tag:inbox")); + QVERIFY(queries.at(0).pinned); + QVERIFY(queries.at(0).account.isEmpty()); + + // pinned defaults to false, which is what puts a query in the menu rather + // than on the row. + QVERIFY(!queries.at(1).pinned); + QCOMPARE(queries.at(1).account, QStringLiteral("work")); +} + +/// A field written by a later build must survive an older build's save, or a +/// downgrade silently strips config the user set. +void TestConfig::unknownFieldsSurviveARoundTrip() +{ + QTemporaryDir dir; + const QString path = writeIni(dir, QString()); + const QString queriesPath = writeQueries(dir, QStringLiteral(R"({ + "version": 1, + "colour_scheme": "solarized", + "queries": [ + { "name": "Inbox", "query": "tag:inbox", "icon": "mail-inbox" } + ] + })")); + + Config config; + config.load(path); + QVERIFY(config.saveSavedQueries()); + + QFile f(queriesPath); + QVERIFY(f.open(QIODevice::ReadOnly)); + const QJsonObject root = QJsonDocument::fromJson(f.readAll()).object(); + f.close(); + + QCOMPARE(root.value(QStringLiteral("colour_scheme")).toString(), + QStringLiteral("solarized")); + const QJsonObject entry = + root.value(QStringLiteral("queries")).toArray().at(0).toObject(); + QCOMPARE(entry.value(QStringLiteral("icon")).toString(), + QStringLiteral("mail-inbox")); +} + +void TestConfig::savedQueriesRoundTripUnchanged() +{ + QTemporaryDir dir; + const QString path = writeIni(dir, QString()); + writeQueries(dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Zebra", "query": "tag:zebra", "pinned": true }, + { "name": "Apple", "query": "from:a@example.org", "account": "work" } + ] + })")); + + Config first; + first.load(path); + QVERIFY(first.saveSavedQueries()); + + Config second; + second.load(path); + + const QList<SavedQuery> a = first.savedQueries(); + const QList<SavedQuery> b = second.savedQueries(); + QCOMPARE(b.size(), a.size()); + for (int i = 0; i < a.size(); ++i) { + QCOMPARE(b.at(i).name, a.at(i).name); + QCOMPARE(b.at(i).query, a.at(i).query); + QCOMPARE(b.at(i).pinned, a.at(i).pinned); + QCOMPARE(b.at(i).account, a.at(i).account); + } +} + +/// Asserts the INI is byte-identical, NOT that it still parses. Re-reading it +/// through QSettings would pass against a rewrite that kept every value while +/// dropping the comments and key order, which is the loss this design exists +/// to avoid. +void TestConfig::migrationWritesJsonAndLeavesTheIniByteIdentical() +{ + QTemporaryDir dir; + const QString ini = QStringLiteral( + "; a comment the user wrote and expects to keep\n" + "[queries]\n" + "Unread=tag:unread\n" + "Inbox=tag:inbox\n" + "\n" + "[general]\n" + "startup_query=Unread\n" + ); + const QString path = writeIni(dir, ini); + + QFile before(path); + QVERIFY(before.open(QIODevice::ReadOnly)); + const QByteArray originalBytes = before.readAll(); + before.close(); + + Config config; + config.load(path); + + const QString queriesPath = dir.filePath(QStringLiteral("queries.json")); + QVERIFY2(QFile::exists(queriesPath), "migration did not write queries.json"); + + QFile after(path); + QVERIFY(after.open(QIODevice::ReadOnly)); + const QByteArray afterBytes = after.readAll(); + after.close(); + + QCOMPARE(afterBytes, originalBytes); + + // The [queries] section is left in place, so an older build still works. + QVERIFY(afterBytes.contains("[queries]")); + QVERIFY(afterBytes.contains("; a comment the user wrote")); +} + +/// A migrated query that was not pinned would vanish from the query row, which +/// on the first launch after an upgrade looks like data loss. +void TestConfig::migrationPinsEveryEntry() +{ + QTemporaryDir dir; + const QString path = writeIni(dir, QStringLiteral( + "[queries]\n" + "Inbox=tag:inbox\n" + "Unread=tag:unread\n" + )); + + Config config; + config.load(path); + + const QList<SavedQuery> queries = config.savedQueries(); + QCOMPARE(queries.size(), 2); + for (const SavedQuery &query : queries) + QVERIFY2(query.pinned, qPrintable( + QStringLiteral("migrated query '%1' is not pinned").arg(query.name))); +} + +/// Once the JSON exists, [queries] is dead. Two sources of truth was the +/// option this design rejected. +void TestConfig::jsonWinsOnceItExists() +{ + QTemporaryDir dir; + const QString path = writeIni(dir, QStringLiteral( + "[queries]\n" + "FromTheIni=tag:ini\n" + )); + writeQueries(dir, QStringLiteral(R"({ + "version": 1, + "queries": [ { "name": "FromTheJson", "query": "tag:json" } ] + })")); + + Config config; + config.load(path); + + const QList<SavedQuery> queries = config.savedQueries(); + QCOMPARE(queries.size(), 1); + QCOMPARE(queries.at(0).name, QStringLiteral("FromTheJson")); +} + +void TestConfig::malformedQueriesFileIsAProblemNotACrash() +{ + QTemporaryDir dir; + const QString path = writeIni(dir, QString()); + writeQueries(dir, QStringLiteral("{ this is not json at all")); + + Config config; + config.load(path); + + QVERIFY(config.savedQueries().isEmpty()); + QVERIFY2(!config.problems().isEmpty(), + "a malformed queries.json must be reported"); +} + +/// Refusing an unknown version is the same contract rules.json keeps: a file +/// from a newer build is not silently reinterpreted, and above all is not +/// overwritten with a lossy reading of itself. +void TestConfig::futureVersionIsRefusedAndReported() +{ + QTemporaryDir dir; + const QString path = writeIni(dir, QString()); + writeQueries(dir, QStringLiteral(R"({ + "version": 99, + "queries": [ { "name": "Inbox", "query": "tag:inbox" } ] + })")); + + Config config; + config.load(path); + + QVERIFY(config.savedQueries().isEmpty()); + QVERIFY(!config.problems().isEmpty()); +} + +/// The fallback stops meaning "alphabetically first" and starts meaning "first +/// in the user's own order". "Zebra" first proves it: alphabetical would pick +/// "Apple". +void TestConfig::startupQueryFallsBackToDocumentOrder() +{ + QTemporaryDir dir; + const QString path = writeIni(dir, QStringLiteral( + "[general]\n" + "startup_query=NoSuchQuery\n" + )); + writeQueries(dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Zebra", "query": "tag:zebra" }, + { "name": "Apple", "query": "tag:apple" } + ] + })")); + + Config config; + config.load(path); + + QCOMPARE(config.startupSavedQuery().name, QStringLiteral("Zebra")); +} + +/// The parentheses are load-bearing. Without them `path:... and a or b` binds +/// as `(path:... and a) or b`, so a query saved with a disjunction escapes its +/// account scope and matches every account. +void TestConfig::scopedSavedQueryParenthesisesADisjunction() +{ + QTemporaryDir dir; + const QString path = writeIni(dir, QStringLiteral( + "[account.work]\n" + "name=Test User\n" + "address=user@example.org\n" + "maildir=work-mail\n" + )); + writeQueries(dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Either", + "query": "from:a@example.org or from:b@example.org", + "account": "work" } + ] + })")); + + Config config; + config.load(path); + + const QString scoped = config.resolvedQuery(config.savedQueries().at(0)); + QCOMPARE(scoped, QStringLiteral( + "path:\"work-mail/**\" and " + "(from:a@example.org or from:b@example.org)")); + + // An account key naming nothing resolves to the bare query rather than a + // scope built from an empty maildir, which would be path:"/**". + SavedQuery orphan; + orphan.name = QStringLiteral("Orphan"); + orphan.query = QStringLiteral("tag:inbox"); + orphan.account = QStringLiteral("deleted-account"); + QCOMPARE(config.resolvedQuery(orphan), QStringLiteral("tag:inbox")); +} + QTEST_MAIN(TestConfig) #include "test_config.moc" |