Merge branch 'saved-queries': saved queries in a file of their own

Items 23 and 82. Saved queries move out of the [queries] INI section into
~/.config/qtmaildir/queries.json, gaining the three things the INI could not
express: an order, a pinned flag, and a per-query account scope. They can now be
created, edited, reordered, unpinned and deleted from the UI rather than only by
hand-editing a config file.

The INI could not carry order at all: QSettings reads a section through
childKeys(), which sorts alphabetically and never follows the file, so the
buttons could not be arranged. Migration reads the old section once, marks every
entry pinned so nothing moves on the first launch, and leaves the config file
byte-identical, since rewriting it with QSettings would drop the user's comments
and key order.

Sent stops being a hardcoded button and becomes an ordinary entry carrying
"generated": "sent". Its query is still composed from the accounts every time
it runs, so correcting a folder name still needs no edit here, but the row now
follows one rule instead of having one member the user could not move, rename or
remove.

Item 82 was found by hand-testing item 23 and fixed on the same branch: saving
worked and nothing else did, so a saved query could be created and never
changed. It also uncovered a defect that predated it, where rebuilding the row
with deleteLater() left the stale row answering findChild(), which was already
reachable from the save path.

Hand-tested throughout: the migration against the real config, saving, unpinning,
deleting, reordering, the read-only query field on a generated entry, and an
unpin surviving a restart.

1 files changed, 533 insertions, 4 deletions

diff --git a/tests/test_config.cpp b/tests/test_config.cpp
index 95398e6..3b094ba 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,23 @@ 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 aGeneratedQueryResolvesFromTheAccounts();
+    void aGeneratedQueryTracksAConfigChange();
+    void anUnknownGeneratorResolvesToNothingAndReports();
+    void migrationAddsSentWhenAnAccountHasOne();
+    void migrationAddsNoSentWithoutTheKey();
+    void aGeneratedEntryWritesNoRedundantKeys();
     void generalSectionKeysAreActuallyRead();
     void messageZoomDefaultsAndValidates();
     void messageZoomOutOfRangeIsReported();
