diff options
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 36 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 93 | ||||
| -rw-r--r-- | src/mainwindow.h | 33 | ||||
| -rw-r--r-- | src/notmuchworker.cpp | 42 | ||||
| -rw-r--r-- | src/notmuchworker.h | 17 | ||||
| -rw-r--r-- | src/types.h | 14 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 97 | ||||
| -rw-r--r-- | tests/test_notmuchworker.cpp | 44 |
8 files changed, 375 insertions, 1 deletions
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 2e0930c..1cc75a7 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 @@ -82,7 +82,7 @@ taking that too literally. | 31 | The quit prompt has no highlighted default button | discoverability | XS | **done** | | 32 | Esc does not blank the right pane | workflow | XS | **done** | | 33 | Status bar messages never expire | feedback | S | **done** | -| 34 | No overview of the Maildir itself | information | M | open | +| 34 | No overview of the Maildir itself | information | M | **done** | | 35 | No refresh of the thread list after a sync | workflow | M | open | | 36 | `test_mainwindow` cannot reach the worker | testing | S | open, on demand | | 37 | The worker stalls on a tag edit made during a background sync | correctness | S | **done** | @@ -2001,6 +2001,40 @@ quietly reinterpret what it returns. **Still true from the constraints above:** the account count comes from config, never from notmuch, and the tag list is already fetched for the completer. +### Outcome (done 2026-08-07) + +A dialog under Help, as decided. `requestDatabaseStats` answers messages, +threads and tags in one round trip; the account list comes from `Config`. + +**A separate worker call, not a reuse of `requestCounts`**, exactly as the +revision above warned. That one counts THREADS to match the row count of a +query; this one counts MESSAGES, which is what a user means by "how much mail +is in here". The test asserts 4 messages in 3 threads against the fixture and +fails if they are ever made equal, so a later "simplification" that routes both +through one count cannot pass. + +**Unknown is not zero.** Every field starts at -1, and a field notmuch could not +answer renders as "unknown". Printing 0 would say the Maildir is empty, which is +a claim, and telling someone their mail is gone is the worst available way to +report an index that failed to open. Verified by mutation: removing the guard +puts a literal `-1` on screen. + +**The dialog opens before the answer arrives**, showing "Counting...". Counting +every message is not free on a large database, and a dialog that blocks first is +worse than one that fills in. + +Two lifetime problems follow from that, both handled and both tested: + +- The reply can arrive after the dialog is closed. The label is held in a + `QPointer`, since `WA_DeleteOnClose` means a raw pointer dangles for exactly + as long as the count takes, which is when the user is most likely to have + given up and closed it. The test drains `DeferredDelete` before firing the + late reply, because `close()` deletes through `deleteLater` and without the + drain the case being tested is not the one that occurs. +- The dialog can be closed and reopened while a count runs, so a generation + counter drops the older answer rather than filling in the newer dialog with + numbers that predate the reopen. + ## 35. No refresh of the thread list after a sync **Observed (user, 2026-08-04):** "auto refresh list after sync." diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 3f8a53d..0ce94a8 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -288,6 +288,7 @@ MainWindow::MainWindow(const Config &config, QWidget *parent) qRegisterMetaType<ThreadSummary>(); qRegisterMetaType<MessageRef>(); qRegisterMetaType<TagChange>(); + qRegisterMetaType<DatabaseStats>(); qRegisterMetaType<QVector<ThreadSummary>>(); qRegisterMetaType<QVector<MessageRef>>(); @@ -885,6 +886,13 @@ void MainWindow::buildMenus() auto *shortcuts = helpMenu->addAction(tr("&Keyboard shortcuts")); connect(shortcuts, &QAction::triggered, this, &MainWindow::showShortcutReference); + // A dialog the user asks for, per item 34: counting every message is not + // free on a large database, so this must not be anything that refreshes on + // its own. + auto *maildirInfo = helpMenu->addAction(tr("&Maildir overview")); + maildirInfo->setObjectName(QStringLiteral("maildirOverview")); + connect(maildirInfo, &QAction::triggered, + this, &MainWindow::showMaildirOverview); auto *about = helpMenu->addAction(tr("&About")); connect(about, &QAction::triggered, this, &MainWindow::showAbout); @@ -1031,6 +1039,89 @@ void MainWindow::showShortcutReference() dialog.exec(); } +void MainWindow::showMaildirOverview() +{ + auto *dialog = new QDialog(this); + dialog->setWindowTitle(tr("Maildir overview")); + dialog->setObjectName(QStringLiteral("maildirOverviewDialog")); + // Deleted on close, which is what makes m_overviewCounts a QPointer: the + // worker's reply can arrive after the user has dismissed it. + dialog->setAttribute(Qt::WA_DeleteOnClose); + + auto *counts = new QLabel(dialog); + counts->setObjectName(QStringLiteral("maildirCounts")); + counts->setTextFormat(Qt::RichText); + // Shown as pending rather than as zero. The dialog opens before the answer + // arrives, and a zero would read as "no mail", which is a claim rather than + // an absence of one. + counts->setText(tr("<b>Counting...</b>")); + m_overviewCounts = counts; + + // From config, never from notmuch, which does not model accounts at all. + // That is the whole reason per-account subdirectories are configured. + QString accountText; + const QList<Account> accounts = m_config.accounts(); + accountText += tr("<b>%n account(s)</b>", "", int(accounts.size())); + if (!accounts.isEmpty()) { + accountText += QStringLiteral("<ul>"); + for (const Account &account : accounts) { + // Account names are user-written config, and this label is rich + // text, so they are escaped like any other untrusted value. + const QString label = account.label.isEmpty() ? account.key + : account.label; + accountText += QStringLiteral("<li>%1</li>") + .arg(label.toHtmlEscaped()); + } + accountText += QStringLiteral("</ul>"); + } + + auto *accountLabel = new QLabel(accountText, dialog); + accountLabel->setObjectName(QStringLiteral("maildirAccounts")); + accountLabel->setTextFormat(Qt::RichText); + + auto *buttons = new QDialogButtonBox(QDialogButtonBox::Close, dialog); + connect(buttons, &QDialogButtonBox::rejected, dialog, &QDialog::reject); + + auto *layout = new QVBoxLayout(dialog); + layout->addWidget(counts); + layout->addWidget(accountLabel); + layout->addStretch(); + layout->addWidget(buttons); + + // Asked for when the dialog opens and never on a timer: counting every + // message in a large database is not free, which is the constraint that + // made this a dialog rather than a status-bar field. + QMetaObject::invokeMethod(m_worker, "requestDatabaseStats", + Qt::QueuedConnection, + Q_ARG(quint64, ++m_statsGeneration)); + + dialog->show(); +} + +void MainWindow::onDatabaseStatsReady(const DatabaseStats &stats, + quint64 generation) +{ + // Closed and reopened while the count ran: this answer belongs to the old + // dialog. The QPointer covers "closed", this covers "closed and reopened". + if (generation != m_statsGeneration) + return; + + if (!m_overviewCounts) + return; + + // A field notmuch could not answer stays unknown. Printing 0 would say the + // database is empty, which is the opposite of "we could not tell". + const auto number = [](int value) { + return value < 0 ? tr("unknown") : QLocale().toString(value); + }; + + m_overviewCounts->setText( + tr("<b>%1</b> messages in <b>%2</b> threads<br>" + "<b>%3</b> tags") + .arg(number(stats.messages), number(stats.threads), + number(stats.tags))); +} + void MainWindow::showAbout() { QDialog dialog(this); @@ -1098,6 +1189,8 @@ void MainWindow::wireWorker() this, &MainWindow::onAllTagsReady); connect(m_worker, &NotmuchWorker::countsReady, this, &MainWindow::onCountsReady); + connect(m_worker, &NotmuchWorker::databaseStatsReady, + this, &MainWindow::onDatabaseStatsReady); // A confirmed write clears the pending revert: without this, a later // unrelated error would roll back a change that actually succeeded. diff --git a/src/mainwindow.h b/src/mainwindow.h index 8043727..579597c 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -20,6 +20,7 @@ #include <QHash> #include <QMainWindow> +#include <QPointer> #include <QThread> #include <QUndoCommand> #include <QUndoStack> @@ -133,6 +134,13 @@ public: /// stale, so a test standing in for the worker has to know the current one. quint64 currentGenerationForTesting() const { return m_generation; } + /// The generation a database-stats reply must carry to be accepted. + /// + /// A test seam, for the same reason as the one above: onDatabaseStatsReady + /// discards a reply belonging to a dialog that has since been closed and + /// reopened, so a test standing in for the worker needs the current value. + quint64 statsGenerationForTesting() const { return m_statsGeneration; } + protected: void closeEvent(QCloseEvent *event) override; @@ -189,6 +197,10 @@ private slots: /// requestPlaceholderCounts() asked for them. void onCountsReady(const QVector<int> &counts, quint64 generation); + /// Fills in the overview dialog's counts when the worker answers. Does + /// nothing if the dialog has since been closed. + void onDatabaseStatsReady(const DatabaseStats &stats, quint64 generation); + /// Runs a query the user clicked on the placeholder pane. void onPlaceholderQueryRequested(const QString &query); @@ -225,6 +237,16 @@ private: void showShortcutReference(); void showAbout(); + /// The Maildir overview (item 34): what notmuch knows about the database, + /// plus the account list, which comes from config since notmuch does not + /// model accounts at all. + /// + /// Opens immediately showing the counts as pending and fills them in when + /// the worker answers, rather than blocking: counting every message is not + /// free on a large database and a dialog that hangs first is worse than one + /// that populates. + void showMaildirOverview(); + /// Creates a QAction, binds it to the sequence KeyMap holds for `name`, /// and registers it. `name` is the action name used in [keys]. QAction *addAction(const QString &name, const QString &text, @@ -422,6 +444,17 @@ private: /// attention, so it must survive until the next successful run. bool m_lastSyncFailed = false; + /// The overview dialog's counts label while that dialog is open, null + /// otherwise. A QPointer because the dialog is deleted on close and the + /// worker's reply can arrive afterwards: a raw pointer would dangle for + /// exactly as long as the count takes on a large database, which is + /// precisely when the user is most likely to close it first. + QPointer<QLabel> m_overviewCounts; + + /// Discriminates a stats reply from a dialog that has since been closed + /// and reopened, so an old answer cannot fill in a newer dialog. + quint64 m_statsGeneration = 0; + /// Holds the sync log and its close button, so the pane can be dismissed. QWidget *m_syncLogPane = nullptr; QPlainTextEdit *m_syncLog = nullptr; diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index 973d110..752a52c 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -363,6 +363,48 @@ void NotmuchWorker::requestAllTags(quint64 generation) emit allTagsReady(result, generation); } +void NotmuchWorker::requestDatabaseStats(quint64 generation) +{ + if (!openReadOnly()) + return; + + DatabaseStats stats; + + // "*" is notmuch's match-everything query. Counting messages and threads + // needs two calls on it: the numbers differ by the reply depth of the + // database and there is no single call that yields both. + NmQuery all(notmuch_query_create(m_db, "*")); + if (all) { + unsigned int messages = 0; + if (notmuch_query_count_messages(all.get(), &messages) + == NOTMUCH_STATUS_SUCCESS) { + stats.messages = static_cast<int>(messages); + } + } + + // A second query object rather than reusing the one above: notmuch caches + // results on a query, and counting both ways from one has bitten people. + NmQuery allThreads(notmuch_query_create(m_db, "*")); + if (allThreads) { + unsigned int threads = 0; + if (notmuch_query_count_threads(allThreads.get(), &threads) + == NOTMUCH_STATUS_SUCCESS) { + stats.threads = static_cast<int>(threads); + } + } + + // Already enumerated for the completer, so this costs nothing extra. + NmTags tags(notmuch_database_get_all_tags(m_db)); + if (tags) { + int count = 0; + for (; notmuch_tags_valid(tags.get()); notmuch_tags_move_to_next(tags.get())) + ++count; + stats.tags = count; + } + + emit databaseStatsReady(stats, generation); +} + void NotmuchWorker::requestCounts(const QStringList &queries, quint64 generation) { if (!openReadOnly()) diff --git a/src/notmuchworker.h b/src/notmuchworker.h index 05c1d3a..7d6a587 100644 --- a/src/notmuchworker.h +++ b/src/notmuchworker.h @@ -83,6 +83,19 @@ public slots: /// refreshing one nobody is looking at is work for nothing. void requestCounts(const QStringList &queries, quint64 generation); + /// Database-level facts for the Maildir overview (item 34): total messages, + /// total threads, and the number of tags. + /// + /// **Messages, not threads**, which is what distinguishes this from + /// requestCounts above. That one answers "how many rows will this query + /// produce" and counts threads to match the list; this one describes the + /// database, where the message total is the number a user means by "how + /// much mail is in here". + /// + /// Counting every message is not free on a large database, so this is + /// called when the dialog is opened and never on a timer. + void requestDatabaseStats(quint64 generation); + signals: void threadsReady(const QVector<ThreadSummary> &threads, quint64 generation); void queryFinished(int totalThreads, quint64 generation); @@ -95,6 +108,10 @@ signals: /// positional correspondence the caller relies on always holds. void countsReady(const QVector<int> &counts, quint64 generation); + /// Fields left at -1 are ones notmuch could not answer, which the dialog + /// renders as unknown rather than as zero. + void databaseStatsReady(const DatabaseStats &stats, quint64 generation); + void errorOccurred(const QString &message); private: diff --git a/src/types.h b/src/types.h index 59d1d06..999d3f3 100644 --- a/src/types.h +++ b/src/types.h @@ -78,6 +78,20 @@ struct TagChange } }; +/// Database-level facts for the Maildir overview. +/// +/// Every field is -1 until answered, so a dialog opened against a database that +/// cannot be read shows "unknown" rather than a confident zero. A zero is a +/// claim, and "no mail at all" is exactly the wrong thing to tell someone whose +/// index failed to open. +struct DatabaseStats +{ + int messages = -1; ///< Every message notmuch has indexed. + int threads = -1; ///< Every thread. Differs from messages by reply depth. + int tags = -1; ///< Distinct tag names in the database. +}; + Q_DECLARE_METATYPE(ThreadSummary) Q_DECLARE_METATYPE(MessageRef) Q_DECLARE_METATYPE(TagChange) +Q_DECLARE_METATYPE(DatabaseStats) diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 2625bf5..ef6f99d 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -78,6 +78,8 @@ private slots: void theStatusBarReportsAMultiRowSelection(); void clearSelectionBlanksThePaneAndDeselects(); void clearPaneLeavesTheSelectionAlone(); + void maildirOverviewShowsUnknownRatherThanZero(); + void maildirOverviewIgnoresAStaleReply(); void theThreadListOffersAContextMenu(); void aSecondRowBlanksThePaneNotOnlyAThird(); void aLocalSyncIsNotReportedAsABackgroundOne(); @@ -1179,6 +1181,101 @@ void TestMainWindow::clearPaneLeavesTheSelectionAlone() QCOMPARE(view->selectionModel()->selectedRows().size(), 1); } +void TestMainWindow::maildirOverviewShowsUnknownRatherThanZero() +{ + // A field notmuch could not answer must not render as 0. "0 messages" says + // the Maildir is empty, which is a claim; the truth is that the count + // failed, and telling someone their mail is gone is the worst available + // way to report an unreadable index. + const Config config; + MainWindow window(config); + + auto *action = window.findChild<QAction *>(QStringLiteral("maildirOverview")); + QVERIFY2(action, "no maildirOverview action"); + action->trigger(); + + auto *counts = window.findChild<QLabel *>(QStringLiteral("maildirCounts")); + QVERIFY2(counts, "the overview dialog has no counts label"); + + // The worker never answers in this fixture, so drive the slot directly + // with the all-unknown stats a failed open produces. + QMetaObject::invokeMethod( + &window, "onDatabaseStatsReady", Qt::DirectConnection, + Q_ARG(DatabaseStats, DatabaseStats{}), + Q_ARG(quint64, window.statsGenerationForTesting())); + + QVERIFY2(counts->text().contains(QStringLiteral("unknown")), + qPrintable(QStringLiteral("counts label says '%1'") + .arg(counts->text()))); + QVERIFY2(!counts->text().contains(QStringLiteral(">0<")), + qPrintable(QStringLiteral("an unanswered count rendered as zero: " + "'%1'").arg(counts->text()))); + + // WA_DeleteOnClose, so closing is what frees it. Left open, each test + // leaks a window for the rest of the run. + counts->window()->close(); +} + +void TestMainWindow::maildirOverviewIgnoresAStaleReply() +{ + // Counting every message is slow enough that closing and reopening the + // dialog while one runs is realistic. The older answer must not fill in the + // newer dialog, or the numbers silently predate whatever prompted the + // reopen. + const Config config; + MainWindow window(config); + + auto *action = window.findChild<QAction *>(QStringLiteral("maildirOverview")); + QVERIFY(action); + action->trigger(); + + const quint64 stale = window.statsGenerationForTesting(); + + // Reopening bumps the generation, which is what makes the first reply old. + action->trigger(); + QVERIFY2(window.statsGenerationForTesting() != stale, + "reopening the dialog did not bump the generation, so a reply for " + "the previous one cannot be told apart"); + + auto *counts = window.findChild<QLabel *>(QStringLiteral("maildirCounts")); + QVERIFY(counts); + const QString before = counts->text(); + + DatabaseStats old; + old.messages = 4321; + old.threads = 999; + old.tags = 42; + QMetaObject::invokeMethod(&window, "onDatabaseStatsReady", + Qt::DirectConnection, + Q_ARG(DatabaseStats, old), + Q_ARG(quint64, stale)); + + QCOMPARE(counts->text(), before); + QVERIFY2(!counts->text().contains(QStringLiteral("4321")), + "a reply for the previous dialog filled in the current one"); + + QPointer<QLabel> watch(counts); + counts->window()->close(); + + // WA_DeleteOnClose deletes through deleteLater, so the label outlives + // close() until the event loop runs. Drain it, or the "reply after the + // dialog is gone" case below is not actually being tested. + QCoreApplication::sendPostedEvents(nullptr, QEvent::DeferredDelete); + QVERIFY2(watch.isNull(), + "the dialog was not destroyed, so the case below is not the one " + "this test means to exercise"); + + // The QPointer's reason for being: counting a large database takes long + // enough that closing the dialog first is ordinary, and the reply then + // arrives for a label that has been deleted. A raw pointer would dangle + // here, so this must not crash. + QMetaObject::invokeMethod(&window, "onDatabaseStatsReady", + Qt::DirectConnection, + Q_ARG(DatabaseStats, old), + Q_ARG(quint64, + window.statsGenerationForTesting())); +} + void TestMainWindow::theThreadListOffersAContextMenu() { // Right-click is the other half of discoverability: until now every tag diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index be62ad3..b419915 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -60,6 +60,8 @@ private slots: void requestCountsAnswersOneCountPerQuery(); void requestCountsKeepsPositionOnAnInvalidQuery(); + void requestDatabaseStatsCountsMessagesNotThreads(); + void requestDatabaseStatsOnUnreadableConfigEmitsError(); private: /// Tags of one message, read back through a fresh worker query. @@ -523,5 +525,47 @@ void TestNotmuchWorker::requestCountsKeepsPositionOnAnInvalidQuery() QCOMPARE(counts.at(2), 3); } +void TestNotmuchWorker::requestDatabaseStatsCountsMessagesNotThreads() +{ + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy spy(&worker, &NotmuchWorker::databaseStatsReady); + + worker.requestDatabaseStats(11); + + QCOMPARE(spy.count(), 1); + QCOMPARE(spy.at(0).at(1).value<quint64>(), quint64(11)); + const auto stats = spy.at(0).at(0).value<DatabaseStats>(); + + // The fixture holds four messages in three threads: thread A is a message + // and its reply. **That difference is the whole point of this call.** + // requestCounts() counts threads, to match the row count of a query; this + // one counts messages, which is what a user means by "how much mail". A + // reimplementation that reused the thread count would report 3 here and be + // confidently wrong under the label "messages". + QCOMPARE(stats.messages, 4); + QCOMPARE(stats.threads, 3); + QVERIFY2(stats.messages != stats.threads, + "messages and threads are equal, so this fixture cannot prove the " + "two counts are distinct: add a reply to it"); + + // Every tag the fixture creates, plus notmuch's own. + QVERIFY(stats.tags > 0); +} + +void TestNotmuchWorker::requestDatabaseStatsOnUnreadableConfigEmitsError() +{ + // Fails closed like every other entry point. The dialog then shows its + // fields as unknown rather than as zero, since "no mail at all" is the + // wrong thing to tell someone whose index failed to open. + NotmuchWorker worker(QStringLiteral("/nonexistent/qtmaildir-test/config")); + QSignalSpy ready(&worker, &NotmuchWorker::databaseStatsReady); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + + worker.requestDatabaseStats(1); + + QCOMPARE(errors.size(), 1); + QVERIFY(ready.isEmpty()); +} + QTEST_MAIN(TestNotmuchWorker) #include "test_notmuchworker.moc" |