From 5e30d1805656895387ba83865d9635caf2e51618 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 19:06:41 +0200 Subject: feat(queries): store saved queries in queries.json First half of item 23. The storage moves out of the [queries] INI section into ~/.config/qtmaildir/queries.json; the UI that writes it comes next. The INI could not express order. QSettings reads a section through childKeys(), which sorts alphabetically and never follows the file, so the saved-query buttons could not be arranged and config.cpp carried a comment saying a hand-rolled parser would be needed to change that. queries.json is an ordered array and nothing sorts it on load. That also makes room for the two fields the save dialog needs: pinned, which decides whether a query is a button or a menu entry, and account, which scopes it. account stores the account KEY, not the maildir path, so it does not duplicate config that already lives in the account section and go stale when the user edits it. Config::resolvedQuery() 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. A key naming an account that no longer exists resolves to the bare query rather than a scope built from an empty maildir, which would be path:"/**" and match everything. Migration reads [queries] once, when queries.json is absent, marks every entry pinned so the query row does not empty on the first launch after an upgrade, and leaves the INI section untouched. Stripping it would mean rewriting a hand-edited file with QSettings, which drops comments and key order across the whole file. The format follows rules.json in shape only: a version and unknown fields preserved at both levels, so a file written by a later build survives a save from this one. None of its two-implementation machinery is here, because queries have exactly one reader; the version constant says so where a future reader will look. A file whose version this build does not know is refused AND blocks the save, so a newer document is never overwritten with a lossy reading of itself. Twelve tests, each checked against a mutation that puts the corresponding bug back: sorting on load fails three of them, stripping the INI section after migration fails the byte-identical assertion, concatenating the scope without parentheses fails the disjunction test, and dropping unknown-field preservation fails the round trip. The migration test compares the INI file's BYTES rather than re-reading it through QSettings, which would have passed against a rewrite that kept every value while dropping the comments. startup_query still resolves by name, but its fallback now returns the first entry in the user's own order rather than the alphabetically first one. That is user-visible for a config whose startup_query matches nothing. Co-Authored-By: Claude Opus 5 --- src/config.cpp | 185 ++++++++++++++++++++++++++-- src/config.h | 65 ++++++++++ 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 +#include +#include #include +#include +#include +#include #include +#include #include #include @@ -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 +#include #include #include #include #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 accounts() const { return m_accounts; } Account account(const QString &key) const; + + /// In document order, which IS the display order. Never sort this. QList savedQueries() const { return m_savedQueries; } + void setSavedQueries(const QList &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 m_accounts; QList 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 #include #include +#include +#include +#include #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 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 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 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 a = first.savedQueries(); + const QList 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 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 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" -- cgit v1.2.3 From 0c5eea8f0d0ccc5b8eb6220814c9e212d6c1ccc2 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 19:19:58 +0200 Subject: feat(queries): save a query from the UI, and split the buttons off the query row Second half of item 23, on top of the storage change. A query can now be kept without hand-editing a file, and the row of buttons no longer grows without bound. Ctrl+S opens a dialog on whatever is in the query bar, taking a name, an optional account scope and whether the query is pinned. It preselects the account already chosen in the dropdown, since that is the scope the user is looking at, and it says so when a name is about to replace an existing query rather than refusing the name: overwriting a saved query on purpose is a normal edit, and the only thing worth preventing is doing it without noticing. Saving over an entry keeps the stored entry's unknown fields rather than the dialog's fresh value, so a field written by a later build survives being edited here. The saved queries move to a row of their own beneath the query bar, pinned ones as buttons and the rest behind a More queries menu that only exists when something is in it. The ponytail note that stood in the query row predicted exactly this: an unbounded list of buttons sharing the row squeezed the field. Sent moves down with them and is still not a saved query, for the reason already recorded there. A saved query's account scope goes through the account DROPDOWN rather than being baked into the query text. runQuery() already wraps the query in the selected account's path, so pre-scoping here would apply it twice, and setting the dropdown also shows the user which scope they are in. An unscoped query clears the selection rather than inheriting whatever the last one left, which is the same defect the rules preview had. Seven tests, three mutations. Ignoring the pinned flag fails two of them, pre-scoping the text instead of setting the dropdown fails two, and letting an unscoped query inherit the previous account fails one. The menu-absence test initially passed against no implementation at all, since it only asserted a widget was missing; it now proves the row was populated first, which is the guard that class of test needs. Two existing invariants caught real omissions rather than needing adjustment: every registered action must appear in KeyMap::knownActions(), which is what gives it a configurable binding, and every action needs its own icon. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 37 +++++++ README.md | 58 +++++++--- src/CMakeLists.txt | 1 + src/keymap.cpp | 5 + src/mainwindow.cpp | 219 +++++++++++++++++++++++++++++------- src/mainwindow.h | 21 ++++ src/savequerydialog.cpp | 128 +++++++++++++++++++++ src/savequerydialog.h | 63 +++++++++++ tests/test_mainwindow.cpp | 275 ++++++++++++++++++++++++++++++++++++++++++++++ 9 files changed, 755 insertions(+), 52 deletions(-) create mode 100644 src/savequerydialog.cpp create mode 100644 src/savequerydialog.h diff --git a/CHANGELOG.md b/CHANGELOG.md index e867f02..2d264e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,43 @@ point at which they are stable. ## [Unreleased] +### Added + +- A **Save query** action (`Ctrl+S`) keeps the query in the bar as a saved + query, naming it, optionally scoping it to one account, and choosing whether + it appears as a button or in a menu. Saved queries no longer have to be added + by hand-editing the config file (item 23). +- Saved queries live in `~/.config/qtmaildir/queries.json`, which carries their + **order**, a `pinned` flag and an optional account scope. The order in the + file is the order the buttons appear in, so rearranging them is a matter of + moving lines. + +### Changed + +- Saved queries have a **row of their own** beneath the query bar rather than + sharing it, and the unpinned ones sit behind a **More queries** menu, so the + query field is no longer squeezed by a long list of buttons. +- Saved-query buttons follow the file's order instead of appearing + alphabetically. + +### Upgrading + +Saved queries move out of the `[queries]` section of `qtmaildir.conf` and into +`~/.config/qtmaildir/queries.json`. **The first launch migrates them for you**: +the section is read, the JSON file is written from it, and every entry is +marked pinned so your buttons stay where they were. + +Your config file is left byte-for-byte alone. The old `[queries]` section stays +in it, ignored from then on, and can be deleted by hand whenever you like. It is +not removed automatically because rewriting the file would discard your comments +and reorder your keys. + +One behaviour changes with the move. Buttons used to appear in alphabetical +order and now follow the file. If `[general] startup_query` names a query that +does not exist, the fallback is likewise the first query in the file rather than +the alphabetically first one, so a config that relied on that fallback may open +on a different query than before. + ## [0.17.0] - 2026-08-13 A rule is built from rows now instead of typed into four free-text fields: diff --git a/README.md b/README.md index 0921791..81ec7d8 100644 --- a/README.md +++ b/README.md @@ -106,7 +106,7 @@ identity. ; point: once you zoom with Ctrl+wheel or Ctrl+/Ctrl-, that is remembered ; separately and this value no longer applies. ; message_zoom = 1.0 -; Optional. Which [queries] entry to open at startup, by name. Defaults to +; Optional. Which saved query to open at startup, by name. Defaults to ; Unread. Falls back to the first saved query if no query by this name ; exists, and warns if you named one explicitly. ; startup_query = Unread @@ -191,11 +191,6 @@ shopping = #3366cc ; also colours shopping/amazon, shopping/nike, ... shopping/amazon = #ff9900 ; ... unless the exact tag overrides it work = #cc4444 -[queries] -Inbox = tag:inbox -Unread = tag:unread -Important = tag:flagged - [keys] Ctrl+E = archive Ctrl+D = delete @@ -203,14 +198,47 @@ j = next_thread k = prev_thread ``` -Saved-query buttons appear in alphabetical order rather than file order: -QSettings returns keys sorted, and preserving file order would mean -hand-rolling an INI parser. Which query opens at startup is therefore a -separate setting, `[general] startup_query`, rather than "the first one". +### Saved queries + +Saved queries live in `~/.config/qtmaildir/queries.json`, not in the config +file. They are written by the **Save query** action (`Ctrl+S`), which names the +query in the bar, optionally scopes it to one account, and chooses whether it +gets a button: + +```json +{ + "version": 1, + "queries": [ + { "name": "Inbox", "query": "tag:inbox", "pinned": true }, + { "name": "Unread", "query": "tag:unread", "pinned": true }, + { "name": "Billing", "query": "from:billing", "account": "work" } + ] +} +``` -The button text is the key you write here, so these names are yours to -choose. `Important = tag:flagged` and `Flagged = tag:flagged` run the same -query and differ only in what the button says. +The order in the file is the order the buttons appear in, so rearranging them +is a matter of moving lines. `pinned` decides between a button and the **More +queries** menu, which keeps the row usable once you have more than a handful. +`account` names an `[account.]` section and scopes the query to it, the +same as choosing that account in the dropdown; leave it out for a query that +spans every account. + +The name is what the button says, so `Important` and `Flagged` can run the same +query and differ only in the label. + +**Upgrading from 0.17.0 or earlier.** Saved queries used to live in a +`[queries]` section of `qtmaildir.conf`. The first launch after upgrading reads +that section, writes `queries.json` from it, and marks every entry pinned so +your buttons stay where they were. Your config file is not modified: the old +`[queries]` section is left exactly as it is, ignored from then on, and you can +delete it by hand whenever you like. The reason it is not removed for you is +that rewriting the file would drop your comments and reorder your keys. + +One behaviour changes with the move. Buttons used to appear in alphabetical +order, because the INI backend returns keys sorted and preserving file order +would have meant hand-rolling a parser. They now follow the file. If +`startup_query` names a query that does not exist, the fallback is likewise the +first query in the file rather than the alphabetically first one. ### Sent mail @@ -253,7 +281,8 @@ behaves like any other query, threads and all. ## The query bar The bar at the top takes a notmuch query and shows the matching threads. -Saved queries from `[queries]` sit beside it as buttons. +Saved queries sit on their own row beneath it: the pinned ones as buttons, the +rest behind **More queries**. `Ctrl+S` keeps the current query as a new one. Completion helps with the syntax rather than replacing it. `Ctrl+Space` opens the popup, and ordinary typing keeps it up to date. Candidates carry a @@ -478,6 +507,7 @@ Defaults, all rebindable through `[keys]`: | `Ctrl+I` | `flag` | Mark important (adds `flagged`) | | `Ctrl+L` | `focus_query` | Focus and select the query bar | | `Ctrl+Space` | `complete_query` | Focus the query bar and offer completions | +| `Ctrl+S` | `save_query` | Keep the current query as a saved query | | `Ctrl+H` | `toggle_html` | Switch the thread between HTML and plain text | | `Ctrl+M` | `load_remote` | Load remote images for the current thread | | `Ctrl+T` | `edit_tags` | Add or remove any tag on the selected threads | diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 6ae157b..0945f65 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -11,6 +11,7 @@ add_library(qtmaildir_lib STATIC notmuchworker.cpp tagchip.cpp tagcolors.cpp + savequerydialog.cpp tagdialog.cpp tagrules.cpp tagrulesdialog.cpp diff --git a/src/keymap.cpp b/src/keymap.cpp index 71c5355..52b015f 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -39,6 +39,7 @@ QStringList KeyMap::knownActions() QStringLiteral("flag"), QStringLiteral("focus_query"), QStringLiteral("complete_query"), + QStringLiteral("save_query"), QStringLiteral("select_all"), QStringLiteral("clear_pane"), QStringLiteral("clear_selection"), @@ -100,6 +101,10 @@ QList> KeyMap::defaultBindings() // shells and editors, and it is a named key rather than a symbol, so // no layout has to shift it. { QStringLiteral("Ctrl+Space"), QStringLiteral("complete_query") }, + // The conventional save key, and free here: nothing in this window + // saves a document, so Ctrl+S is unclaimed and means what a user + // expects it to. + { QStringLiteral("Ctrl+S"), QStringLiteral("save_query") }, // The conventional select-all key, and free here: the thread list is a // read-only view, so nothing else in the window wants it. { QStringLiteral("Ctrl+A"), QStringLiteral("select_all") }, diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index e2df6de..3966e9f 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -56,6 +56,7 @@ #include "cardlayout.h" #include "tagchip.h" #include "tagdialog.h" +#include "savequerydialog.h" #include "tagrulesdialog.h" #include "threadlistmodel.h" #include "threadlistview.h" @@ -339,6 +340,21 @@ MainWindow::MainWindow(const Config &config, QWidget *parent) buildUi(); registerActions(); + + // After registerActions(), not inside buildUi(): the query bar exists by + // then but the action does not, so wiring this where the field is built + // silently connected nothing and left Save query enabled on an empty + // query. Hung on textChanged rather than textEdited, because the field is + // also set programmatically, by the saved-query buttons and by + // recoverStaleThread(), and the action must track those too. + if (QAction *save = m_actions.value(QStringLiteral("save_query"))) { + auto updateSaveState = [this, save]() { + save->setEnabled(!m_queryEdit->text().trimmed().isEmpty()); + }; + connect(m_queryEdit, &QLineEdit::textChanged, this, updateSaveState); + updateSaveState(); + } + buildMenus(); // After buildMenus(): QMainWindow::restoreState() matches toolbars by // object name, so they must already exist or their position is dropped. @@ -543,49 +559,17 @@ void MainWindow::buildUi() this, &MainWindow::onExternalSyncStateChanged); m_syncMonitor->start(); - // One row: the account dropdown, the query field, then the saved queries. - // The field is the only stretching item, so it is framed on both sides - // rather than running flush to the window edge, which is what the removed - // Sync button used to terminate. - // - // ponytail: no overflow handling. [queries] is unbounded and enough entries - // would squeeze the field, but three is the real-world case today. Item 23 - // already specifies buttons-plus-menu and is where that belongs. + // The query row proper: account, sort order, the field. The saved queries + // used to share it and now have a row of their own below, which is what + // stops an unbounded list squeezing the field (item 23; the ponytail note + // that stood here predicted exactly this). queryRow->addWidget(m_accountBox); queryRow->addWidget(m_sortOrder); queryRow->addWidget(m_queryEdit, 1); - for (const SavedQuery &saved : m_config.savedQueries()) { - auto *button = new QPushButton(saved.name, central); - connect(button, &QPushButton::clicked, this, [this, saved]() { - m_queryEdit->setText(saved.query); - runCurrentQuery(); - }); - queryRow->addWidget(button); - } - - // Sent sits with the saved queries and is not one: its query is COMPOSED - // from the accounts' `sent` keys at click time, so adding an account or - // correcting a folder name is a config edit and nothing else. A [queries] - // entry holding the same string would go stale silently, and could not - // narrow to the selected account the way this does through - // runCurrentQuery()'s existing scope wrap. - // - // Hidden entirely when no account configures a sent folder, rather than - // offering a button that always finds nothing. - if (!m_config.allSentQuery().isEmpty()) { - auto *sentButton = new QPushButton(tr("Sent"), central); - sentButton->setObjectName(QStringLiteral("sentButton")); - connect(sentButton, &QPushButton::clicked, this, [this]() { - m_queryEdit->setText(m_config.allSentQuery()); - // Flat for this query only. runCurrentQuery() clears it again for - // anything else, including the same query typed by hand, so the - // flag cannot outlive the button that set it. - runQuery(FlatResult::Yes); - }); - queryRow->addWidget(sentButton); - } layout->addLayout(queryRow); + buildSavedQueryRow(central, layout); + // Thread list and message pane. m_model = new ThreadListModel(this); m_model->setTagColors(&m_tagColors); @@ -836,6 +820,10 @@ void MainWindow::registerActions() tr("Edit the rules that tag mail as it arrives"), [this]() { showTagRulesDialog(); }); + addAction(QStringLiteral("save_query"), tr("&Save query..."), + tr("Keep the current query as a saved query"), [this]() { + saveCurrentQuery(); + }); addAction(QStringLiteral("toggle_html"), tr("Toggle &HTML"), tr("Switch the thread between HTML and plain text"), [this]() { m_messageView->toggleHtml(); @@ -982,6 +970,7 @@ void MainWindow::buildMenus() editMenu->addSeparator(); editMenu->addAction(m_actions.value(QStringLiteral("focus_query"))); editMenu->addAction(m_actions.value(QStringLiteral("complete_query"))); + editMenu->addAction(m_actions.value(QStringLiteral("save_query"))); editMenu->addSeparator(); editMenu->addAction(m_actions.value(QStringLiteral("select_all"))); @@ -1060,6 +1049,7 @@ void MainWindow::buildMenus() // the selection's tags. { QStringLiteral("tag_rules"), QStringLiteral("configure") }, { QStringLiteral("complete_query"), QStringLiteral("edit-find-replace") }, + { QStringLiteral("save_query"), QStringLiteral("document-save") }, { QStringLiteral("select_all"), QStringLiteral("edit-select-all") }, { QStringLiteral("clear_pane"), QStringLiteral("edit-clear") }, { QStringLiteral("clear_selection"), QStringLiteral("edit-clear-all") }, @@ -1644,6 +1634,159 @@ void MainWindow::showWarnings() problems.join(QLatin1Char('\n'))); } +void MainWindow::buildSavedQueryRow(QWidget *parent, QVBoxLayout *layout) +{ + auto *row = new QWidget(parent); + row->setObjectName(QStringLiteral("savedQueryRow")); + auto *box = new QHBoxLayout(row); + box->setContentsMargins(0, 0, 0, 0); + + QList unpinned; + for (const SavedQuery &saved : m_config.savedQueries()) { + if (!saved.pinned) { + unpinned.append(saved); + continue; + } + auto *button = new QPushButton(saved.name, row); + connect(button, &QPushButton::clicked, this, + [this, saved]() { runSavedQuery(saved); }); + box->addWidget(button); + } + + // Sent sits with the saved queries and is not one: its query is COMPOSED + // from the accounts' `sent` keys at click time, so adding an account or + // correcting a folder name is a config edit and nothing else. A stored + // entry holding the same string would go stale silently, and could not + // narrow to the selected account the way this does through + // runCurrentQuery()'s existing scope wrap. + // + // Hidden entirely when no account configures a sent folder, rather than + // offering a button that always finds nothing. + if (!m_config.allSentQuery().isEmpty()) { + auto *sentButton = new QPushButton(tr("Sent"), row); + sentButton->setObjectName(QStringLiteral("sentButton")); + connect(sentButton, &QPushButton::clicked, this, [this]() { + m_queryEdit->setText(m_config.allSentQuery()); + // Flat for this query only. runCurrentQuery() clears it again for + // anything else, including the same query typed by hand, so the + // flag cannot outlive the button that set it. + runQuery(FlatResult::Yes); + }); + box->addWidget(sentButton); + } + + // The overflow menu, and only when something is in it: an empty menu + // button is a control that always does nothing. + if (!unpinned.isEmpty()) { + auto *menuButton = new QPushButton(tr("More queries"), row); + menuButton->setObjectName(QStringLiteral("savedQueryMenuButton")); + auto *menu = new QMenu(menuButton); + for (const SavedQuery &saved : unpinned) { + QAction *action = menu->addAction(saved.name); + connect(action, &QAction::triggered, this, + [this, saved]() { runSavedQuery(saved); }); + } + menuButton->setMenu(menu); + box->addWidget(menuButton); + } + + box->addStretch(1); + layout->addWidget(row); + + // Nothing saved and no sent folder leaves an empty strip of padding, so + // the row goes away rather than sitting there as a gap. + if (box->count() == 1) + row->hide(); +} + +void MainWindow::runSavedQuery(const SavedQuery &saved) +{ + // Through the dropdown, never by pre-scoping the text: runQuery() applies + // the selected account's path itself, so a scope baked in here would be + // applied twice. An unscoped query CLEARS the selection rather than + // inheriting whatever was there, which is the defect the rules preview hit. + const int index = saved.account.isEmpty() + ? m_accountBox->findData(QString()) + : m_accountBox->findData(saved.account); + if (index >= 0) + m_accountBox->setCurrentIndex(index); + + m_queryEdit->setText(saved.query); + runCurrentQuery(); +} + +void MainWindow::saveCurrentQuery() +{ + const QString query = m_queryEdit->text().trimmed(); + if (query.isEmpty()) + return; + + SaveQueryDialog dialog(m_config, query, + m_accountBox->currentData().toString(), this); + if (dialog.exec() != QDialog::Accepted) + return; + + QList queries = m_config.savedQueries(); + const SavedQuery saved = dialog.savedQuery(); + + // Replacing by name keeps the dialog's overwrite offer honest, and keeps + // the entry where it already sat rather than moving it to the end. + bool replaced = false; + for (SavedQuery &existing : queries) { + if (existing.name.compare(saved.name, Qt::CaseInsensitive) == 0) { + // The unknown fields belong to the STORED entry, not to the + // dialog's fresh value, so a field a later build wrote survives + // being edited here. + SavedQuery merged = saved; + merged.unknown = existing.unknown; + existing = merged; + replaced = true; + break; + } + } + if (!replaced) + queries.append(saved); + + m_config.setSavedQueries(queries); + if (!m_config.saveSavedQueries()) { + QMessageBox::warning(this, tr("Save query"), + tr("Could not write the saved queries file.")); + return; + } + + rebuildSavedQueryRow(); + statusBar()->showMessage(tr("Saved query '%1'.").arg(saved.name), + kStatusMessageMs); +} + +void MainWindow::rebuildSavedQueryRow() +{ + // The row is rebuilt wholesale rather than patched: a new query can be + // pinned, unpinned, or replace an existing one, and each moves a different + // widget. Deleting and rebuilding is a handful of buttons and cannot get + // the three cases wrong. + auto *old = findChild(QStringLiteral("savedQueryRow")); + if (!old) + return; + + auto *layout = qobject_cast(centralWidget()->layout()); + if (!layout) + return; + + const int index = layout->indexOf(old); + layout->removeWidget(old); + old->deleteLater(); + + buildSavedQueryRow(centralWidget(), layout); + + // buildSavedQueryRow appends; move it back to where the old row sat, or it + // lands under the thread list. + if (index >= 0) { + auto *item = layout->takeAt(layout->count() - 1); + layout->insertItem(index, item); + } +} + void MainWindow::runQuery(FlatResult flat) { // Set on EVERY run, not only when Yes. This is the line that stops flat diff --git a/src/mainwindow.h b/src/mainwindow.h index 18fbbb4..8b43fd0 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -49,6 +49,7 @@ class QPlainTextEdit; class QSplitter; class QProgressBar; class QTimer; +class QVBoxLayout; class ThreadListModel; class MessageView; @@ -231,6 +232,26 @@ private: /// is what widgets connect to. void runQuery(FlatResult flat); + /// Builds the row of saved-query buttons, the overflow menu and Sent. + /// + /// Its own row since item 23: an unbounded list of buttons sharing the + /// query row squeezed the field, which is the whole reason for the + /// pinned/unpinned split. + void buildSavedQueryRow(QWidget *parent, QVBoxLayout *layout); + + /// Runs a saved query, taking its account scope through the dropdown. + /// + /// Not by pre-scoping the text: runQuery() already wraps the query in the + /// selected account's path, so a scope baked in here would be applied + /// twice. Setting the dropdown also shows the user what scope they are in. + void runSavedQuery(const SavedQuery &saved); + + /// Names the current query and stores it in queries.json. + void saveCurrentQuery(); + + /// Rebuilds the saved-query row in place after the stored list changed. + void rebuildSavedQueryRow(); + private slots: void runCurrentQuery() { runQuery(FlatResult::No); } diff --git a/src/savequerydialog.cpp b/src/savequerydialog.cpp new file mode 100644 index 0000000..1ea97c2 --- /dev/null +++ b/src/savequerydialog.cpp @@ -0,0 +1,128 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#include "savequerydialog.h" + +#include +#include +#include +#include +#include +#include +#include +#include + +bool SaveQueryDialog::namesAnExistingQuery(const Config &config, + const QString &name) +{ + for (const SavedQuery &saved : config.savedQueries()) { + if (saved.name.compare(name.trimmed(), Qt::CaseInsensitive) == 0) + return true; + } + return false; +} + +SaveQueryDialog::SaveQueryDialog(const Config &config, const QString &query, + const QString &accountKey, QWidget *parent) + : QDialog(parent) + , m_config(config) +{ + setWindowTitle(tr("Save query")); + + auto *layout = new QVBoxLayout(this); + auto *form = new QFormLayout; + + m_name = new QLineEdit(this); + m_name->setObjectName(QStringLiteral("saveQueryName")); + m_name->setPlaceholderText(tr("A name for this query")); + form->addRow(tr("Name"), m_name); + + m_query = new QLineEdit(query, this); + m_query->setObjectName(QStringLiteral("saveQueryQuery")); + form->addRow(tr("Query"), m_query); + + // The scope is stored as an account KEY, so the entries carry the key as + // data exactly as the main window's dropdown does. "All accounts" is the + // empty key, not a missing entry, so an unscoped query is a real choice + // rather than the absence of one. + m_account = new QComboBox(this); + m_account->setObjectName(QStringLiteral("saveQueryAccount")); + m_account->addItem(tr("All accounts"), QString()); + for (const Account &account : config.accounts()) + m_account->addItem(account.key, account.key); + const int index = m_account->findData(accountKey); + m_account->setCurrentIndex(index >= 0 ? index : 0); + form->addRow(tr("Account"), m_account); + + m_pinned = new QCheckBox(tr("Show as a button"), this); + m_pinned->setObjectName(QStringLiteral("saveQueryPinned")); + m_pinned->setChecked(true); + form->addRow(QString(), m_pinned); + + layout->addLayout(form); + + // Says what is about to happen rather than refusing the name. Overwriting + // a saved query on purpose is a normal edit, and the only thing worth + // preventing is doing it without noticing. + m_notice = new QLabel(this); + m_notice->setObjectName(QStringLiteral("saveQueryNotice")); + m_notice->setWordWrap(true); + layout->addWidget(m_notice); + + auto *buttons = new QDialogButtonBox( + QDialogButtonBox::Save | QDialogButtonBox::Cancel, this); + connect(buttons, &QDialogButtonBox::accepted, this, &QDialog::accept); + connect(buttons, &QDialogButtonBox::rejected, this, &QDialog::reject); + layout->addWidget(buttons); + + m_ok = buttons->button(QDialogButtonBox::Save); + m_ok->setObjectName(QStringLiteral("saveQueryOk")); + + connect(m_name, &QLineEdit::textChanged, + this, &SaveQueryDialog::updateOkState); + connect(m_query, &QLineEdit::textChanged, + this, &SaveQueryDialog::updateOkState); + updateOkState(); + + m_name->setFocus(); +} + +void SaveQueryDialog::updateOkState() +{ + const QString name = m_name->text().trimmed(); + const bool usable = !name.isEmpty() && !m_query->text().trimmed().isEmpty(); + m_ok->setEnabled(usable); + + if (!name.isEmpty() && namesAnExistingQuery(m_config, name)) { + m_notice->setText( + tr("A saved query named '%1' already exists and will be " + "replaced.").arg(name)); + } else { + m_notice->clear(); + } +} + +SavedQuery SaveQueryDialog::savedQuery() const +{ + SavedQuery saved; + saved.name = m_name->text().trimmed(); + saved.query = m_query->text().trimmed(); + saved.account = m_account->currentData().toString(); + saved.pinned = m_pinned->isChecked(); + return saved; +} diff --git a/src/savequerydialog.h b/src/savequerydialog.h new file mode 100644 index 0000000..be5a2af --- /dev/null +++ b/src/savequerydialog.h @@ -0,0 +1,63 @@ +/* + * qtmaildir - a Qt6 mail client for notmuch-indexed Maildirs + * Copyright (C) 2026 Danilo M. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#pragma once + +#include + +#include "config.h" + +class QCheckBox; +class QComboBox; +class QLabel; +class QLineEdit; +class QPushButton; + +/// Names a query and keeps it in queries.json. +/// +/// Pure UI: it is handed the config and returns a SavedQuery. It writes +/// nothing, so it can be tested without touching a file, and the caller owns +/// the decision of what to do with the result. +class SaveQueryDialog : public QDialog +{ + Q_OBJECT +public: + /// `query` is the text to store, `accountKey` the scope to preselect, + /// which is the account the user is already looking at. + SaveQueryDialog(const Config &config, const QString &query, + const QString &accountKey, QWidget *parent = nullptr); + + /// The query as edited. Only meaningful after exec() returned Accepted. + SavedQuery savedQuery() const; + + /// Whether `name` already names a stored query. Case-insensitive, matching + /// how startup_query resolves, so "inbox" and "Inbox" cannot both exist and + /// leave the user unable to tell which one a button ran. + static bool namesAnExistingQuery(const Config &config, const QString &name); + +private: + void updateOkState(); + + const Config &m_config; + QLineEdit *m_name = nullptr; + QLineEdit *m_query = nullptr; + QComboBox *m_account = nullptr; + QCheckBox *m_pinned = nullptr; + QPushButton *m_ok = nullptr; + QLabel *m_notice = nullptr; +}; diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 4c1c5d9..2c70e0f 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -188,6 +188,14 @@ private slots: void flatModeDoesNotSurviveTheNextQuery(); void noTwoActionsShareAnIcon(); + void onlyPinnedQueriesBecomeButtons(); + void unpinnedQueriesReachTheMenu(); + void pinnedButtonsFollowTheDocumentOrder(); + void theSavedQueryMenuIsHiddenWhenEveryQueryIsPinned(); + void aScopedSavedQuerySelectsItsAccount(); + void anUnscopedSavedQueryClearsTheAccount(); + void theSaveQueryActionIsDisabledOnAnEmptyQuery(); + private: /// Owns the throwaway lock table init() points every test at. A pointer /// rather than a value because it is rebuilt per test, and QTemporaryDir @@ -5265,4 +5273,271 @@ int main(int argc, char *argv[]) return QTest::qExec(&test, argc, argv); } +// --------------------------------------------------------------------------- +// Saved queries in the query row (item 23) +// --------------------------------------------------------------------------- + +/// Writes a config plus a queries.json beside it, and loads both. +static void loadWithQueries(Config &config, QTemporaryDir &dir, + const QString &queriesJson, + const QString &iniExtra = {}) +{ + QDir().mkpath(dir.filePath(QStringLiteral("qtmaildir"))); + const QString conf = + dir.filePath(QStringLiteral("qtmaildir/qtmaildir.conf")); + QFile ini(conf); + ini.open(QIODevice::WriteOnly | QIODevice::Text); + ini.write(iniExtra.toUtf8()); + ini.close(); + + QFile json(dir.filePath(QStringLiteral("qtmaildir/queries.json"))); + json.open(QIODevice::WriteOnly); + json.write(queriesJson.toUtf8()); + json.close(); + + config.load(conf); +} + +/// Buttons in the saved-query row, by label, in the order they are laid out. +static QStringList savedQueryButtonLabels(MainWindow &window) +{ + QStringList labels; + auto *row = window.findChild(QStringLiteral("savedQueryRow")); + if (!row) + return labels; + const QList buttons = + row->findChildren(QString(), Qt::FindDirectChildrenOnly); + for (QPushButton *button : buttons) { + // The menu button is not a saved query and must not be counted as one. + if (button->objectName() != QStringLiteral("savedQueryMenuButton")) + labels.append(button->text()); + } + return labels; +} + +void TestMainWindow::onlyPinnedQueriesBecomeButtons() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Inbox", "query": "tag:inbox", "pinned": true }, + { "name": "Buried", "query": "tag:buried" } + ] + })")); + + MainWindow window(config); + const QStringList labels = savedQueryButtonLabels(window); + + QVERIFY2(labels.contains(QStringLiteral("Inbox")), + "a pinned query must have a button"); + QVERIFY2(!labels.contains(QStringLiteral("Buried")), + "an unpinned query must NOT have a button"); +} + +void TestMainWindow::unpinnedQueriesReachTheMenu() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Inbox", "query": "tag:inbox", "pinned": true }, + { "name": "Buried", "query": "tag:buried" } + ] + })")); + + MainWindow window(config); + auto *menuButton = + window.findChild(QStringLiteral("savedQueryMenuButton")); + QVERIFY2(menuButton, "an unpinned query needs a menu to live in"); + QVERIFY(menuButton->menu()); + + QStringList entries; + const QList actions = menuButton->menu()->actions(); + for (QAction *action : actions) + entries.append(action->text()); + + QVERIFY2(entries.contains(QStringLiteral("Buried")), + "the unpinned query is missing from the menu"); + // A pinned query is already a button; listing it twice is the duplicate + // this asserts against. + QVERIFY2(!entries.contains(QStringLiteral("Inbox")), + "a pinned query must not also appear in the menu"); +} + +/// The property the whole storage change was made for. "Zebra" is written +/// first and must stay first; alphabetical order would put it last. +void TestMainWindow::pinnedButtonsFollowTheDocumentOrder() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Zebra", "query": "tag:zebra", "pinned": true }, + { "name": "Apple", "query": "tag:apple", "pinned": true } + ] + })")); + + MainWindow window(config); + const QStringList labels = savedQueryButtonLabels(window); + + QCOMPARE(labels.size(), 2); + QCOMPARE(labels.at(0), QStringLiteral("Zebra")); + QCOMPARE(labels.at(1), QStringLiteral("Apple")); +} + +void TestMainWindow::theSavedQueryMenuIsHiddenWhenEveryQueryIsPinned() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Inbox", "query": "tag:inbox", "pinned": true } + ] + })")); + + MainWindow window(config); + + // The guard the assertion below needs. Asserting only that the menu button + // is absent passed against NO implementation at all, before any of this + // was built, so it has to prove first that the row it is looking in was + // populated and that a button was found. + const QStringList labels = savedQueryButtonLabels(window); + QCOMPARE(labels, QStringList{ QStringLiteral("Inbox") }); + + auto *menuButton = + window.findChild(QStringLiteral("savedQueryMenuButton")); + QVERIFY2(!menuButton, + "an empty menu button is a control that always does nothing"); +} + +/// The scope goes through the account dropdown rather than being baked into +/// the query text. runQuery() already scopes by that dropdown, so pre-scoping +/// the text would apply the path twice, and the selection would be invisible. +void TestMainWindow::aScopedSavedQuerySelectsItsAccount() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Billing", "query": "from:billing", + "account": "work", "pinned": true } + ] + })"), QStringLiteral( + "[account.work]\n" + "name=Test User\n" + "address=user@example.org\n" + "maildir=work-mail\n" + "\n" + "[account.personal]\n" + "name=Test User\n" + "address=me@example.net\n" + "maildir=personal\n" + )); + + MainWindow window(config); + auto *accountBox = + window.findChild(QStringLiteral("accountBox")); + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + QVERIFY(accountBox); + QVERIFY(queryEdit); + + // Start somewhere else, so a passing result cannot be the default. + accountBox->setCurrentIndex(accountBox->findData( + QStringLiteral("personal"))); + QCOMPARE(accountBox->currentData().toString(), QStringLiteral("personal")); + + auto *row = window.findChild(QStringLiteral("savedQueryRow")); + QVERIFY(row); + auto *button = row->findChild(); + QVERIFY(button); + button->click(); + + QCOMPARE(accountBox->currentData().toString(), QStringLiteral("work")); + // The text is the bare query. The path scope is applied once, by + // runQuery(), from the dropdown this just set. + QCOMPARE(queryEdit->text(), QStringLiteral("from:billing")); + QVERIFY2(!queryEdit->text().contains(QStringLiteral("path:")), + "the scope must not be baked into the query text"); +} + +/// A query with no account must CLEAR the dropdown, not inherit whatever the +/// last one left there. Confirmed against the same defect in the rules +/// preview, where an already-selected account survived the click. +void TestMainWindow::anUnscopedSavedQueryClearsTheAccount() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Everywhere", "query": "tag:inbox", "pinned": true } + ] + })"), QStringLiteral( + "[account.work]\n" + "name=Test User\n" + "address=user@example.org\n" + "maildir=work-mail\n" + )); + + MainWindow window(config); + auto *accountBox = + window.findChild(QStringLiteral("accountBox")); + QVERIFY(accountBox); + + accountBox->setCurrentIndex(accountBox->findData(QStringLiteral("work"))); + QCOMPARE(accountBox->currentData().toString(), QStringLiteral("work")); + + auto *row = window.findChild(QStringLiteral("savedQueryRow")); + QVERIFY(row); + auto *button = row->findChild(); + QVERIFY(button); + button->click(); + + QVERIFY2(accountBox->currentData().toString().isEmpty(), + "an unscoped saved query must clear the account selection"); +} + +void TestMainWindow::theSaveQueryActionIsDisabledOnAnEmptyQuery() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, "queries": [] + })")); + + MainWindow window(config); + auto *save = window.findChild(QStringLiteral("save_query")); + QVERIFY2(save, "there is no way to save a query"); + + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + QVERIFY(queryEdit); + + queryEdit->clear(); + QVERIFY2(!save->isEnabled(), + "saving an empty query would store a query that matches nothing"); + + queryEdit->setText(QStringLiteral("tag:inbox")); + QVERIFY2(save->isEnabled(), "a real query must be savable"); + + // Whitespace is not a query. setText does not drive a completer, but it + // does emit textChanged, which is what the enabling is hung on. + queryEdit->setText(QStringLiteral(" ")); + QVERIFY(!save->isEnabled()); +} + #include "test_mainwindow.moc" -- cgit v1.2.3 From 14842cd136e45c551dde4f276af1176bfdf22023 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 19:29:18 +0200 Subject: fix(queries): put the Save query button beside the query bar The spec asked for "a Save query button beside the search bar" and what shipped was a menu entry and Ctrl+S. The user went looking for the button where the design said it would be and did not find it. Saving is a thing you decide on while looking at the results, so it belongs where the results came from rather than behind a menu or a remembered chord. The button takes the action through setDefaultAction rather than a second connect, so it inherits the text, icon, tooltip and enabled state and cannot end up offering to save an empty query while the menu entry correctly refuses. The mutation that replaces it with a plain clicked() connection fails the test. Also records item 82: a saved query cannot be edited, unpinned or deleted from the UI. Item 23 specified saving and nothing else, and that is exactly what was built, so the only way to unpin a query is a text editor or retyping it in full under the same name. An action that creates something the UI cannot then change or remove is incomplete, and this was found within minutes of the first hand test. It is filed as a defect rather than an enhancement, and the spec now says so where a reader would otherwise take the design for complete. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 9 +++-- .../plans/2026-08-03-post-0.1.0-usability.md | 44 ++++++++++++++++++++++ .../specs/2026-08-13-saved-queries-design.md | 11 ++++++ src/mainwindow.cpp | 17 +++++++++ src/mainwindow.h | 3 ++ tests/test_mainwindow.cpp | 34 +++++++++++++++++ 6 files changed, 114 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d264e2..428e3e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,10 +13,11 @@ point at which they are stable. ### Added -- A **Save query** action (`Ctrl+S`) keeps the query in the bar as a saved - query, naming it, optionally scoping it to one account, and choosing whether - it appears as a button or in a menu. Saved queries no longer have to be added - by hand-editing the config file (item 23). +- A **Save query** button beside the query bar, also on the Edit menu and bound + to `Ctrl+S`, keeps the query in the bar as a saved query: naming it, + optionally scoping it to one account, and choosing whether it appears as a + button or in a menu. Saved queries no longer have to be added by + hand-editing the config file (item 23). - Saved queries live in `~/.config/qtmaildir/queries.json`, which carries their **order**, a `pinned` flag and an optional account scope. The order in the file is the order the buttons appear in, so rearranging them is a matter of diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index 268dbb4..9d19e4d 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -146,6 +146,7 @@ taking that too literally. | 80 | A rule with many conditions squeezes the rule list to one visible row | defect | XS | **done** 2026-08-13 on `rule-builder`, unreleased. Follows item 76 | | 79 | Opening the rules dialog and saving destroys the first rule | defect | XS | **fixed on `rule-builder`** 2026-08-13, unreleased. Shipped in 0.16.0; damaged one real rule, repaired by hand | | 81 | No way to turn a saved query into a tagging rule | workflow | S | open; depends on 23, which builds the dialog, and is excluded from its spec on purpose. Writes to the shared rules file, so it spans this repo and `mailctl` | +| 82 | A saved query cannot be edited, unpinned or deleted from the UI | defect | S | open; found by hand-testing item 23 on 2026-08-13. Saving works, unsaving does not | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -574,6 +575,49 @@ failing silently. **Size: S** on top of 23, and not meaningful before it. +## 82. A saved query cannot be edited, unpinned or deleted from the UI + +**Observed (user, 2026-08-13):** hand-testing item 23. The user saved a query, +then asked how to unpin it, and there is no answer that does not involve either +a text editor or retyping the whole query. + +**Cause:** item 23 specified saving and nothing else, and that is exactly what +shipped. `SaveQueryDialog` opens on the contents of the query BAR, not on a +stored entry, so the only route to changing one field of an existing query is to +reconstruct the whole query, name it identically, and let +`MainWindow::saveCurrentQuery()` replace it by name. There is no delete at any +price: nothing in the UI removes an entry from `queries.json`. + +This is a defect rather than a missing enhancement. An action that creates +something the UI cannot then edit or remove is incomplete, and the user hit it +within minutes of the first hand test. + +**Approach.** A context menu on a saved-query button and on each **More +queries** entry, offering Edit, Unpin (or Pin) and Delete. + +- **Edit** opens `SaveQueryDialog` prefilled from the STORED entry rather than + from the query bar. The dialog already carries every field it needs; what it + lacks is a constructor that takes a `SavedQuery`. +- **Unpin** is a one-field write and does not need the dialog at all. +- **Delete** removes the entry and rewrites the file. + +**Constraints.** + +- `saveCurrentQuery()` already merges an existing entry's `unknown` fields over + the dialog's fresh value, and every one of these paths must do the same or a + field written by a later build is dropped by an edit here. +- Renaming through Edit is a rename, not a second entry: match on the name the + dialog was OPENED with, not the one it returns, or renaming silently creates a + duplicate and leaves the original behind. +- Delete is destructive and the file is user config, so it is one of the few + places in this application that wants a confirmation. The no-confirmation rule + in CLAUDE.md is about tag mutations, which are undoable through the undo + stack; this is not on that stack and cannot be undone. +- A test must exercise every route the way item 75's did not: the dialog's + Cancel goes through `done(int)` and never sends a `QCloseEvent`. + +**Size: S**, and it should land before the saved-query work is called done. + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering diff --git a/docs/superpowers/specs/2026-08-13-saved-queries-design.md b/docs/superpowers/specs/2026-08-13-saved-queries-design.md index 6d0019f..ba6b23e 100644 --- a/docs/superpowers/specs/2026-08-13-saved-queries-design.md +++ b/docs/superpowers/specs/2026-08-13-saved-queries-design.md @@ -160,6 +160,17 @@ Sent stays where it is. It is not a saved query, it is built from would mean generating a per-account path query into stored config, which is the duplication the `account`-key decision just rejected. +## Editing and deleting are NOT here, and that is a defect + +This document specifies creating a saved query and says nothing about changing +or removing one. That gap shipped: the first hand test produced "how do I unpin +a query?", and the honest answer was a text editor. Recorded as **item 82**, +sized S, and it should land before this work is called finished. + +Anything built there must merge the stored entry's `unknown` fields the way +`saveCurrentQuery()` does, and must match a rename on the name the dialog was +opened with rather than the one it returns. + ## What is deliberately not here **Item 81, saving a query as a tagging rule.** A saved query is a view and costs diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 3966e9f..9eff3d5 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -45,6 +45,7 @@ #include #include #include +#include #include #include "mailsync.h" @@ -348,6 +349,12 @@ MainWindow::MainWindow(const Config &config, QWidget *parent) // also set programmatically, by the saved-query buttons and by // recoverStaleThread(), and the action must track those too. if (QAction *save = m_actions.value(QStringLiteral("save_query"))) { + // setDefaultAction, not a second connect: the button then takes the + // action's text, icon, tooltip and ENABLED state, so it cannot end up + // offering to save an empty query while the menu entry refuses. + m_saveQueryButton->setDefaultAction(save); + m_saveQueryButton->setToolButtonStyle(Qt::ToolButtonIconOnly); + auto updateSaveState = [this, save]() { save->setEnabled(!m_queryEdit->text().trimmed().isEmpty()); }; @@ -566,6 +573,16 @@ void MainWindow::buildUi() queryRow->addWidget(m_accountBox); queryRow->addWidget(m_sortOrder); queryRow->addWidget(m_queryEdit, 1); + + // Beside the field, where a user looks for it. The menu entry and Ctrl+S + // were not enough on their own: saving is a thing you decide on while + // looking at the results, so it needs to be visible at the query bar + // rather than remembered. Created here and given its action in the + // constructor, since registerActions() has not run yet. + m_saveQueryButton = new QToolButton(central); + m_saveQueryButton->setObjectName(QStringLiteral("saveQueryButton")); + queryRow->addWidget(m_saveQueryButton); + layout->addLayout(queryRow); buildSavedQueryRow(central, layout); diff --git a/src/mainwindow.h b/src/mainwindow.h index 8b43fd0..e6ee322 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -49,6 +49,7 @@ class QPlainTextEdit; class QSplitter; class QProgressBar; class QTimer; +class QToolButton; class QVBoxLayout; class ThreadListModel; @@ -625,6 +626,8 @@ private: QUndoStack m_undoStack; QLineEdit *m_queryEdit = nullptr; + /// Save query, beside the field. Driven by the save_query action. + QToolButton *m_saveQueryButton = nullptr; QueryCompleter *m_queryCompleter = nullptr; /// Its own type, not the QTreeView base. The strip painting and the /// expander column are ThreadListView's, and holding the base here only diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 2c70e0f..581daae 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -195,6 +195,7 @@ private slots: void aScopedSavedQuerySelectsItsAccount(); void anUnscopedSavedQueryClearsTheAccount(); void theSaveQueryActionIsDisabledOnAnEmptyQuery(); + void thereIsASaveButtonBesideTheQueryBar(); private: /// Owns the throwaway lock table init() points every test at. A pointer @@ -5540,4 +5541,37 @@ void TestMainWindow::theSaveQueryActionIsDisabledOnAnEmptyQuery() QVERIFY(!save->isEnabled()); } +/// A menu entry and a shortcut are not a button. The spec asks for one beside +/// the query bar, and the user went looking for it there and did not find it. +void TestMainWindow::thereIsASaveButtonBesideTheQueryBar() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, "queries": [] + })")); + + MainWindow window(config); + auto *button = + window.findChild(QStringLiteral("saveQueryButton")); + QVERIFY2(button, "no Save query button beside the query bar"); + + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + QVERIFY(queryEdit); + + // In the query row itself, not somewhere else in the window that a + // findChild would also reach. + QCOMPARE(button->parentWidget(), queryEdit->parentWidget()); + + // Follows the action, so it cannot offer to save an empty query while the + // menu entry correctly refuses. + queryEdit->clear(); + QVERIFY2(!button->isEnabled(), + "the button must follow the action's enabled state"); + queryEdit->setText(QStringLiteral("tag:inbox")); + QVERIFY(button->isEnabled()); +} + #include "test_mainwindow.moc" -- cgit v1.2.3 From 59f5161500ef670421cbba9665bd4aadbbb9ad30 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 19:35:15 +0200 Subject: feat(queries): right-align the More queries menu, and keep the row when nothing is pinned The saved-query buttons are the row's content and read as a set; the overflow menu is a control over that set, so it belongs apart from them rather than trailing the last button. Moving the stretch above it pushes it to the right edge. Doing that exposed a latent defect in the same function. The row hid itself when its layout held nothing but the stretch, which was written as a count of one and happened to be right only because the stretch went last. With the stretch moved the count changes, and the condition as written would have hidden a row holding only the menu: a config with saved queries but none pinned would have had no route to any of them, the menu buried along with the row. The check now counts the content added before the stretch and treats an unpinned query as content in its own right. Both are mutation-checked. Putting the stretch back at the end fails the alignment test, and restoring the old hide condition fails the new one, which asserts the row survives with nothing but unpinned queries in it. The alignment is asserted on the layout's own ordering rather than on x coordinates, since a geometry assertion would also pass for a row that merely ran out of width. Co-Authored-By: Claude Opus 5 --- src/mainwindow.cpp | 16 ++++++++--- tests/test_mainwindow.cpp | 72 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 4 deletions(-) diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 9eff3d5..61c883e 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1692,6 +1692,13 @@ void MainWindow::buildSavedQueryRow(QWidget *parent, QVBoxLayout *layout) box->addWidget(sentButton); } + // Everything above is left-aligned; the stretch here pushes what follows + // to the right edge. The buttons are the row's content and read as a set, + // while the overflow menu is a control over that set, so it sits apart + // from them rather than trailing the last one. + const int contentCount = box->count(); + box->addStretch(1); + // The overflow menu, and only when something is in it: an empty menu // button is a control that always does nothing. if (!unpinned.isEmpty()) { @@ -1707,12 +1714,13 @@ void MainWindow::buildSavedQueryRow(QWidget *parent, QVBoxLayout *layout) box->addWidget(menuButton); } - box->addStretch(1); layout->addWidget(row); - // Nothing saved and no sent folder leaves an empty strip of padding, so - // the row goes away rather than sitting there as a gap. - if (box->count() == 1) + // Nothing on either side of the stretch leaves an empty strip of padding, + // so the row goes away rather than sitting there as a gap. Counted before + // the stretch was added, since the stretch is always there: an unpinned + // query with no pinned ones still needs the row for its menu. + if (contentCount == 0 && unpinned.isEmpty()) row->hide(); } diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 581daae..080e4cd 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -52,6 +52,7 @@ #include #include +#include #include #include #include "tagchip.h" @@ -196,6 +197,8 @@ private slots: void anUnscopedSavedQueryClearsTheAccount(); void theSaveQueryActionIsDisabledOnAnEmptyQuery(); void thereIsASaveButtonBesideTheQueryBar(); + void theMenuIsRightAlignedAwayFromTheButtons(); + void theRowSurvivesWithNothingButUnpinnedQueries(); private: /// Owns the throwaway lock table init() points every test at. A pointer @@ -5574,4 +5577,73 @@ void TestMainWindow::thereIsASaveButtonBesideTheQueryBar() QVERIFY(button->isEnabled()); } +/// Right-aligned, meaning a stretch sits between the buttons and the menu. +/// Asserted on the layout rather than on x coordinates: the offscreen platform +/// lays out widgets, but a geometry assertion here would also pass for a row +/// that simply ran out of width. +void TestMainWindow::theMenuIsRightAlignedAwayFromTheButtons() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Inbox", "query": "tag:inbox", "pinned": true }, + { "name": "Buried", "query": "tag:buried" } + ] + })")); + + MainWindow window(config); + auto *row = window.findChild(QStringLiteral("savedQueryRow")); + QVERIFY(row); + auto *box = qobject_cast(row->layout()); + QVERIFY(box); + + auto *menuButton = + window.findChild(QStringLiteral("savedQueryMenuButton")); + QVERIFY(menuButton); + + int menuIndex = -1; + int stretchIndex = -1; + for (int i = 0; i < box->count(); ++i) { + QLayoutItem *item = box->itemAt(i); + if (item->widget() == menuButton) + menuIndex = i; + else if (!item->widget() && item->spacerItem()) + stretchIndex = i; + } + + QVERIFY2(stretchIndex >= 0, "the row has no stretch to align against"); + QVERIFY2(menuIndex > stretchIndex, + "the menu must come AFTER the stretch to sit at the right edge"); +} + +/// The row must not vanish when every saved query is unpinned: the menu is +/// then the only way to reach any of them, and hiding the row buries it. +void TestMainWindow::theRowSurvivesWithNothingButUnpinnedQueries() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Buried", "query": "tag:buried" }, + { "name": "AlsoBuried", "query": "tag:also" } + ] + })")); + + MainWindow window(config); + auto *row = window.findChild(QStringLiteral("savedQueryRow")); + QVERIFY(row); + QVERIFY2(!row->isHidden(), + "the row was hidden, so the only route to these queries is gone"); + + auto *menuButton = + window.findChild(QStringLiteral("savedQueryMenuButton")); + QVERIFY(menuButton); + QCOMPARE(menuButton->menu()->actions().size(), 2); +} + #include "test_mainwindow.moc" -- cgit v1.2.3 From 8268bb478dbdf9f23b35c5114c5d13bc98254659 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 19:40:27 +0200 Subject: fix(queries): give Save query a clearer icon and a label document-save is the floppy/disk shape. It means "write a file somewhere", which leaves the user to guess what is being written, and next to a row of saved-query buttons it reads as an unrelated control. Saving a query is bookmarking a search, so bookmark-new is the icon every desktop already uses for "keep this for later". Verified to resolve with real art in the desktop's actual theme rather than assumed present. The button also shows its label now instead of the icon alone. It sits among text buttons, and an icon on its own next to them reads as a different kind of control; it is also the one action whose meaning an icon cannot carry, since "save" is a familiar shape whose question is always "save what?". The toolbar is unaffected and still follows the desktop's own button style. The label is the button's own text rather than the action's. "&Save query..." is menu phrasing, and setDefaultAction copies it verbatim, so the button rendered an accelerator ampersand and the ellipsis that promises a dialog. The action keeps both for the menu it lives in, and the test asserts the override survives setDefaultAction rather than trusting that it does. Co-Authored-By: Claude Opus 5 --- src/mainwindow.cpp | 19 +++++++++++++++++-- tests/test_mainwindow.cpp | 18 ++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 61c883e..f06b308 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -353,7 +353,18 @@ MainWindow::MainWindow(const Config &config, QWidget *parent) // action's text, icon, tooltip and ENABLED state, so it cannot end up // offering to save an empty query while the menu entry refuses. m_saveQueryButton->setDefaultAction(save); - m_saveQueryButton->setToolButtonStyle(Qt::ToolButtonIconOnly); + // Icon AND text, unlike the toolbar, which follows the desktop's + // button style. This button sits in a row of text buttons, the saved + // queries, and an icon on its own next to them reads as a different + // kind of control than it is. It is also the one action whose meaning + // an icon alone does not carry: "save" is a shape everyone knows and + // the question is always "save WHAT". + m_saveQueryButton->setToolButtonStyle(Qt::ToolButtonTextBesideIcon); + // Its own text, not the action's: "&Save query..." is menu phrasing, + // and a button rendering the ampersand's accelerator and the ellipsis + // that promises a dialog reads as a menu entry that escaped. The + // action keeps both for the menu it lives in. + m_saveQueryButton->setText(tr("Save")); auto updateSaveState = [this, save]() { save->setEnabled(!m_queryEdit->text().trimmed().isEmpty()); @@ -1066,7 +1077,11 @@ void MainWindow::buildMenus() // the selection's tags. { QStringLiteral("tag_rules"), QStringLiteral("configure") }, { QStringLiteral("complete_query"), QStringLiteral("edit-find-replace") }, - { QStringLiteral("save_query"), QStringLiteral("document-save") }, + // NOT "document-save": that is the floppy/disk shape, which reads as + // "write a file somewhere" and asks the user to guess what is being + // written. Saving a query is bookmarking a search, and bookmark-new is + // the icon set every desktop already uses for "keep this for later". + { QStringLiteral("save_query"), QStringLiteral("bookmark-new") }, { QStringLiteral("select_all"), QStringLiteral("edit-select-all") }, { QStringLiteral("clear_pane"), QStringLiteral("edit-clear") }, { QStringLiteral("clear_selection"), QStringLiteral("edit-clear-all") }, diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 080e4cd..42d7d78 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -52,6 +52,7 @@ #include #include +#include #include #include #include @@ -5560,6 +5561,23 @@ void TestMainWindow::thereIsASaveButtonBesideTheQueryBar() window.findChild(QStringLiteral("saveQueryButton")); QVERIFY2(button, "no Save query button beside the query bar"); + // Icon AND text. An icon alone was the first version and read as + // ambiguous: "save" is a familiar shape whose meaning is always "save + // what?". + auto *toolButton = qobject_cast(button); + QVERIFY(toolButton); + QCOMPARE(toolButton->toolButtonStyle(), Qt::ToolButtonTextBesideIcon); + QVERIFY2(!button->icon().isNull(), "the button has no icon"); + QVERIFY2(!button->text().isEmpty(), "the button has no text"); + + // Button phrasing, not the menu's: no accelerator ampersand, and no + // ellipsis. setDefaultAction copies the action's text, so this asserts the + // override survived it. + QVERIFY2(!button->text().contains(QLatin1Char('&')), + "the menu accelerator leaked onto the button"); + QVERIFY2(!button->text().contains(QStringLiteral("...")), + "the menu's ellipsis leaked onto the button"); + auto *queryEdit = window.findChild(QStringLiteral("queryEdit")); QVERIFY(queryEdit); -- cgit v1.2.3 From 97c81f8cad571ce9ce724ddab8269e911df05a7c Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 19:52:13 +0200 Subject: feat(queries): make Sent a saved query rather than a fixed button The user asked whether the default queries could be unified with Sent. The answer runs the other way: Sent joins the saved queries rather than the saved queries becoming hardcoded. Inbox, Unread and Important are complete strings that depend on nothing and can never go stale, so generating them would buy nothing and would cost the four things the file just gained: reordering, unpinning, renaming and deleting. Hardcoding them would also make them undeletable, which is a regression for anyone who does not want one of them. Sent is different only in that its query CANNOT be stored: it is composed from every account's `sent` key, so a stored copy goes stale the moment a folder is renamed. That is a property of Sent, not of "default queries". Storing the GENERATOR rather than its output keeps both halves: `"generated": "sent"` still resolves from the accounts at click time, and the entry is an ordinary row that can be reordered, renamed, unpinned or removed. The row now follows one rule instead of carrying one member the user did not own. Two properties had to travel with the entry. The composed query, resolved through Config::resolvedQuery() so what lands in the bar is what actually ran; and FLAT mode, since a sent view lists messages and a threaded one folds every reply back into the conversation the user sent one message into. The sent generator implies flat rather than trusting the file to say so, because a hand-edited row would otherwise produce a threaded sent view. An unknown generator is reported but the row is KEPT: a later build may know it, and dropping it here would delete it from the file on the next save, which is the same data loss the unknown-field handling exists to prevent. A generator whose accounts configure nothing is skipped entirely, exactly as the hardcoded button was hidden rather than offering one that finds nothing. Eight new tests. The four pre-existing Sent tests reach this through migration and were left alone, which is what proves the migrated path still behaves; the new ones cover a STORED file, which is the path every launch after the first takes. Mutations: a generator resolving to nothing fails three, ignoring flat fails two, and not skipping an empty generator fails one. A rename test guards the property the change exists for, since anything keyed on the literal name "Sent" would break it. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 12 +++- README.md | 28 +++++++-- src/config.cpp | 66 +++++++++++++++++++- src/config.h | 21 +++++++ src/mainwindow.cpp | 50 ++++++++------- tests/test_config.cpp | 156 ++++++++++++++++++++++++++++++++++++++++++++++ tests/test_mainwindow.cpp | 111 +++++++++++++++++++++++++++++++++ 7 files changed, 413 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 428e3e5..b384eda 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,11 @@ point at which they are stable. **order**, a `pinned` flag and an optional account scope. The order in the file is the order the buttons appear in, so rearranging them is a matter of moving lines. +- **Sent is a saved query now**, carrying `"generated": "sent"` instead of a + stored query. It is still composed from your accounts' `sent` keys every time + you click it, so correcting a folder name still updates it with no edit, but + it can now be reordered, renamed, unpinned or deleted like any other entry + rather than being a fixed button you did not own. ### Changed @@ -30,13 +35,18 @@ point at which they are stable. query field is no longer squeezed by a long list of buttons. - Saved-query buttons follow the file's order instead of appearing alphabetically. +- The Sent button is no longer hardcoded beside the saved queries, so the whole + row now follows one rule instead of having one member that behaved + differently from its neighbours. ### Upgrading Saved queries move out of the `[queries]` section of `qtmaildir.conf` and into `~/.config/qtmaildir/queries.json`. **The first launch migrates them for you**: the section is read, the JSON file is written from it, and every entry is -marked pinned so your buttons stay where they were. +marked pinned so your buttons stay where they were. Sent is appended as a +`generated` entry, where its button already sat, provided an account configures +a sent folder. Your config file is left byte-for-byte alone. The old `[queries]` section stays in it, ignored from then on, and can be deleted by hand whenever you like. It is diff --git a/README.md b/README.md index 81ec7d8..749d89d 100644 --- a/README.md +++ b/README.md @@ -223,16 +223,36 @@ queries** menu, which keeps the row usable once you have more than a handful. same as choosing that account in the dropdown; leave it out for a query that spans every account. +**Sent is an entry like any other**, and the one that carries `generated` +instead of `query`: + +```json +{ "name": "Sent", "generated": "sent", "pinned": true } +``` + +A generated query is composed from your accounts every time you click it, +rather than stored. That is why Sent has no `query` of its own: it is built +from every account's `sent` key, so adding an account or correcting a folder +name updates the button with no edit here. A stored copy of the same string +would quietly go on naming the old folder. + +Being an ordinary entry, it can be reordered, renamed, unpinned or deleted like +the rest. Renaming it to `Posta inviata` changes only the label. `sent` is the +only generator today, and it is skipped entirely when no account configures a +sent folder, rather than offering a button that finds nothing. + The name is what the button says, so `Important` and `Flagged` can run the same query and differ only in the label. **Upgrading from 0.17.0 or earlier.** Saved queries used to live in a `[queries]` section of `qtmaildir.conf`. The first launch after upgrading reads that section, writes `queries.json` from it, and marks every entry pinned so -your buttons stay where they were. Your config file is not modified: the old -`[queries]` section is left exactly as it is, ignored from then on, and you can -delete it by hand whenever you like. The reason it is not removed for you is -that rewriting the file would drop your comments and reorder your keys. +your buttons stay where they were. Sent is added as a `generated` entry at the +end, where its button already sat, provided an account configures a sent +folder. Your config file is not modified: the old `[queries]` section is left +exactly as it is, ignored from then on, and you can delete it by hand whenever +you like. The reason it is not removed for you is that rewriting the file would +drop your comments and reorder your keys. One behaviour changes with the move. Buttons used to appear in alphabetical order, because the INI backend returns keys sorted and preserving file order diff --git a/src/config.cpp b/src/config.cpp index 8cd5e56..9edba50 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -51,6 +51,14 @@ constexpr int kMaxToolbarIconSize = 64; /// two-repo change and no hook stops tagging if it is half-deployed. constexpr int kQueriesFormatVersion = 1; +/// Generators a saved query may name in its `generated` field. +/// +/// A closed set, checked on load so a typo is reported rather than producing a +/// button that silently finds nothing. Adding one here needs no format bump: +/// an older build keeps the row and reports it, which is why an unknown +/// generator is a problem rather than a reason to drop the entry. +const QStringList kQueryGenerators = { QStringLiteral("sent") }; + } // namespace QString Account::scopedQuery(const QString &query) const @@ -457,9 +465,26 @@ void Config::loadSavedQueries(const QString &configPath, QSettings &settings) } settings.endGroup(); + // Sent was a hardcoded button beside the saved queries and becomes an + // ordinary row here, so it can be reordered, renamed, unpinned or + // removed like any other. It stays GENERATED, so it still follows the + // accounts. Appended last, where the button already sat. + // + // Only when an account actually configures a sent folder: the button + // was hidden entirely otherwise, and migrating a row that always finds + // nothing would be worse than what it replaces. + if (!allSentQuery().isEmpty()) { + SavedQuery sent; + sent.name = QStringLiteral("Sent"); + sent.generated = QStringLiteral("sent"); + sent.pinned = true; + sent.flat = true; + m_savedQueries.append(sent); + } + // 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()) { + if (!m_savedQueries.isEmpty() && !saveSavedQueries()) { addProblem(QStringLiteral("Could not write saved queries to %1.") .arg(m_queriesPath)); } @@ -516,6 +541,26 @@ void Config::loadSavedQueries(const QString &configPath, QSettings &settings) query.query = object.value(QStringLiteral("query")).toString(); query.pinned = object.value(QStringLiteral("pinned")).toBool(false); query.account = object.value(QStringLiteral("account")).toString(); + query.generated = object.value(QStringLiteral("generated")).toString(); + // A generator carries its own view mode, so "sent" is flat whether or + // not the file says so. Storing it as a plain field would let a + // hand-edited or migrated-from-elsewhere row produce a THREADED sent + // view, which folds every reply back into the conversation the user + // sent one message into. The file may still set it for an ordinary + // query. + query.flat = object.value(QStringLiteral("flat")).toBool(false) + || query.generated == QStringLiteral("sent"); + + if (query.isGenerated() + && !kQueryGenerators.contains(query.generated)) { + // Reported but KEPT. A later build may know this generator, and + // dropping the row here would delete it from the file on the next + // save, which is the same data loss the unknown-field handling + // exists to prevent. + addProblem(QStringLiteral("Saved query '%1' uses an unknown " + "generator '%2' and will find nothing.") + .arg(query.name, query.generated)); + } if (query.name.isEmpty()) { addProblem(QStringLiteral("A saved query in %1 has no name and was " @@ -526,7 +571,8 @@ void Config::loadSavedQueries(const QString &configPath, QSettings &settings) for (auto it = object.begin(); it != object.end(); ++it) { static const QStringList known = { QStringLiteral("name"), QStringLiteral("query"), - QStringLiteral("pinned"), QStringLiteral("account") + QStringLiteral("pinned"), QStringLiteral("account"), + QStringLiteral("generated"), QStringLiteral("flat") }; if (!known.contains(it.key())) query.unknown.insert(it.key(), it.value()); @@ -550,6 +596,10 @@ bool Config::saveSavedQueries() const object.insert(QStringLiteral("pinned"), true); if (!query.account.isEmpty()) object.insert(QStringLiteral("account"), query.account); + if (query.isGenerated()) + object.insert(QStringLiteral("generated"), query.generated); + if (query.flat) + object.insert(QStringLiteral("flat"), true); for (auto it = query.unknown.begin(); it != query.unknown.end(); ++it) object.insert(it.key(), it.value()); array.append(object); @@ -572,6 +622,18 @@ bool Config::saveSavedQueries() const QString Config::resolvedQuery(const SavedQuery &query) const { + // Composed from the accounts every time it is asked for, which is the + // point: the answer follows the config rather than a copy of it taken when + // the entry was written. + if (query.isGenerated()) { + if (query.generated == QStringLiteral("sent")) + return allSentQuery(); + // An unknown generator was reported on load. Empty rather than the + // bare stored query, which for a generated entry is empty anyway and + // would otherwise run as "match everything". + return QString(); + } + if (query.account.isEmpty()) return query.query; diff --git a/src/config.h b/src/config.h index 4211b0c..b9ee9d6 100644 --- a/src/config.h +++ b/src/config.h @@ -122,6 +122,27 @@ struct SavedQuery /// user edits it. Resolve through Config::resolvedQuery(). QString account; + /// Names a builtin that COMPOSES this query from the accounts at run time, + /// rather than storing it. Empty for an ordinary query. + /// + /// "sent" is the only one today. Its query is built from every account's + /// `sent` key, so adding an account or correcting a folder name is a config + /// edit and nothing else; a stored copy of the same string would go stale + /// silently. That property is why Sent used to be hardcoded beside the + /// saved queries instead of living with them, which left one button on the + /// row that could not be reordered, renamed, unpinned or removed. + /// + /// Storing the GENERATOR rather than its output keeps both: the query stays + /// live, and the entry is an ordinary row the user owns. + QString generated; + + /// Lists messages rather than threads. Set for the sent view, where a + /// thread would fold every reply back into the conversation the user sent + /// one message into. + bool flat = false; + + bool isGenerated() const { return !generated.isEmpty(); } + /// Keys this build does not understand, preserved verbatim so a file /// written by a later version survives a save from this one. QJsonObject unknown; diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index f06b308..62f4751 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1673,40 +1673,34 @@ void MainWindow::buildSavedQueryRow(QWidget *parent, QVBoxLayout *layout) auto *box = new QHBoxLayout(row); box->setContentsMargins(0, 0, 0, 0); + // Sent is an ordinary row here, not a hardcoded button beside the others. + // It is still GENERATED, so its query is composed from the accounts' `sent` + // keys at click time and correcting a folder name stays a config edit and + // nothing else; what changed is that the entry can now be reordered, + // renamed, unpinned or removed like every other, instead of being the one + // control on the row the user did not own. QList unpinned; for (const SavedQuery &saved : m_config.savedQueries()) { + // A generator whose accounts configure nothing produces a button that + // always finds nothing. Skipped entirely, which is what the hardcoded + // Sent button did and is worth keeping. + if (saved.isGenerated() && m_config.resolvedQuery(saved).isEmpty()) + continue; + if (!saved.pinned) { unpinned.append(saved); continue; } auto *button = new QPushButton(saved.name, row); + // The generated entries keep a stable object name so a test can find + // the sent button without depending on what the user renamed it to. + if (saved.generated == QStringLiteral("sent")) + button->setObjectName(QStringLiteral("sentButton")); connect(button, &QPushButton::clicked, this, [this, saved]() { runSavedQuery(saved); }); box->addWidget(button); } - // Sent sits with the saved queries and is not one: its query is COMPOSED - // from the accounts' `sent` keys at click time, so adding an account or - // correcting a folder name is a config edit and nothing else. A stored - // entry holding the same string would go stale silently, and could not - // narrow to the selected account the way this does through - // runCurrentQuery()'s existing scope wrap. - // - // Hidden entirely when no account configures a sent folder, rather than - // offering a button that always finds nothing. - if (!m_config.allSentQuery().isEmpty()) { - auto *sentButton = new QPushButton(tr("Sent"), row); - sentButton->setObjectName(QStringLiteral("sentButton")); - connect(sentButton, &QPushButton::clicked, this, [this]() { - m_queryEdit->setText(m_config.allSentQuery()); - // Flat for this query only. runCurrentQuery() clears it again for - // anything else, including the same query typed by hand, so the - // flag cannot outlive the button that set it. - runQuery(FlatResult::Yes); - }); - box->addWidget(sentButton); - } - // Everything above is left-aligned; the stretch here pushes what follows // to the right edge. The buttons are the row's content and read as a set, // while the overflow menu is a control over that set, so it sits apart @@ -1751,8 +1745,16 @@ void MainWindow::runSavedQuery(const SavedQuery &saved) if (index >= 0) m_accountBox->setCurrentIndex(index); - m_queryEdit->setText(saved.query); - runCurrentQuery(); + // A generated entry has no stored query: the text is composed from the + // accounts now, so what lands in the bar is what actually ran and the user + // can see and edit it. + m_queryEdit->setText(saved.isGenerated() ? m_config.resolvedQuery(saved) + : saved.query); + + // Flat for this query only. runQuery() sets the mode on EVERY run, so the + // flag cannot outlive the entry that asked for it, including for the same + // query typed by hand afterwards. + runQuery(saved.flat ? FlatResult::Yes : FlatResult::No); } void MainWindow::saveCurrentQuery() diff --git a/tests/test_config.cpp b/tests/test_config.cpp index 8024728..dfe463b 100644 --- a/tests/test_config.cpp +++ b/tests/test_config.cpp @@ -62,6 +62,11 @@ private slots: void futureVersionIsRefusedAndReported(); void startupQueryFallsBackToDocumentOrder(); void scopedSavedQueryParenthesisesADisjunction(); + void aGeneratedQueryResolvesFromTheAccounts(); + void aGeneratedQueryTracksAConfigChange(); + void anUnknownGeneratorResolvesToNothingAndReports(); + void migrationAddsSentWhenAnAccountHasOne(); + void migrationAddsNoSentWithoutTheKey(); void generalSectionKeysAreActuallyRead(); void messageZoomDefaultsAndValidates(); void messageZoomOutOfRangeIsReported(); @@ -1416,5 +1421,156 @@ void TestConfig::scopedSavedQueryParenthesisesADisjunction() 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 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 queries = config.savedQueries(); + QCOMPARE(queries.size(), 1); + QCOMPARE(queries.at(0).name, QStringLiteral("Inbox")); +} + QTEST_MAIN(TestConfig) #include "test_config.moc" diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 42d7d78..883e9a7 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -200,6 +200,9 @@ private slots: void thereIsASaveButtonBesideTheQueryBar(); void theMenuIsRightAlignedAwayFromTheButtons(); void theRowSurvivesWithNothingButUnpinnedQueries(); + void aStoredGeneratedQueryRunsFlatAndComposed(); + void aRenamedSentEntryKeepsWorking(); + void aGeneratedQueryWithNothingToShowIsSkipped(); private: /// Owns the throwaway lock table init() points every test at. A pointer @@ -5664,4 +5667,112 @@ void TestMainWindow::theRowSurvivesWithNothingButUnpinnedQueries() QCOMPARE(menuButton->menu()->actions().size(), 2); } +static QString oneAccountWithSent() +{ + return QStringLiteral( + "[account.work]\n" + "name=Test User\n" + "address=user@example.org\n" + "maildir=work-mail\n" + "sent=Sent\n" + ); +} + +/// The existing Sent tests reach the generated entry through MIGRATION, since +/// their configs have no queries.json. This one starts from a stored file, so +/// it covers the path a user is on from the second launch onwards. +void TestMainWindow::aStoredGeneratedQueryRunsFlatAndComposed() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Sent", "generated": "sent", "pinned": true } + ] + })"), oneAccountWithSent()); + + MainWindow window(config); + auto *button = + window.findChild(QStringLiteral("sentButton")); + QVERIFY(button); + + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + QVERIFY(queryEdit); + auto *model = window.findChild(); + QVERIFY(model); + QVERIFY2(!model->flatMode(), "the model starts threaded"); + + button->click(); + + // Composed from the account, not read from the file: the entry stores no + // query at all. + QCOMPARE(queryEdit->text(), config.allSentQuery()); + QVERIFY(queryEdit->text().contains( + QStringLiteral("path:\"work-mail/Sent/**\""))); + QVERIFY2(model->flatMode(), + "a sent view must be flat, or replies fold back into the thread"); +} + +/// The point of the change: Sent is the user's row now. Renaming it must not +/// break it, which it would if anything keyed on the literal name "Sent". +void TestMainWindow::aRenamedSentEntryKeepsWorking() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Posta inviata", "generated": "sent", "pinned": true } + ] + })"), oneAccountWithSent()); + + MainWindow window(config); + const QStringList labels = savedQueryButtonLabels(window); + QCOMPARE(labels, QStringList{ QStringLiteral("Posta inviata") }); + + auto *button = + window.findChild(QStringLiteral("sentButton")); + QVERIFY2(button, "the generated entry lost its identity when renamed"); + + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + button->click(); + QCOMPARE(queryEdit->text(), config.allSentQuery()); +} + +/// The hardcoded button was hidden entirely when no account configured a sent +/// folder, rather than offering one that always finds nothing. A stored row +/// must behave the same way. +void TestMainWindow::aGeneratedQueryWithNothingToShowIsSkipped() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Inbox", "query": "tag:inbox", "pinned": true }, + { "name": "Sent", "generated": "sent", "pinned": true } + ] + })"), QStringLiteral( + "[account.work]\n" + "name=Test User\n" + "address=user@example.org\n" + "maildir=work-mail\n" + )); + + MainWindow window(config); + + // The guard: the row was built and the other entry did get a button, so a + // missing Sent means it was skipped rather than that nothing was built. + QCOMPARE(savedQueryButtonLabels(window), + QStringList{ QStringLiteral("Inbox") }); + QVERIFY2(!window.findChild(QStringLiteral("sentButton")), + "a generated query with nothing to show must not get a button"); +} + #include "test_mainwindow.moc" -- cgit v1.2.3 From a7872cf56b7f313881f0d5e50d548ffdb5ba68b9 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 20:07:31 +0200 Subject: feat(queries): edit, pin and delete a saved query from the UI Item 82. Saving a query worked and nothing else did: changing one field meant retyping the whole query under the same name, and deleting one meant editing the file by hand. An action that creates something the UI cannot then change or remove is incomplete, and the user hit it within minutes of the first hand test. Right-clicking a saved query, on its button or its menu entry, now offers Edit, Move to menu / Show as a button, and Delete. Every path funnels through one replaceSavedQuery(), which matches on the name the dialog was OPENED with rather than the one it returns, so a rename replaces the entry instead of leaving the original behind beside a new one, and which merges the stored entry's unknown fields in a single place rather than in three. Delete confirms first: the rule against confirmation dialogs covers tag mutations, which the undo stack can take back, and this writes user config that it cannot. Two cases the item did not anticipate. A generated entry has no query to edit, so the dialog shows its composed query read-only rather than offering a field that changes nothing, and carries `generated` and `flat` through an edit rather than letting it decay into a plain entry holding a snapshot of what it resolved to today. And the overwrite notice had to learn to ignore the entry being edited, since warning that "Inbox" already exists while editing Inbox is noise. This also fixes a defect that predated it and was already reachable from the save path. rebuildSavedQueryRow() called deleteLater() on the old row, which defers destruction to the event loop, so the stale row went on answering findChild() and every lookup after a rebuild reported the state from before the edit. Nothing looked wrong on screen, which is why it surfaced only as three tests failing against a row that had in fact been rebuilt correctly. Five tests, three mutations. Matching on the returned name fails two, never writing the file fails three, and dropping the unknown-field merge fails one. That last one initially proved nothing: it drove UNPIN, which copies the stored entry and so carries `unknown` along by itself, and passed with the merge deleted. It now goes through the edit path with a replacement that has none, which is what the dialog actually returns. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 6 + README.md | 6 + .../plans/2026-08-03-post-0.1.0-usability.md | 26 ++- src/mainwindow.cpp | 113 ++++++++++++ src/mainwindow.h | 36 ++++ src/savequerydialog.cpp | 60 ++++++- src/savequerydialog.h | 16 ++ tests/test_mainwindow.cpp | 200 +++++++++++++++++++++ 8 files changed, 455 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b384eda..5b14938 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,12 @@ point at which they are stable. it can now be reordered, renamed, unpinned or deleted like any other entry rather than being a fixed button you did not own. +- **Right-click a saved query** to edit, pin, unpin or delete it. A saved query + could previously be created and never changed: the only route to adjusting one + field was to retype the whole query under the same name, and there was no way + to delete one at all short of editing the file (item 82). Deleting asks first, + since it writes your config and is not undoable. + ### Changed - Saved queries have a **row of their own** beneath the query bar rather than diff --git a/README.md b/README.md index 749d89d..9148923 100644 --- a/README.md +++ b/README.md @@ -244,6 +244,12 @@ sent folder, rather than offering a button that finds nothing. The name is what the button says, so `Important` and `Flagged` can run the same query and differ only in the label. +**Right-click a saved query** (a button, or its entry in the menu) to edit it, +move it between the row and the menu, or delete it. Deleting asks first: it +rewrites this file and there is no undo for it. Editing a generated entry shows +its composed query read-only, since that one is built from your accounts rather +than stored. + **Upgrading from 0.17.0 or earlier.** Saved queries used to live in a `[queries]` section of `qtmaildir.conf`. The first launch after upgrading reads that section, writes `queries.json` from it, and marks every entry pinned so diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index 9d19e4d..5e97337 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -146,7 +146,7 @@ taking that too literally. | 80 | A rule with many conditions squeezes the rule list to one visible row | defect | XS | **done** 2026-08-13 on `rule-builder`, unreleased. Follows item 76 | | 79 | Opening the rules dialog and saving destroys the first rule | defect | XS | **fixed on `rule-builder`** 2026-08-13, unreleased. Shipped in 0.16.0; damaged one real rule, repaired by hand | | 81 | No way to turn a saved query into a tagging rule | workflow | S | open; depends on 23, which builds the dialog, and is excluded from its spec on purpose. Writes to the shared rules file, so it spans this repo and `mailctl` | -| 82 | A saved query cannot be edited, unpinned or deleted from the UI | defect | S | open; found by hand-testing item 23 on 2026-08-13. Saving works, unsaving does not | +| 82 | A saved query cannot be edited, unpinned or deleted from the UI | defect | S | **done** 2026-08-13 on `saved-queries`, unreleased. Right-click offers Edit, Pin/Unpin and Delete | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -618,6 +618,30 @@ queries** entry, offering Edit, Unpin (or Pin) and Delete. **Size: S**, and it should land before the saved-query work is called done. +**Done 2026-08-13.** A context menu on each button and each menu entry, with +Edit, Move to menu / Show as a button, and Delete. Every path goes through one +`replaceSavedQuery()`, matched on the name the dialog was OPENED with, so a +rename replaces rather than duplicating, and merging the stored entry's unknown +fields in one place rather than three. + +Two things the approach above did not anticipate. A GENERATED entry has no +query to edit, so the dialog shows its composed query read-only rather than +offering a field that changes nothing, and carries `generated` and `flat` +through an edit rather than letting it decay into a plain entry holding a +snapshot. And the overwrite notice had to learn to ignore the entry being +edited: warning that "Inbox" already exists while editing Inbox is noise. + +It also exposed a defect that predated it. `rebuildSavedQueryRow()` called +`deleteLater()` on the old row, which defers destruction to the event loop, so +the stale row went on answering `findChild()` and every lookup after a rebuild +saw the state from before the edit. It was already reachable from the save path. +Fixed by reparenting the row out immediately. + +The unknown-fields test initially passed against the merge being deleted: it +drove UNPIN, which copies the stored entry and therefore carries `unknown` +along by itself. It now goes through the edit path with a replacement that has +none, which is what the dialog actually returns. + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 62f4751..14e4202 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1698,6 +1698,7 @@ void MainWindow::buildSavedQueryRow(QWidget *parent, QVBoxLayout *layout) button->setObjectName(QStringLiteral("sentButton")); connect(button, &QPushButton::clicked, this, [this, saved]() { runSavedQuery(saved); }); + addSavedQueryActions(button, saved); box->addWidget(button); } @@ -1718,6 +1719,12 @@ void MainWindow::buildSavedQueryRow(QWidget *parent, QVBoxLayout *layout) QAction *action = menu->addAction(saved.name); connect(action, &QAction::triggered, this, [this, saved]() { runSavedQuery(saved); }); + // A menu entry has no context menu of its own, so its own submenu + // carries the same three actions; an unpinned query would + // otherwise be the one thing that cannot be edited or deleted. + auto *entryMenu = new QMenu(menu); + addSavedQueryActions(entryMenu, saved); + action->setMenu(entryMenu); } menuButton->setMenu(menu); box->addWidget(menuButton); @@ -1733,6 +1740,105 @@ void MainWindow::buildSavedQueryRow(QWidget *parent, QVBoxLayout *layout) row->hide(); } +void MainWindow::addSavedQueryActions(QWidget *target, const SavedQuery &saved) +{ + target->setContextMenuPolicy(Qt::ActionsContextMenu); + + auto *edit = new QAction(tr("Edit..."), target); + edit->setObjectName(QStringLiteral("editQuery")); + connect(edit, &QAction::triggered, this, + [this, saved]() { editSavedQuery(saved); }); + target->addAction(edit); + + auto *pin = new QAction(saved.pinned ? tr("Move to menu") + : tr("Show as a button"), + target); + pin->setObjectName(QStringLiteral("pinQuery")); + connect(pin, &QAction::triggered, this, [this, saved]() { + SavedQuery toggled = saved; + toggled.pinned = !saved.pinned; + replaceSavedQuery(saved.name, toggled); + }); + target->addAction(pin); + + auto *separator = new QAction(target); + separator->setSeparator(true); + target->addAction(separator); + + auto *remove = new QAction(tr("Delete"), target); + remove->setObjectName(QStringLiteral("deleteQuery")); + connect(remove, &QAction::triggered, this, + [this, saved]() { deleteSavedQuery(saved); }); + target->addAction(remove); +} + +void MainWindow::editSavedQuery(const SavedQuery &saved) +{ + SaveQueryDialog dialog(m_config, saved, this); + if (dialog.exec() != QDialog::Accepted) + return; + + // Matched on the name the dialog OPENED with. Using the returned name would + // leave the original entry in place and add a second one under the new + // name, which is a duplicate rather than a rename. + replaceSavedQuery(saved.name, dialog.savedQuery()); +} + +void MainWindow::deleteSavedQuery(const SavedQuery &saved) +{ + // One of the few places in this application that confirms. The rule against + // confirmation dialogs covers tag mutations, which are undoable through the + // undo stack; this writes user config, is not on that stack, and cannot be + // taken back. + if (m_confirmDelete) { + const auto answer = QMessageBox::question( + this, tr("Delete saved query"), + tr("Delete the saved query '%1'?").arg(saved.name), + QMessageBox::Yes | QMessageBox::No, QMessageBox::No); + if (answer != QMessageBox::Yes) + return; + } + + replaceSavedQuery(saved.name, SavedQuery()); +} + +void MainWindow::replaceSavedQuery(const QString &originalName, + const SavedQuery &replacement) +{ + QList queries = m_config.savedQueries(); + const bool removing = replacement.name.isEmpty(); + + for (int i = 0; i < queries.size(); ++i) { + if (queries.at(i).name.compare(originalName, Qt::CaseInsensitive) != 0) + continue; + + if (removing) { + queries.removeAt(i); + } else { + // The unknown fields belong to the STORED entry: a field written by + // a later build survives an edit made here rather than being + // dropped on the next save. + SavedQuery merged = replacement; + merged.unknown = queries.at(i).unknown; + queries[i] = merged; + } + break; + } + + m_config.setSavedQueries(queries); + if (!m_config.saveSavedQueries()) { + QMessageBox::warning(this, tr("Saved queries"), + tr("Could not write the saved queries file.")); + return; + } + + rebuildSavedQueryRow(); + statusBar()->showMessage( + removing ? tr("Deleted saved query '%1'.").arg(originalName) + : tr("Updated saved query '%1'.").arg(replacement.name), + kStatusMessageMs); +} + void MainWindow::runSavedQuery(const SavedQuery &saved) { // Through the dropdown, never by pre-scoping the text: runQuery() applies @@ -1817,6 +1923,13 @@ void MainWindow::rebuildSavedQueryRow() const int index = layout->indexOf(old); layout->removeWidget(old); + // Reparented out NOW, not merely scheduled for deletion. deleteLater() + // defers destruction to the event loop, so the old row goes on answering + // findChild() until it runs, and findChild returns the FIRST match: every + // lookup after a rebuild found the stale row and reported the state from + // before the edit. Nothing visible was wrong, which is why this only + // showed up as three tests failing on a row that had in fact been rebuilt. + old->setParent(nullptr); old->deleteLater(); buildSavedQueryRow(centralWidget(), layout); diff --git a/src/mainwindow.h b/src/mainwindow.h index e6ee322..6e8501a 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -125,6 +125,25 @@ public: static void setLocksPathForTesting(const QString &path); static QString locksPath(); + /// Suppresses the delete confirmation. + /// + /// A test seam. Deleting a saved query is destructive and not on the undo + /// stack, so it asks first; a test cannot answer a modal dialog without + /// hanging, and driving one through QTest would assert the dialog rather + /// than the deletion. + void setConfirmDeleteForTesting(bool confirm) { m_confirmDelete = confirm; } + + /// Renames or replaces a stored query, as the edit dialog would on accept. + /// + /// A test seam for the rename path specifically: the dialog is modal, and + /// the property worth asserting is that a rename REPLACES rather than + /// duplicating, which is decided after the dialog returns. + void replaceSavedQueryForTesting(const QString &originalName, + const SavedQuery &replacement) + { + replaceSavedQuery(originalName, replacement); + } + /// How many commands are on the undo stack. /// /// A test seam. The undo QAction is always enabled and checks canUndo() @@ -253,6 +272,21 @@ private: /// Rebuilds the saved-query row in place after the stored list changed. void rebuildSavedQueryRow(); + /// Hangs Edit, Pin/Unpin and Delete on a saved query's button or menu + /// entry. The only route to changing a stored query from the UI. + void addSavedQueryActions(QWidget *target, const SavedQuery &saved); + + /// Replaces the entry named `originalName`, writes the file and rebuilds + /// the row. An empty `replacement.name` deletes it instead. + /// + /// Matched on the ORIGINAL name, not the replacement's: a rename otherwise + /// leaves the old entry in place and adds a second one. + void replaceSavedQuery(const QString &originalName, + const SavedQuery &replacement); + + void editSavedQuery(const SavedQuery &saved); + void deleteSavedQuery(const SavedQuery &saved); + private slots: void runCurrentQuery() { runQuery(FlatResult::No); } @@ -628,6 +662,8 @@ private: QLineEdit *m_queryEdit = nullptr; /// Save query, beside the field. Driven by the save_query action. QToolButton *m_saveQueryButton = nullptr; + /// Whether deleting a saved query asks first. Always true outside tests. + bool m_confirmDelete = true; QueryCompleter *m_queryCompleter = nullptr; /// Its own type, not the QTreeView base. The strip painting and the /// expander column are ThreadListView's, and holding the base here only diff --git a/src/savequerydialog.cpp b/src/savequerydialog.cpp index 1ea97c2..2d09986 100644 --- a/src/savequerydialog.cpp +++ b/src/savequerydialog.cpp @@ -42,19 +42,51 @@ SaveQueryDialog::SaveQueryDialog(const Config &config, const QString &query, : QDialog(parent) , m_config(config) { + SavedQuery initial; + initial.query = query; + initial.account = accountKey; + initial.pinned = true; setWindowTitle(tr("Save query")); + build(initial); +} +SaveQueryDialog::SaveQueryDialog(const Config &config, + const SavedQuery &existing, QWidget *parent) + : QDialog(parent) + , m_config(config) + , m_originalName(existing.name) + , m_generated(existing.generated) + , m_flat(existing.flat) +{ + setWindowTitle(tr("Edit saved query")); + build(existing); +} + +void SaveQueryDialog::build(const SavedQuery &initial) +{ auto *layout = new QVBoxLayout(this); auto *form = new QFormLayout; - m_name = new QLineEdit(this); + m_name = new QLineEdit(initial.name, this); m_name->setObjectName(QStringLiteral("saveQueryName")); m_name->setPlaceholderText(tr("A name for this query")); form->addRow(tr("Name"), m_name); - m_query = new QLineEdit(query, this); + m_query = new QLineEdit(initial.query, this); m_query->setObjectName(QStringLiteral("saveQueryQuery")); - form->addRow(tr("Query"), m_query); + if (initial.isGenerated()) { + // A generated entry has no stored query: it is composed from the + // accounts every time it runs. Shown, so the user can see what it will + // do, but read-only, since editing it would change nothing. + m_query->setText(m_config.resolvedQuery(initial)); + m_query->setReadOnly(true); + m_query->setToolTip(tr("Built from your accounts and not editable. " + "It follows the sent folder each account " + "configures.")); + form->addRow(tr("Query"), m_query); + } else { + form->addRow(tr("Query"), m_query); + } // The scope is stored as an account KEY, so the entries carry the key as // data exactly as the main window's dropdown does. "All accounts" is the @@ -63,15 +95,15 @@ SaveQueryDialog::SaveQueryDialog(const Config &config, const QString &query, m_account = new QComboBox(this); m_account->setObjectName(QStringLiteral("saveQueryAccount")); m_account->addItem(tr("All accounts"), QString()); - for (const Account &account : config.accounts()) + for (const Account &account : m_config.accounts()) m_account->addItem(account.key, account.key); - const int index = m_account->findData(accountKey); + const int index = m_account->findData(initial.account); m_account->setCurrentIndex(index >= 0 ? index : 0); form->addRow(tr("Account"), m_account); m_pinned = new QCheckBox(tr("Show as a button"), this); m_pinned->setObjectName(QStringLiteral("saveQueryPinned")); - m_pinned->setChecked(true); + m_pinned->setChecked(initial.pinned); form->addRow(QString(), m_pinned); layout->addLayout(form); @@ -108,7 +140,14 @@ void SaveQueryDialog::updateOkState() const bool usable = !name.isEmpty() && !m_query->text().trimmed().isEmpty(); m_ok->setEnabled(usable); - if (!name.isEmpty() && namesAnExistingQuery(m_config, name)) { + // Ignores the entry being edited: warning that "Inbox" already exists + // while editing Inbox is noise, and the real case worth catching is a + // rename onto a name something else already holds. + const bool isItsOwnName = + !m_originalName.isEmpty() + && name.compare(m_originalName, Qt::CaseInsensitive) == 0; + if (!name.isEmpty() && !isItsOwnName + && namesAnExistingQuery(m_config, name)) { m_notice->setText( tr("A saved query named '%1' already exists and will be " "replaced.").arg(name)); @@ -124,5 +163,12 @@ SavedQuery SaveQueryDialog::savedQuery() const saved.query = m_query->text().trimmed(); saved.account = m_account->currentData().toString(); saved.pinned = m_pinned->isChecked(); + // Carried through rather than re-derived: an edit must not turn a + // generated entry into a plain one holding a snapshot of what it happened + // to resolve to today. + saved.generated = m_generated; + saved.flat = m_flat; + if (saved.isGenerated()) + saved.query.clear(); return saved; } diff --git a/src/savequerydialog.h b/src/savequerydialog.h index be5a2af..d859254 100644 --- a/src/savequerydialog.h +++ b/src/savequerydialog.h @@ -42,6 +42,11 @@ public: SaveQueryDialog(const Config &config, const QString &query, const QString &accountKey, QWidget *parent = nullptr); + /// Edits an entry that already exists, prefilled from it rather than from + /// the query bar. + SaveQueryDialog(const Config &config, const SavedQuery &existing, + QWidget *parent = nullptr); + /// The query as edited. Only meaningful after exec() returned Accepted. SavedQuery savedQuery() const; @@ -51,8 +56,19 @@ public: static bool namesAnExistingQuery(const Config &config, const QString &name); private: + void build(const SavedQuery &initial); void updateOkState(); + /// The name the dialog was opened on, empty when creating. The caller + /// matches on this rather than on the returned name, so a rename replaces + /// the entry instead of adding a second one beside it. + QString m_originalName; + + /// Set for a generated entry, whose query is composed from the accounts + /// and cannot be edited here. + QString m_generated; + bool m_flat = false; + const Config &m_config; QLineEdit *m_name = nullptr; QLineEdit *m_query = nullptr; diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 883e9a7..d090016 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -203,6 +203,11 @@ private slots: void aStoredGeneratedQueryRunsFlatAndComposed(); void aRenamedSentEntryKeepsWorking(); void aGeneratedQueryWithNothingToShowIsSkipped(); + void aSavedQueryButtonOffersEditUnpinAndDelete(); + void unpinningMovesAQueryToTheMenu(); + void deletingRemovesTheQueryFromTheFile(); + void anEditedQueryKeepsItsUnknownFields(); + void renamingReplacesRatherThanDuplicating(); private: /// Owns the throwaway lock table init() points every test at. A pointer @@ -5775,4 +5780,199 @@ void TestMainWindow::aGeneratedQueryWithNothingToShowIsSkipped() "a generated query with nothing to show must not get a button"); } +/// Reads queries.json back from disk, which is what "it was saved" means. +static QJsonArray storedQueries(const QTemporaryDir &dir) +{ + QFile f(dir.filePath(QStringLiteral("qtmaildir/queries.json"))); + if (!f.open(QIODevice::ReadOnly)) + return {}; + const QJsonObject root = QJsonDocument::fromJson(f.readAll()).object(); + return root.value(QStringLiteral("queries")).toArray(); +} + +static QAction *contextActionNamed(MainWindow &window, QWidget *target, + const QString &objectName) +{ + const QList actions = target->actions(); + for (QAction *action : actions) { + if (action->objectName() == objectName) + return action; + } + Q_UNUSED(window); + return nullptr; +} + +void TestMainWindow::aSavedQueryButtonOffersEditUnpinAndDelete() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Inbox", "query": "tag:inbox", "pinned": true } + ] + })")); + + MainWindow window(config); + auto *row = window.findChild(QStringLiteral("savedQueryRow")); + QVERIFY(row); + auto *button = row->findChild(); + QVERIFY(button); + + // A context menu, so the actions live on the widget itself. + QCOMPARE(button->contextMenuPolicy(), Qt::ActionsContextMenu); + QVERIFY(contextActionNamed(window, button, QStringLiteral("editQuery"))); + QVERIFY(contextActionNamed(window, button, QStringLiteral("pinQuery"))); + QVERIFY(contextActionNamed(window, button, QStringLiteral("deleteQuery"))); +} + +void TestMainWindow::unpinningMovesAQueryToTheMenu() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Inbox", "query": "tag:inbox", "pinned": true }, + { "name": "Other", "query": "tag:other", "pinned": true } + ] + })")); + + MainWindow window(config); + auto *row = window.findChild(QStringLiteral("savedQueryRow")); + QVERIFY(row); + QCOMPARE(savedQueryButtonLabels(window).size(), 2); + QVERIFY(!window.findChild( + QStringLiteral("savedQueryMenuButton"))); + + auto *button = row->findChild(); + QVERIFY(button); + QAction *pin = contextActionNamed(window, button, QStringLiteral("pinQuery")); + QVERIFY(pin); + pin->trigger(); + + // Off the row, into the menu, and written to the file: an unpin that only + // redrew would come back pinned on the next launch. + QCOMPARE(savedQueryButtonLabels(window), QStringList{ QStringLiteral("Other") }); + auto *menuButton = + window.findChild(QStringLiteral("savedQueryMenuButton")); + QVERIFY(menuButton); + QCOMPARE(menuButton->menu()->actions().size(), 1); + + const QJsonArray stored = storedQueries(dir); + QCOMPARE(stored.size(), 2); + QCOMPARE(stored.at(0).toObject().value(QStringLiteral("name")).toString(), + QStringLiteral("Inbox")); + QVERIFY2(!stored.at(0).toObject().contains(QStringLiteral("pinned")), + "the unpin did not reach the file"); +} + +void TestMainWindow::deletingRemovesTheQueryFromTheFile() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Doomed", "query": "tag:doomed", "pinned": true }, + { "name": "Keeper", "query": "tag:keeper", "pinned": true } + ] + })")); + + MainWindow window(config); + auto *row = window.findChild(QStringLiteral("savedQueryRow")); + QVERIFY(row); + auto *button = row->findChild(); + QVERIFY(button); + QCOMPARE(button->text(), QStringLiteral("Doomed")); + + QAction *del = + contextActionNamed(window, button, QStringLiteral("deleteQuery")); + QVERIFY(del); + // Destructive and not on the undo stack, so it confirms. Suppressed here + // rather than driven through the modal dialog, which would hang the test. + window.setConfirmDeleteForTesting(false); + del->trigger(); + + QCOMPARE(savedQueryButtonLabels(window), + QStringList{ QStringLiteral("Keeper") }); + + const QJsonArray stored = storedQueries(dir); + QCOMPARE(stored.size(), 1); + QCOMPARE(stored.at(0).toObject().value(QStringLiteral("name")).toString(), + QStringLiteral("Keeper")); +} + +/// A field a later build wrote must survive an edit here, or upgrading and +/// downgrading silently strips config the user set. +void TestMainWindow::anEditedQueryKeepsItsUnknownFields() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Inbox", "query": "tag:inbox", "pinned": true, + "icon": "mail-inbox" } + ] + })")); + + MainWindow window(config); + + // Through the EDIT path, with a replacement carrying no unknown fields of + // its own, which is exactly what SaveQueryDialog returns. Driving this + // through unpin instead proved nothing: unpin copies the stored entry, so + // it carries `unknown` along by itself and the merge is never exercised. + // That version passed with the merge deleted. + SavedQuery edited; + edited.name = QStringLiteral("Inbox"); + edited.query = QStringLiteral("tag:inbox and not tag:muted"); + edited.pinned = true; + QVERIFY(edited.unknown.isEmpty()); + window.replaceSavedQueryForTesting(QStringLiteral("Inbox"), edited); + + const QJsonArray stored = storedQueries(dir); + QCOMPARE(stored.size(), 1); + const QJsonObject entry = stored.at(0).toObject(); + // The edit landed... + QCOMPARE(entry.value(QStringLiteral("query")).toString(), + QStringLiteral("tag:inbox and not tag:muted")); + // ...and did not take the unknown field down with it. + QCOMPARE(entry.value(QStringLiteral("icon")).toString(), + QStringLiteral("mail-inbox")); +} + +/// Renaming must match on the name the dialog OPENED with. Matching on the +/// returned name leaves the original in place and adds a second entry. +void TestMainWindow::renamingReplacesRatherThanDuplicating() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Old", "query": "tag:old", "pinned": true } + ] + })")); + + MainWindow window(config); + + SavedQuery renamed; + renamed.name = QStringLiteral("New"); + renamed.query = QStringLiteral("tag:old"); + renamed.pinned = true; + window.replaceSavedQueryForTesting(QStringLiteral("Old"), renamed); + + const QJsonArray stored = storedQueries(dir); + QCOMPARE(stored.size(), 1); + QCOMPARE(stored.at(0).toObject().value(QStringLiteral("name")).toString(), + QStringLiteral("New")); + QCOMPARE(savedQueryButtonLabels(window), QStringList{ QStringLiteral("New") }); +} + #include "test_mainwindow.moc" -- cgit v1.2.3 From 9be1b13b91188cf44a40c6786a83de034988cdbd Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Thu, 13 Aug 2026 20:14:25 +0200 Subject: fix(queries): stop writing keys that carry no information Saving a generated entry wrote `"query": ""` and `"flat": true` alongside its generator. Both reload correctly, so nothing was broken, but queries.json is meant to be hand-edited and each redundant key is one more thing to read past. A generated entry has no query of its own, and the sent generator already implies flat. Written now only when they say something, which is the rule `pinned` and `account` already followed: `query` is skipped for a generated entry in favour of `generated`, and `flat` is skipped when the generator implies it. Omitting `flat` is only safe because loadSavedQueries() reapplies it from the generator, so the two are coupled: the mutation that stops reapplying it fails this test and one other, in both suites. That is deliberate, since a round-trip test can otherwise pass while quietly writing less than it reads. Co-Authored-By: Claude Opus 5 --- src/config.cpp | 16 ++++++++++++---- tests/test_config.cpp | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/src/config.cpp b/src/config.cpp index 9edba50..ae26555 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -589,16 +589,24 @@ bool Config::saveSavedQueries() const QJsonArray array; for (const SavedQuery &query : m_savedQueries) { + // Only what carries information. The file is hand-editable, so a key + // that always holds the same value, or one the generator already + // implies, is just something the reader has to skip past. Same reason + // `pinned` and `account` are written only when set. QJsonObject object; object.insert(QStringLiteral("name"), query.name); - object.insert(QStringLiteral("query"), query.query); + if (query.isGenerated()) { + object.insert(QStringLiteral("generated"), query.generated); + } else { + object.insert(QStringLiteral("query"), query.query); + } if (query.pinned) object.insert(QStringLiteral("pinned"), true); if (!query.account.isEmpty()) object.insert(QStringLiteral("account"), query.account); - if (query.isGenerated()) - object.insert(QStringLiteral("generated"), query.generated); - if (query.flat) + // Skipped when the generator already implies it, which loadSavedQueries + // reapplies on the way back in. + if (query.flat && query.generated != QStringLiteral("sent")) object.insert(QStringLiteral("flat"), true); for (auto it = query.unknown.begin(); it != query.unknown.end(); ++it) object.insert(it.key(), it.value()); diff --git a/tests/test_config.cpp b/tests/test_config.cpp index dfe463b..3b094ba 100644 --- a/tests/test_config.cpp +++ b/tests/test_config.cpp @@ -67,6 +67,7 @@ private slots: void anUnknownGeneratorResolvesToNothingAndReports(); void migrationAddsSentWhenAnAccountHasOne(); void migrationAddsNoSentWithoutTheKey(); + void aGeneratedEntryWritesNoRedundantKeys(); void generalSectionKeysAreActuallyRead(); void messageZoomDefaultsAndValidates(); void messageZoomOutOfRangeIsReported(); @@ -1572,5 +1573,54 @@ void TestConfig::migrationAddsNoSentWithoutTheKey() 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" -- cgit v1.2.3