@@ -139,10 +159,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 +1113,514 @@ 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"));
+}
+
+// ---------------------------------------------------------------------------
+// Generated saved queries
+// ---------------------------------------------------------------------------
+
+static QString twoAccountsWithSent()
+{
+    return QStringLiteral(
+        "[account.work]\n"
+        "name=Test User\n"
+        "address=user@example.org\n"
+        "maildir=work-mail\n"
+        "sent=Sent\n"
+        "\n"
+        "[account.personal]\n"
+        "name=Test User\n"
+        "address=me@example.net\n"
+        "maildir=personal\n"
+        "sent=[Provider]/Posta inviata\n"
+    );
+}
+
+void TestConfig::aGeneratedQueryResolvesFromTheAccounts()
+{
+    QTemporaryDir dir;
+    const QString path = writeIni(dir, twoAccountsWithSent());
+    writeQueries(dir, QStringLiteral(R"({
+        "version": 1,
+        "queries": [
+            { "name": "Sent", "generated": "sent", "pinned": true }
+        ]
+    })"));
+
+    Config config;
+    config.load(path);
+
+    const SavedQuery sent = config.savedQueries().at(0);
+    QVERIFY(sent.isGenerated());
+    // The stored query is empty; the text comes from the accounts.
+    QVERIFY(sent.query.isEmpty());
+    QCOMPARE(config.resolvedQuery(sent), config.allSentQuery());
+    QVERIFY(config.resolvedQuery(sent).contains(
+        QStringLiteral("path:\"work-mail/Sent/**\"")));
+    // The quotes matter: "[" and "]" are Xapian syntax and an unquoted term
+    // is parsed rather than matched.
+    QVERIFY(config.resolvedQuery(sent).contains(
+        QStringLiteral("path:\"personal/[Provider]/Posta inviata/**\"")));
+
+    // Flat, not threaded: a sent view lists messages, and that property has to
+    // travel with the entry or it is lost the moment Sent is a stored row.
+    QVERIFY(sent.flat);
+}
+
+/// The whole reason Sent is generated rather than stored. A stored copy would
+/// keep naming an account that has been renamed or a folder that has moved.
+void TestConfig::aGeneratedQueryTracksAConfigChange()
+{
+    QTemporaryDir dir;
+    const QString queries = QStringLiteral(R"({
+        "version": 1,
+        "queries": [ { "name": "Sent", "generated": "sent" } ]
+    })");
+
+    const QString before = writeIni(dir, twoAccountsWithSent());
+    writeQueries(dir, queries);
+    Config first;
+    first.load(before);
+    const QString firstResolved = first.resolvedQuery(first.savedQueries().at(0));
+
+    // The user corrects a folder name. Nothing in queries.json changes.
+    QTemporaryDir second;
+    const QString after = writeIni(second, QStringLiteral(
+        "[account.work]\n"
+        "name=Test User\n"
+        "address=user@example.org\n"
+        "maildir=work-mail\n"
+        "sent=Sent Items\n"
+    ));
+    writeQueries(second, queries);
+    Config later;
+    later.load(after);
+    const QString laterResolved = later.resolvedQuery(later.savedQueries().at(0));
+
+    QVERIFY(firstResolved != laterResolved);
+    QVERIFY(laterResolved.contains(QStringLiteral("Sent Items")));
+}
+
+void TestConfig::anUnknownGeneratorResolvesToNothingAndReports()
+{
+    QTemporaryDir dir;
+    const QString path = writeIni(dir, twoAccountsWithSent());
+    writeQueries(dir, QStringLiteral(R"({
+        "version": 1,
+        "queries": [ { "name": "Future", "generated": "not_a_generator" } ]
+    })"));
+
+    Config config;
+    config.load(path);
+
+    // Kept rather than dropped: a later build may know this generator, and
+    // silently deleting the row on save would lose it.
+    QCOMPARE(config.savedQueries().size(), 1);
+    QVERIFY(config.resolvedQuery(config.savedQueries().at(0)).isEmpty());
+    QVERIFY2(!config.problems().isEmpty(),
+             "an unknown generator must be reported, not silently inert");
+}
+
+void TestConfig::migrationAddsSentWhenAnAccountHasOne()
+{
+    QTemporaryDir dir;
+    const QString path = writeIni(dir, twoAccountsWithSent()
+                                       + QStringLiteral(
+        "\n[queries]\n"
+        "Inbox=tag:inbox\n"
+    ));
+
+    Config config;
+    config.load(path);
+
+    const QList<SavedQuery> queries = config.savedQueries();
+    QCOMPARE(queries.size(), 2);
+    // Last, where the button already sat: after the saved queries.
+    QCOMPARE(queries.at(1).name, QStringLiteral("Sent"));
+    QVERIFY(queries.at(1).isGenerated());
+    QVERIFY(queries.at(1).pinned);
+    QVERIFY(queries.at(1).flat);
+}
+
+/// Today the button is hidden entirely when no account configures a sent
+/// folder, rather than offering one that always finds nothing. The migration
+/// must not invent a row that would do exactly that.
+void TestConfig::migrationAddsNoSentWithoutTheKey()
+{
+    QTemporaryDir dir;
+    const QString path = writeIni(dir, QStringLiteral(
+        "[account.work]\n"
+        "name=Test User\n"
+        "address=user@example.org\n"
+        "maildir=work-mail\n"
+        "\n"
+        "[queries]\n"
+        "Inbox=tag:inbox\n"
+    ));
+
+    Config config;
+    config.load(path);
+
+    const QList<SavedQuery> queries = config.savedQueries();
+    QCOMPARE(queries.size(), 1);
+    QCOMPARE(queries.at(0).name, QStringLiteral("Inbox"));
+}
+
+/// The file is meant to be hand-edited, so a key that carries no information
+/// is a key the reader has to skip past. `query` says nothing on a generated
+/// entry, and `flat` is implied by the sent generator.
+void TestConfig::aGeneratedEntryWritesNoRedundantKeys()
+{
+    QTemporaryDir dir;
+    const QString path = writeIni(dir, twoAccountsWithSent());
+    const QString queriesPath = writeQueries(dir, QStringLiteral(R"({
+        "version": 1,
+        "queries": [
+            { "name": "Sent", "generated": "sent", "pinned": true },
+            { "name": "Inbox", "query": "tag:inbox", "pinned": true }
+        ]
+    })"));
+
+    Config config;
+    config.load(path);
+    QVERIFY(config.saveSavedQueries());
+
+    QFile f(queriesPath);
+    QVERIFY(f.open(QIODevice::ReadOnly));
+    const QJsonArray array = QJsonDocument::fromJson(f.readAll())
+                                 .object()
+                                 .value(QStringLiteral("queries"))
+                                 .toArray();
+    f.close();
+
+    const QJsonObject sent = array.at(0).toObject();
+    QCOMPARE(sent.value(QStringLiteral("generated")).toString(),
+             QStringLiteral("sent"));
+    QVERIFY2(!sent.contains(QStringLiteral("query")),
+             "a generated entry has no query of its own to store");
+    QVERIFY2(!sent.contains(QStringLiteral("flat")),
+             "the sent generator implies flat; storing it says nothing");
+
+    // The ordinary entry is untouched by any of that.
+    const QJsonObject inbox = array.at(1).toObject();
+    QCOMPARE(inbox.value(QStringLiteral("query")).toString(),
+             QStringLiteral("tag:inbox"));
+
+    // And it all still reads back the same.
+    Config reloaded;
+    reloaded.load(path);
+    QCOMPARE(reloaded.savedQueries().size(), 2);
+    QVERIFY(reloaded.savedQueries().at(0).isGenerated());
+    QVERIFY2(reloaded.savedQueries().at(0).flat,
+             "flat must come back from the generator, not from the file");
+}
+
 QTEST_MAIN(TestConfig)
 #include "test_config.moc"