From de0d3aaa63be2867d127304d55d34259eb6e30d6 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 15 Aug 2026 10:50:59 +0200 Subject: feat(filters): resolve built-in filters per account Item 93, the Config half. Four built-in filters, Unread, Inbox, Flagged and Sent, as generated entries in kQueryGenerators, which was already a closed set validated on load for Sent alone. resolvedQuery() gains an overload taking an account key, and that is what makes a filter compose with the account dropdown instead of fighting it. A generator is asked for the account's OWN query rather than having its all-accounts query wrapped in a scope: wrapping gives path:"a/**" and (path:"a/Sent/**" or path:"b/Sent/**") which returns the right rows only because path: is hierarchical, so a row-count test passes against it. The tests assert on the query string for that reason, and the mutation putting the wrap back fails two of them. An ordinary saved query ignores the account key and keeps resolving through its own stored account, which is the behaviour item 90 leaves alone. matchNothingQuery() exists because an empty query means "match everything" to notmuch: an account configuring no sent folder would otherwise give a button labelled Sent that shows the entire Maildir. Config gains Q_DECLARE_TR_FUNCTIONS for the filter names, which are button labels. The generator names are not translated: they are matched against the closed set and stored in queries.json, so translating them would make a file written in one locale unreadable in another. No UI yet, and no migration: the query row still builds from pinned saved queries. --- src/config.cpp | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++- src/config.h | 46 ++++++++++++++++++++++ 2 files changed, 166 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/config.cpp b/src/config.cpp index ae26555..0b65053 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -57,7 +57,24 @@ constexpr int kQueriesFormatVersion = 1; /// 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") }; +const QStringList kQueryGenerators = { QStringLiteral("unread"), + QStringLiteral("inbox"), + QStringLiteral("flagged"), + QStringLiteral("sent") }; + +/// The tag a generator matches, for the three filters that are a plain tag +/// query. Empty for "sent", which composes from each account's folder instead +/// and is handled separately. +QString generatorTag(const QString &generator) +{ + if (generator == QStringLiteral("unread")) + return QStringLiteral("unread"); + if (generator == QStringLiteral("inbox")) + return QStringLiteral("inbox"); + if (generator == QStringLiteral("flagged")) + return QStringLiteral("flagged"); + return QString(); +} } // namespace @@ -652,6 +669,108 @@ QString Config::resolvedQuery(const SavedQuery &query) const return scope.scopedQuery(query.query); } +bool Config::isKnownGenerator(const QString &generator) +{ + return kQueryGenerators.contains(generator); +} + +QString Config::matchNothingQuery() +{ + // notmuch reads an EMPTY query as "match everything", so a generator with + // nothing to match must say so explicitly. `tag:` and its negation cannot + // both hold, and the tag name is irrelevant: what matters is that this + // parses and matches nothing. A malformed string would not do, since + // notmuch accepts almost anything and matches nothing quietly, which is the + // same result reached by luck rather than by contract. + return QStringLiteral("tag:unread and not tag:unread"); +} + +QList Config::builtinFilters() +{ + // Left to right on the query row. Fixed rather than configurable: item 94 + // removes the mixed row entirely once these are confirmed, so a settings + // surface for the order would be built and deleted inside two items. + QList filters; + for (const QString &generator : kQueryGenerators) + filters.append(builtinFilter(generator)); + return filters; +} + +SavedQuery Config::builtinFilter(const QString &generator) +{ + if (!isKnownGenerator(generator)) + return {}; + + SavedQuery filter; + filter.generated = generator; + + // Translated, because these are the labels on the buttons. The GENERATOR + // name is not: it is stored in queries.json and matched against a closed + // set, so translating it would make a file written in one locale unreadable + // in another. + if (generator == QStringLiteral("unread")) { + filter.name = tr("Unread"); + } else if (generator == QStringLiteral("inbox")) { + filter.name = tr("Inbox"); + } else if (generator == QStringLiteral("flagged")) { + filter.name = tr("Flagged"); + } else if (generator == QStringLiteral("sent")) { + filter.name = tr("Sent"); + // Messages rather than threads, and the only filter that sets this. A + // thread would fold the user's sent message back into the conversation + // it belongs to, which is item 63's finding. + filter.flat = true; + } + + return filter; +} + +QString Config::resolvedQuery(const SavedQuery &query, + const QString &accountKey) const +{ + // An ordinary saved query is a DESTINATION: it states its own scope and + // ignores the dropdown, which is the behaviour item 90 leaves alone. Only a + // generated filter composes. + if (!query.isGenerated()) + return resolvedQuery(query); + + if (!isKnownGenerator(query.generated)) + return QString(); + + if (accountKey.isEmpty()) { + // Across every account, which for the tag filters is the bare query and + // for Sent is the union of the accounts' folders. + if (query.generated == QStringLiteral("sent")) { + const QString all = allSentQuery(); + return all.isEmpty() ? matchNothingQuery() : all; + } + return QStringLiteral("tag:%1").arg(generatorTag(query.generated)); + } + + const Account scope = account(accountKey); + if (!scope.isValid()) + return resolvedQuery(query, QString()); + + if (query.generated == QStringLiteral("sent")) { + // The account's OWN sent query, never the all-accounts one wrapped in + // this account's path. Wrapping gives + // path:"a/**" and (path:"a/Sent/**" or path:"b/Sent/**") + // which returns the right rows because path: is hierarchical, and is + // still wrong: it double-scopes and works by accident of the syntax. + const QString sent = scope.sentQuery(); + // Empty when the account configures no sent folder, which is a real + // case and not a misconfiguration. Returned as-is it would mean "match + // everything", so a button labelled Sent would show the whole Maildir. + return sent.isEmpty() ? matchNothingQuery() : sent; + } + + // A tag filter carries no path of its own, so scoping is exactly what + // scopedQuery() does. Its parentheses are load-bearing: `path:... and a or + // b` binds as `(path:... and a) or b`. + return scope.scopedQuery( + QStringLiteral("tag:%1").arg(generatorTag(query.generated))); +} + SavedQuery Config::startupSavedQuery() const { if (m_savedQueries.isEmpty()) diff --git a/src/config.h b/src/config.h index b9ee9d6..3946b40 100644 --- a/src/config.h +++ b/src/config.h @@ -19,6 +19,7 @@ #pragma once #include +#include #include #include #include @@ -155,6 +156,11 @@ struct SavedQuery /// allow the GUI to index a different tree than the CLI. class Config { + // Not a QObject: this class is a value holder read from every thread. The + // macro gives it tr() for the built-in filters' NAMES, which are the labels + // on the query row's buttons and therefore user-facing. + Q_DECLARE_TR_FUNCTIONS(Config) + public: /// Path used when load() is called with no argument. static QString defaultPath(); @@ -190,6 +196,46 @@ public: /// than a scope built from an empty maildir, which would be path:"/**". QString resolvedQuery(const SavedQuery &query) const; + /// The query as it should be run in one account's scope, or across all of + /// them when `accountKey` is empty. + /// + /// This is what makes a built-in filter COMPOSE with the account dropdown + /// rather than fight it (item 93). A generator is asked for the account's + /// own query, never handed its all-accounts query to wrap: wrapping gives + /// path:"a/**" and (path:"a/Sent/**" or path:"b/Sent/**") + /// which returns the right rows only because path: is hierarchical, and + /// says something other than what is meant. + /// + /// An ordinary saved query ignores `accountKey` and keeps resolving through + /// its OWN stored account, which is the behaviour item 90 leaves alone: a + /// saved query is a destination and states its own scope. + QString resolvedQuery(const SavedQuery &query, + const QString &accountKey) const; + + /// The built-in filters, in the order they appear on the query row. + /// + /// Shipped rather than stored: these are not the user's saved queries and + /// are not in queries.json at all. The row used to be whatever the user had + /// pinned, which is how it drifted (item 93). + static QList builtinFilters(); + + /// One built-in filter by generator name, or a default-constructed + /// SavedQuery when the name is not one. + static SavedQuery builtinFilter(const QString &generator); + + /// Whether `generator` is one this build knows how to resolve. + /// + /// A closed set, so a typo is reported on load rather than producing a + /// button that silently finds nothing. + static bool isKnownGenerator(const QString &generator); + + /// A query that deliberately matches no message. + /// + /// Needed because an EMPTY query means "match everything" to notmuch, so a + /// generator with nothing to match cannot simply return one: Sent under an + /// account that configures no sent folder would show the entire Maildir. + static QString matchNothingQuery(); + /// Empty when unset; the caller disables the Sync button in that case. QString syncCommand() const { return m_syncCommand; } -- cgit v1.2.3 From 51f04ffc3c17da59b6072c4362029c45b780de82 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 15 Aug 2026 11:00:58 +0200 Subject: feat(filters): put the four built-in filters on the query row Item 93, the UI half. Unread, Inbox, Flagged and Sent are buttons the application ships, sitting first on the row, ahead of the user's pinned saved queries. runFilter() is runSavedQuery()'s opposite in the one way that matters: it READS the account box and never writes it. That is item 90's defect. A filter narrows what the user is already looking at, so the dropdown is its input rather than something it resets on the way past. A saved query keeps setting the account from what it stored, because it is a destination and states its own scope. runQuery() gains an AccountScope parameter. A filter's text arrives already resolved in the selected account's scope, and scoping it again would put path:"work/Sent/**" inside path:"work/**". Two migration changes, both of which unpin rather than delete: - Sent is no longer migrated from the INI into queries.json. The built-in filter covers it, and migrating one too would put two Sent buttons on the row, one editable and one not. - A stored entry naming a known generator is unpinned on load, which is what every install upgraded through 0.19.0 carries. It keeps its name and its generator and moves to the menu. Deleting it would be data loss on a file whose readers are supposed to preserve what they do not own. The test suite needed the same distinction the design makes. savedQueryButtonLabels() now skips the filters, and savedQueryButton(window, label) replaces five positional row->findChild() lookups that were silently returning Unread. One rendering probe had to be fixed rather than adapted. replyRowsKeepTheirTextUnderTheThreadLine resized the window to 300px, and four more buttons pushed the reply row below the viewport: the pixel loop then ran zero times and reported "0 pixels, the row was painted over", which is a different defect from the one it exists to catch. It gets 600px and a guard asserting the row is really inside the viewport, so the next person to shrink it gets told the truth. Verified by putting 300 back: the guard names the row at 83..165 in an 82px viewport. --- src/config.cpp | 36 +++-- src/mainwindow.cpp | 62 +++++++-- src/mainwindow.h | 26 +++- tests/test_config.cpp | 73 +++++++++- tests/test_mainwindow.cpp | 348 ++++++++++++++++++++++++++++++++++++++++++---- 5 files changed, 486 insertions(+), 59 deletions(-) (limited to 'src') diff --git a/src/config.cpp b/src/config.cpp index 0b65053..ece1fb9 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -482,22 +482,16 @@ 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. + // Sent is NOT migrated into queries.json any more. It used to become an + // ordinary saved query here, so the hardcoded button could be + // reordered, renamed or removed; item 93 makes it one of four built-in + // filters instead, which are shipped rather than stored. Migrating it + // as well would put two Sent buttons on the row, one of them the user's + // to edit and one not. // - // 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); - } + // Nothing is lost: the built-in Sent resolves through the same + // generator, so it still follows the accounts, and it now composes with + // the account dropdown rather than resetting it. // Order is alphabetical here because childKeys() is genuinely all the // INI knows. The user reorders once and it sticks from then on. @@ -585,6 +579,18 @@ void Config::loadSavedQueries(const QString &configPath, QSettings &settings) continue; } + // A stored entry naming a generator now duplicates a BUILT-IN filter of + // the same name, since item 93 ships all four rather than storing them. + // 0.19.0 migrated the hardcoded Sent button into exactly such an entry, + // so every existing install has one. + // + // Unpinned, never dropped: the row would otherwise carry two Sent + // buttons, one the user's to edit and one not. Deleting it would be + // data loss on a file whose readers are supposed to preserve what they + // do not own, and an unpin is reversible from the UI. + if (query.isGenerated() && isKnownGenerator(query.generated)) + query.pinned = false; + for (auto it = object.begin(); it != object.end(); ++it) { static const QStringList known = { QStringLiteral("name"), QStringLiteral("query"), diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index c6df95c..6bd91d9 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1717,12 +1717,31 @@ 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. + // The built-in filters come first, in their own fixed order, and they are + // not saved queries: they are shipped, they are not in queries.json, and + // the user cannot edit or delete them (item 93). They are what the row is + // FOR; the pinned saved queries below them are the transitional half that + // item 94 removes. + for (const SavedQuery &filter : Config::builtinFilters()) { + // Sent with no account configuring a sent folder finds nothing by + // construction. Hidden rather than present and empty, which is what the + // hardcoded Sent button did and is worth keeping: a control that always + // returns nothing reads as broken rather than as absent. + if (m_config.resolvedQuery(filter, QString()) + == Config::matchNothingQuery()) + continue; + + auto *button = new QPushButton(filter.name, row); + // A stable object name per filter, so a test finds the button without + // depending on the label, which is translated. + button->setObjectName(filter.generated + QStringLiteral("Button")); + connect(button, &QPushButton::clicked, this, + [this, filter]() { runFilter(filter); }); + box->addWidget(button); + } + + // The user's own saved queries. A pinned one is still a button, beside the + // filters, until item 94 makes the menu their only home. QList unpinned; for (const SavedQuery &saved : m_config.savedQueries()) { // A generator whose accounts configure nothing produces a button that @@ -1736,10 +1755,9 @@ void MainWindow::buildSavedQueryRow(QWidget *parent, QVBoxLayout *layout) 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")); + // No object name here any more. "sentButton" now belongs to the BUILT-IN + // Sent filter, and a migrated Sent entry claiming it too would give two + // buttons one name, so findChild() would return whichever came first. connect(button, &QPushButton::clicked, this, [this, saved]() { runSavedQuery(saved); }); addSavedQueryActions(button, saved); @@ -1928,6 +1946,22 @@ void MainWindow::runSavedQuery(const SavedQuery &saved) runQuery(saved.flat ? FlatResult::Yes : FlatResult::No); } +void MainWindow::runFilter(const SavedQuery &filter) +{ + // The account box is READ and never written. That is the whole difference + // from runSavedQuery(), and it is item 90's defect: a filter narrows what + // the user is already looking at, so the dropdown is its input rather than + // something it resets on the way past. + const QString accountKey = m_accountBox->currentData().toString(); + + // Resolved here, in the account's scope, and put in the bar so what ran is + // visible and editable. runQuery() is told not to scope it again. + m_queryEdit->setText(m_config.resolvedQuery(filter, accountKey)); + + runQuery(filter.flat ? FlatResult::Yes : FlatResult::No, + AccountScope::AlreadyScoped); +} + void MainWindow::saveCurrentQuery() { const QString query = m_queryEdit->text().trimmed(); @@ -2007,7 +2041,7 @@ void MainWindow::rebuildSavedQueryRow() } } -void MainWindow::runQuery(FlatResult flat) +void MainWindow::runQuery(FlatResult flat, AccountScope scope) { // Set on EVERY run, not only when Yes. This is the line that stops flat // mode leaking: any query that is not the Sent button restores the tree, @@ -2017,8 +2051,12 @@ void MainWindow::runQuery(FlatResult flat) QString query = m_queryEdit->text().trimmed(); + // A built-in filter arrives already resolved in the selected account's + // scope, because a generator has to be asked for the account's own query + // rather than have its all-accounts query wrapped. Scoping again here would + // put path:"work/Sent/**" inside path:"work/**". const QString accountKey = m_accountBox->currentData().toString(); - if (!accountKey.isEmpty()) + if (scope == AccountScope::Apply && !accountKey.isEmpty()) query = m_config.account(accountKey).scopedQuery(query); if (query.isEmpty()) diff --git a/src/mainwindow.h b/src/mainwindow.h index 80c7d2b..bb1ba75 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -274,12 +274,23 @@ public: enum class FlatResult { No, Yes }; Q_ENUM(FlatResult) + /// Whether runQuery() applies the selected account's scope to the bar text. + /// + /// Apply is right for anything the user typed or a saved query put there. + /// AlreadyScoped is for a built-in filter, whose text was resolved through + /// Config::resolvedQuery(query, accountKey) and already carries the scope: + /// scoping it a second time would wrap path:"work/Sent/**" in + /// path:"work/**", which is the double scope item 93 exists to avoid. + enum class AccountScope { Apply, AlreadyScoped }; + Q_ENUM(AccountScope) + private: /// The real query runner. Kept off the slot list deliberately: a slot with /// a defaulted argument does not satisfy QObject::connect, which matches /// signal and slot arity at compile time, so the zero-argument slot below /// is what widgets connect to. - void runQuery(FlatResult flat); + void runQuery(FlatResult flat, + AccountScope scope = AccountScope::Apply); /// Builds the row of saved-query buttons, the overflow menu and Sent. /// @@ -295,6 +306,19 @@ private: /// twice. Setting the dropdown also shows the user what scope they are in. void runSavedQuery(const SavedQuery &saved); + /// Runs a built-in filter in whatever account scope is currently selected. + /// + /// The opposite of runSavedQuery() in the one way that matters: it does NOT + /// touch the account box. A filter narrows what the user is already looking + /// at, so the dropdown is its input rather than something it overwrites, + /// which is item 90's defect and item 93's design. + /// + /// The query text is resolved here rather than left to runQuery()'s own + /// scoping, because a generator must be asked for the account's own query: + /// Sent wrapped in a scope double-scopes and works only by accident of + /// path: being hierarchical. See Config::resolvedQuery(query, accountKey). + void runFilter(const SavedQuery &filter); + /// Names the current query and stores it in queries.json. void saveCurrentQuery(); diff --git a/tests/test_config.cpp b/tests/test_config.cpp index 895495d..6c1815c 100644 --- a/tests/test_config.cpp +++ b/tests/test_config.cpp @@ -95,6 +95,7 @@ private slots: void allSentQueryIsEmptyWhenNoAccountHasOne(); void allSentQuerySkipsAccountsWithoutTheKey(); void allSentQueryJoinsEveryConfiguredAccount(); + void aStoredGeneratedQueryIsUnpinnedNotDropped(); void everyBuiltinFilterIsAKnownGenerator(); void aFilterAcrossAllAccountsIsTheUnscopedQuery(); void aTagFilterScopedToAnAccountCarriesThatAccountsPath(); @@ -1463,6 +1464,55 @@ void TestConfig::jsonWinsOnceItExists() QCOMPARE(queries.at(0).name, QStringLiteral("FromTheJson")); } +void TestConfig::aStoredGeneratedQueryIsUnpinnedNotDropped() +{ + // An existing install carries a Sent entry in queries.json: 0.19.0 migrated + // the hardcoded button into one. Item 93 ships Sent as a built-in filter, + // so that stored entry is now a DUPLICATE and would put two Sent buttons on + // the row, one editable and one not. + // + // Unpinned rather than deleted. This file's whole design is that a reader + // preserves what it does not own, and the user's instruction for their own + // redundant queries was the same: fold them into the menu, do not drop + // them. An unpin is reversible from the UI; a delete is not. + QTemporaryDir dir; + const QString path = writeIni(dir, QStringLiteral( + "[account.work]\n" + "maildir=work\n" + "sent=Sent\n")); + writeQueries(dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Sent", "generated": "sent", "pinned": true }, + { "name": "Mine", "query": "tag:todo", "pinned": true } + ] + })")); + + Config config; + config.load(path); + + const QList queries = config.savedQueries(); + QCOMPARE(queries.size(), 2); + + bool sawSent = false; + for (const SavedQuery &query : queries) { + if (query.generated != QStringLiteral("sent")) + continue; + sawSent = true; + QVERIFY2(!query.pinned, + "the stored Sent entry is still a button beside the built-in " + "filter of the same name"); + } + QVERIFY2(sawSent, "the stored Sent entry was DROPPED rather than unpinned"); + + // The user's own query is untouched: only the entry duplicating a built-in + // filter is unpinned. + for (const SavedQuery &query : queries) { + if (query.name == QStringLiteral("Mine")) + QVERIFY2(query.pinned, "an unrelated pinned query was unpinned"); + } +} + void TestConfig::malformedQueriesFileIsAProblemNotACrash() { QTemporaryDir dir; @@ -1676,13 +1726,24 @@ void TestConfig::migrationAddsSentWhenAnAccountHasOne() Config config; config.load(path); + // The migration used to invent a generated Sent entry here, so the + // hardcoded button could be reordered, renamed or removed like any other + // row. Item 93 ships Sent as one of four BUILT-IN filters instead, so + // migrating one as well would put two Sent buttons on the row: one the + // user's to edit and one not. + // + // Nothing is lost. The built-in resolves through the same generator, so it + // still follows the accounts, and it now composes with the account dropdown + // rather than resetting it. 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); + QCOMPARE(queries.size(), 1); + QCOMPARE(queries.at(0).name, QStringLiteral("Inbox")); + + for (const SavedQuery &query : queries) { + QVERIFY2(!query.isGenerated(), + "the migration invented a generated entry that now duplicates " + "a built-in filter"); + } } /// Today the button is hidden entirely when no account configures a sent diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 228a0bc..4cc9954 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -169,6 +169,13 @@ private slots: void narrowingAnEmptyQueryBarIsAPlainSearch(); void aMalformedAccountIsReportedWithoutBlockingTheConstructor(); void aWorkerBackedWindowReturnsRealThreads(); + void theFourBuiltinFiltersAreOnTheRowInOrder(); + void aFilterComposesWithTheSelectedAccount(); + void aFilterAcrossAllAccountsIsUnscoped(); + void aFilterDoesNotClearTheAccountSelection(); + void aSavedQueryStillClearsTheAccountSelection(); + void aFilterOffersNoEditOrDeleteActions(); + void changingTheAccountRunsNothing(); void arrivingBatchesUpdateTheStatusBarWithTheCountSoFar(); void aRefreshsBatchesLeaveTheStatusBarAlone(); void selectingAThreadRootShowsItInTheMessagePane(); @@ -1596,7 +1603,11 @@ void TestMainWindow::replyRowsKeepTheirTextUnderTheThreadLine() reply.depth = 1; model->setThreadMessages(QStringLiteral("t1"), { first, reply }); - window.resize(1400, 300); + // 600, not 300. The query row carries four built-in filter buttons since + // item 93, and at 300 the reply row was pushed below the viewport: the + // pixel loop then ran zero times and reported "0 pixels, the row was + // painted over", which is a different defect from the one that existed. + window.resize(1400, 600); window.show(); QVERIFY(QTest::qWaitForWindowExposed(&window)); @@ -1609,6 +1620,21 @@ void TestMainWindow::replyRowsKeepTheirTextUnderTheThreadLine() const QRect rect = view->visualRect(child); QVERIFY2(rect.height() > 0, "the reply row is not on screen"); + // visualRect reports a height for a row scrolled out of the viewport, so + // the check above passes while the loop below has nothing to walk. Assert + // the rect is really inside the image, or this measures nothing and says + // the row was painted over. + QVERIFY2(rect.top() >= 0 + && rect.bottom() < view->viewport()->height() + && rect.left() >= 0 + && rect.right() <= view->viewport()->width(), + qPrintable(QStringLiteral("the reply row at %1..%2 is outside the " + "%3px viewport, so the pixel count " + "below would measure nothing") + .arg(rect.top()) + .arg(rect.bottom()) + .arg(view->viewport()->height()))); + QImage shot(view->viewport()->size(), QImage::Format_ARGB32); shot.fill(Qt::transparent); view->viewport()->render(&shot); @@ -5464,6 +5490,19 @@ static void loadWithQueries(Config &config, QTemporaryDir &dir, } /// Buttons in the saved-query row, by label, in the order they are laid out. +/// Whether this is one of the four shipped filters rather than a saved query. +/// +/// By object name, not by label: the labels are translated, and a user may name +/// their own query "Unread" too. +static bool isBuiltinFilterButton(QPushButton *button) +{ + for (const SavedQuery &filter : Config::builtinFilters()) { + if (button->objectName() == filter.generated + QStringLiteral("Button")) + return true; + } + return false; +} + static QStringList savedQueryButtonLabels(MainWindow &window) { QStringList labels; @@ -5474,12 +5513,40 @@ static QStringList savedQueryButtonLabels(MainWindow &window) 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()); + if (button->objectName() == QStringLiteral("savedQueryMenuButton")) + continue; + // Neither are the four built-in filters (item 93), which sit first on + // the row and are not in queries.json at all. Every caller of this + // helper is asking about the USER's queries, so counting the filters + // would make each of them assert on a number it does not care about. + if (isBuiltinFilterButton(button)) + continue; + labels.append(button->text()); } return labels; } +/// The user's own pinned button carrying `label`, or null. +/// +/// Positional lookup does not work any more: the built-in filters occupy the +/// first four places on the row, so row->findChild() returns +/// Unread rather than the query a test means. +static QPushButton *savedQueryButton(MainWindow &window, const QString &label) +{ + auto *row = window.findChild(QStringLiteral("savedQueryRow")); + if (!row) + return nullptr; + const QList buttons = + row->findChildren(QString(), Qt::FindDirectChildrenOnly); + for (QPushButton *button : buttons) { + if (isBuiltinFilterButton(button)) + continue; + if (button->text() == label) + return button; + } + return nullptr; +} + void TestMainWindow::onlyPinnedQueriesBecomeButtons() { QTemporaryDir dir; @@ -5625,7 +5692,7 @@ void TestMainWindow::aScopedSavedQuerySelectsItsAccount() auto *row = window.findChild(QStringLiteral("savedQueryRow")); QVERIFY(row); - auto *button = row->findChild(); + auto *button = savedQueryButton(window, QStringLiteral("Billing")); QVERIFY(button); button->click(); @@ -5667,7 +5734,7 @@ void TestMainWindow::anUnscopedSavedQueryClearsTheAccount() auto *row = window.findChild(QStringLiteral("savedQueryRow")); QVERIFY(row); - auto *button = row->findChild(); + auto *button = savedQueryButton(window, QStringLiteral("Everywhere")); QVERIFY(button); button->click(); @@ -5888,13 +5955,31 @@ void TestMainWindow::aRenamedSentEntryKeepsWorking() })"), oneAccountWithSent()); MainWindow window(config); - const QStringList labels = savedQueryButtonLabels(window); - QCOMPARE(labels, QStringList{ QStringLiteral("Posta inviata") }); + // The renamed entry is UNPINNED on load now, because item 93 ships Sent as + // a built-in filter and two Sent buttons on the row, one editable and one + // not, is worse than one of each in its own place. It keeps its name, it + // keeps working, and it is in the menu rather than on the row. + QVERIFY2(savedQueryButtonLabels(window).isEmpty(), + "a stored generated entry is still a button beside the built-in " + "filter that duplicates it"); + + bool found = false; + for (const SavedQuery &saved : config.savedQueries()) { + if (saved.name != QStringLiteral("Posta inviata")) + continue; + found = true; + QVERIFY2(saved.isGenerated(), + "the entry lost its generator when renamed"); + QVERIFY2(!saved.pinned, "the entry was not unpinned"); + } + QVERIFY2(found, "the renamed entry was DROPPED rather than unpinned"); + + // The built-in Sent still resolves the same query, so nothing the user + // could reach before became unreachable. auto *button = window.findChild(QStringLiteral("sentButton")); - QVERIFY2(button, "the generated entry lost its identity when renamed"); - + QVERIFY(button); auto *queryEdit = window.findChild(QStringLiteral("queryEdit")); button->click(); @@ -5969,7 +6054,7 @@ void TestMainWindow::aSavedQueryButtonOffersEditUnpinAndDelete() MainWindow window(config); auto *row = window.findChild(QStringLiteral("savedQueryRow")); QVERIFY(row); - auto *button = row->findChild(); + auto *button = savedQueryButton(window, QStringLiteral("Inbox")); QVERIFY(button); // A context menu, so the actions live on the widget itself. @@ -6017,14 +6102,11 @@ void TestMainWindow::onlyAStoredQueryOffersToBecomeATaggingRule() auto *generated = row->findChild( QStringLiteral("sentButton")); - QPushButton *stored = nullptr; - const QList buttons = row->findChildren(); - for (QPushButton *button : buttons) { - if (button->text().contains(QStringLiteral("Inbox"))) { - stored = button; - break; - } - } + // savedQueryButton(), not a scan for the label: item 93 puts a BUILT-IN + // Inbox filter on the row too, and it carries no context actions by design, + // so a scan finds that one and the assertion below fails against correct + // code. + QPushButton *stored = savedQueryButton(window, QStringLiteral("Inbox")); QVERIFY2(stored, "no button was built for the stored query"); QVERIFY2(generated, "no button was built for the generated query"); @@ -6035,11 +6117,14 @@ void TestMainWindow::onlyAStoredQueryOffersToBecomeATaggingRule() QStringLiteral("queryToRule")), "a generated query must not: its query is a snapshot"); - // The guard proving the generated button HAS a menu, so the assertion - // above is about this one action and not about a button with no actions. - QVERIFY2(contextActionNamed(window, generated, - QStringLiteral("deleteQuery")), - "the generated button must still carry its other actions"); + // The guard, and it has moved since item 93. It used to prove the generated + // button HAS a menu, so the assertion above was about one action rather + // than about a button with none. `generated` is now the BUILT-IN Sent + // filter, which correctly carries no actions at all, so proving the + // machinery works has to happen on the button that does have them. + QVERIFY2(contextActionNamed(window, stored, QStringLiteral("deleteQuery")), + "the stored button lost its other actions, so the assertion above " + "is not about queryToRule in particular"); } void TestMainWindow::unpinningMovesAQueryToTheMenu() @@ -6062,7 +6147,7 @@ void TestMainWindow::unpinningMovesAQueryToTheMenu() QVERIFY(!window.findChild( QStringLiteral("savedQueryMenuButton"))); - auto *button = row->findChild(); + auto *button = savedQueryButton(window, QStringLiteral("Inbox")); QVERIFY(button); QAction *pin = contextActionNamed(window, button, QStringLiteral("pinQuery")); QVERIFY(pin); @@ -6100,7 +6185,7 @@ void TestMainWindow::deletingRemovesTheQueryFromTheFile() MainWindow window(config); auto *row = window.findChild(QStringLiteral("savedQueryRow")); QVERIFY(row); - auto *button = row->findChild(); + auto *button = savedQueryButton(window, QStringLiteral("Doomed")); QVERIFY(button); QCOMPARE(button->text(), QStringLiteral("Doomed")); @@ -6264,6 +6349,219 @@ void TestMainWindow::aWorkerBackedWindowReturnsRealThreads() QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); } +void TestMainWindow::theFourBuiltinFiltersAreOnTheRowInOrder() +{ + // Shipped, not pinned. Nothing in this config names a query, so a row with + // four buttons on it can only have got them from the built-in set: before + // item 93 a fresh install had an empty query row. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + config.load(writeSentConfig(dir, { + {QStringLiteral("work"), QStringLiteral("Sent")}, + })); + + MainWindow window(config); + auto *row = window.findChild(QStringLiteral("savedQueryRow")); + QVERIFY(row); + + QStringList labels; + for (QPushButton *button : row->findChildren()) { + // The overflow menu is a control over the set, not a member of it. + if (button->objectName() == QStringLiteral("savedQueryMenuButton")) + continue; + labels.append(button->text()); + } + + QCOMPARE(labels, (QStringList{ QStringLiteral("Unread"), + QStringLiteral("Inbox"), + QStringLiteral("Flagged"), + QStringLiteral("Sent") })); +} + +void TestMainWindow::aFilterComposesWithTheSelectedAccount() +{ + // Item 90, and the whole point of item 93. Select an account, hit Unread, + // and get that account's unread mail rather than everyone's. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + config.load(writeSentConfig(dir, { + {QStringLiteral("work"), QStringLiteral("Sent")}, + {QStringLiteral("personal"), QStringLiteral("Sent")}, + })); + + MainWindow window(config); + auto *queryEdit = window.findChild(QStringLiteral("queryEdit")); + QVERIFY(queryEdit); + + window.selectAccountForTesting(QStringLiteral("work")); + QCOMPARE(window.selectedAccountForTesting(), QStringLiteral("work")); + + auto *unread = + window.findChild(QStringLiteral("unreadButton")); + QVERIFY2(unread, "no built-in Unread button"); + unread->click(); + + QCOMPARE(queryEdit->text(), + QStringLiteral("path:\"work/**\" and (tag:unread)")); + + // Sent under the same account is the account's OWN folder, not the union + // wrapped in a scope. See the Config test of the same name for why a row + // count cannot tell the two apart. + auto *sent = window.findChild(QStringLiteral("sentButton")); + QVERIFY(sent); + sent->click(); + QCOMPARE(queryEdit->text(), QStringLiteral("path:\"work/Sent/**\"")); + QVERIFY2(!queryEdit->text().contains(QStringLiteral("personal")), + "another account's sent folder leaked into a scoped Sent filter"); +} + +void TestMainWindow::aFilterAcrossAllAccountsIsUnscoped() +{ + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + config.load(writeSentConfig(dir, { + {QStringLiteral("work"), QStringLiteral("Sent")}, + })); + + MainWindow window(config); + auto *queryEdit = window.findChild(QStringLiteral("queryEdit")); + QVERIFY(queryEdit); + + // "All accounts" is the default selection, so this is the fresh state. + QVERIFY(window.selectedAccountForTesting().isEmpty()); + + auto *unread = + window.findChild(QStringLiteral("unreadButton")); + QVERIFY(unread); + unread->click(); + + QCOMPARE(queryEdit->text(), QStringLiteral("tag:unread")); +} + +void TestMainWindow::aFilterDoesNotClearTheAccountSelection() +{ + // The defect item 90 filed: the button used to reset the dropdown to "All + // accounts" before running, so the selection was gone before the query ran. + // A filter leaves it exactly where the user put it. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + config.load(writeSentConfig(dir, { + {QStringLiteral("work"), QStringLiteral("Sent")}, + })); + + MainWindow window(config); + window.selectAccountForTesting(QStringLiteral("work")); + + auto *unread = + window.findChild(QStringLiteral("unreadButton")); + QVERIFY(unread); + unread->click(); + + QCOMPARE(window.selectedAccountForTesting(), QStringLiteral("work")); +} + +void TestMainWindow::aSavedQueryStillClearsTheAccountSelection() +{ + // The other half of the design, and the reason item 90 was not fixed in + // place. A saved query is a DESTINATION: it states its own scope, so an + // unscoped one clears the selection rather than inheriting it. That is the + // behaviour the rules preview depends on and it must survive item 93. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + const QString path = writeSentConfig(dir, { + {QStringLiteral("work"), QStringLiteral("Sent")}, + }); + { + QSettings s(path, QSettings::IniFormat); + s.beginGroup(QStringLiteral("queries")); + s.setValue(QStringLiteral("Mine"), QStringLiteral("tag:todo")); + s.endGroup(); + s.sync(); + } + config.load(path); + + MainWindow window(config); + window.selectAccountForTesting(QStringLiteral("work")); + + // The migrated [queries] entry is pinned, so it is a button beside the + // filters. Found by its label, since only the filters have stable object + // names. + QPushButton *mine = nullptr; + auto *row = window.findChild(QStringLiteral("savedQueryRow")); + QVERIFY(row); + for (QPushButton *button : row->findChildren()) { + if (button->text() == QStringLiteral("Mine")) + mine = button; + } + QVERIFY2(mine, "the user's own pinned query is not on the row"); + + mine->click(); + QVERIFY2(window.selectedAccountForTesting().isEmpty(), + "an unscoped saved query no longer clears the account selection"); +} + +void TestMainWindow::aFilterOffersNoEditOrDeleteActions() +{ + // A filter is not the user's to edit, rename or delete: it is shipped, and + // it is not in queries.json at all. Offering the actions would produce a + // dialog that writes an entry the row does not read. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + config.load(writeSentConfig(dir, { + {QStringLiteral("work"), QStringLiteral("Sent")}, + })); + + MainWindow window(config); + auto *unread = + window.findChild(QStringLiteral("unreadButton")); + QVERIFY(unread); + + for (QAction *action : unread->actions()) { + QVERIFY2(action->objectName() != QStringLiteral("editQuery"), + "a built-in filter offered Edit"); + QVERIFY2(action->objectName() != QStringLiteral("pinQuery"), + "a built-in filter offered a pin toggle"); + QVERIFY2(action->objectName() != QStringLiteral("deleteQuery"), + "a built-in filter offered Delete"); + } +} + +void TestMainWindow::changingTheAccountRunsNothing() +{ + // The user's decision, 2026-08-15: "changing the account should not run the + // query, hitting the button after changing the account is what queries." + // The dropdown selects scope; the button is the verb. + // + // Today m_accountBox has no signal connected at all, so this guards against + // wiring one up by reflex while making the filters compose. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + config.load(writeSentConfig(dir, { + {QStringLiteral("work"), QStringLiteral("Sent")}, + })); + + MainWindow window(config); + auto *queryEdit = window.findChild(QStringLiteral("queryEdit")); + QVERIFY(queryEdit); + + // A known starting point, so "nothing happened" is distinguishable from + // "it was empty all along", which would pass against a re-run that clears. + queryEdit->setText(QStringLiteral("tag:todo")); + const quint64 before = window.currentGenerationForTesting(); + + window.selectAccountForTesting(QStringLiteral("work")); + + QCOMPARE(queryEdit->text(), QStringLiteral("tag:todo")); + QCOMPARE(window.currentGenerationForTesting(), before); +} + void TestMainWindow::arrivingBatchesUpdateTheStatusBarWithTheCountSoFar() { // Item 74: "Searching..." was set once by runQuery and cleared only on -- cgit v1.2.3 From b1a7339120385f3fd8f1f5251ec20e3f0ff94b22 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 15 Aug 2026 11:09:29 +0200 Subject: fix(queries): make a query in the overflow menu runnable An unpinned query could not be run. Its menu action carried both a triggered connection and a submenu of edit actions, and Qt does not emit triggered for an action that owns a menu: clicking the entry only opened the submenu, so the connection had never fired. It shipped unnoticed because the menu was the rarely-used half while the user's queries were pinned buttons. Item 93 moved every query into the menu, which is how it surfaced, and item 94 makes the menu their only home, so this is now the path that has to work. Running is an item inside the submenu, first and above a separator, with the edit actions below it. The entry keeps its submenu because an unpinned query must still be editable and deletable. The test asserts the Run item exists and is first, then that triggering it reaches the query, then that Edit and Delete survived beside it. Restoring the old wiring fails it on the first of those, naming the Qt behaviour rather than just reporting a wrong query string. --- src/mainwindow.cpp | 25 ++++++++++++++--- tests/test_mainwindow.cpp | 69 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 6bd91d9..bf1a33b 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1779,12 +1779,29 @@ void MainWindow::buildSavedQueryRow(QWidget *parent, QVBoxLayout *layout) 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); }); + // 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. + // carries the same actions; an unpinned query would otherwise be + // the one thing that cannot be edited or deleted. auto *entryMenu = new QMenu(menu); + + // Running the query is an item INSIDE that submenu, and must be: + // Qt does not emit triggered for an action that owns a menu, so a + // connection on `action` itself never fires and clicking the entry + // only opens the submenu. That shipped, and went unnoticed while + // the menu was the rarely-used half and the user's queries were + // pinned buttons. Item 93 moved every query into the menu, and item + // 94 makes it their only home. + auto *run = new QAction(tr("Run"), entryMenu); + run->setObjectName(QStringLiteral("runQuery")); + connect(run, &QAction::triggered, this, + [this, saved]() { runSavedQuery(saved); }); + entryMenu->addAction(run); + + auto *runSeparator = new QAction(entryMenu); + runSeparator->setSeparator(true); + entryMenu->addAction(runSeparator); + addSavedQueryActions(entryMenu, saved); action->setMenu(entryMenu); } diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 4cc9954..6a049e3 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -169,6 +169,7 @@ private slots: void narrowingAnEmptyQueryBarIsAPlainSearch(); void aMalformedAccountIsReportedWithoutBlockingTheConstructor(); void aWorkerBackedWindowReturnsRealThreads(); + void aQueryInTheMenuCanActuallyBeRun(); void theFourBuiltinFiltersAreOnTheRowInOrder(); void aFilterComposesWithTheSelectedAccount(); void aFilterAcrossAllAccountsIsUnscoped(); @@ -6349,6 +6350,74 @@ void TestMainWindow::aWorkerBackedWindowReturnsRealThreads() QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); } +void TestMainWindow::aQueryInTheMenuCanActuallyBeRun() +{ + // An unpinned query was UNRUNNABLE. Its action carried both a triggered + // connection and a submenu of edit actions, and Qt does not emit triggered + // for an action that owns a menu: clicking it opens the submenu and nothing + // else. The connection had never fired. + // + // It shipped unnoticed because the menu was the rarely-used half while the + // user's queries were pinned buttons. Item 93 moved every one of them into + // the menu, which is how it surfaced, and item 94 makes the menu their only + // home, so this is the path that has to work. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + loadWithQueries(config, dir, QStringLiteral(R"({ + "version": 1, + "queries": [ + { "name": "Menued", "query": "tag:menued", "pinned": false } + ] + })")); + + MainWindow window(config); + auto *queryEdit = window.findChild(QStringLiteral("queryEdit")); + QVERIFY(queryEdit); + + auto *menuButton = + window.findChild(QStringLiteral("savedQueryMenuButton")); + QVERIFY2(menuButton, "no overflow menu for an unpinned query"); + QVERIFY(menuButton->menu()); + + QAction *entry = nullptr; + for (QAction *action : menuButton->menu()->actions()) { + if (action->text() == QStringLiteral("Menued")) + entry = action; + } + QVERIFY2(entry, "the unpinned query is not in the menu"); + + // The entry keeps its submenu, because an unpinned query must still be + // editable and deletable. What it cannot be is the ONLY thing there: Qt + // does not emit triggered for an action that owns a menu, so running the + // query needs an item of its own. + QVERIFY2(entry->menu(), "the per-query actions are gone"); + + QAction *run = nullptr; + for (QAction *action : entry->menu()->actions()) { + if (action->objectName() == QStringLiteral("runQuery")) + run = action; + } + QVERIFY2(run, "no way to run the query: its submenu offers only edit " + "actions, and Qt never emits triggered for the parent"); + + // First, before the edit actions. Running is what the entry is for; editing + // is what one does to it occasionally. + QCOMPARE(entry->menu()->actions().constFirst(), run); + + run->trigger(); + QCOMPARE(queryEdit->text(), QStringLiteral("tag:menued")); + + // The edit actions survived beside it. + QStringList names; + for (QAction *action : entry->menu()->actions()) + names.append(action->objectName()); + QVERIFY2(names.contains(QStringLiteral("editQuery")), + "the entry lost Edit"); + QVERIFY2(names.contains(QStringLiteral("deleteQuery")), + "the entry lost Delete"); +} + void TestMainWindow::theFourBuiltinFiltersAreOnTheRowInOrder() { // Shipped, not pinned. Nothing in this config names a query, so a row with -- cgit v1.2.3