diff options
| -rw-r--r-- | CHANGELOG.md | 8 | ||||
| -rw-r--r-- | README.md | 39 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 2 | ||||
| -rw-r--r-- | docs/superpowers/specs/2026-08-11-sent-mail-design.md | 62 | ||||
| -rw-r--r-- | src/config.cpp | 37 | ||||
| -rw-r--r-- | src/config.h | 31 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 50 | ||||
| -rw-r--r-- | src/mainwindow.h | 28 | ||||
| -rw-r--r-- | src/mimeparser.cpp | 59 | ||||
| -rw-r--r-- | src/mimeparser.h | 17 | ||||
| -rw-r--r-- | src/notmuchworker.cpp | 57 | ||||
| -rw-r--r-- | src/notmuchworker.h | 15 | ||||
| -rw-r--r-- | src/threadlistmodel.cpp | 39 | ||||
| -rw-r--r-- | src/threadlistmodel.h | 19 | ||||
| -rw-r--r-- | src/types.h | 12 | ||||
| -rw-r--r-- | tests/notmuchfixture.h | 7 | ||||
| -rw-r--r-- | tests/test_config.cpp | 161 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 147 | ||||
| -rw-r--r-- | tests/test_mimeparser.cpp | 76 | ||||
| -rw-r--r-- | tests/test_notmuchworker.cpp | 164 | ||||
| -rw-r--r-- | tests/test_threadlistmodel.cpp | 104 |
21 files changed, 1108 insertions, 26 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 55aa11f..22a6159 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,14 @@ point at which they are stable. ### Added +- **A Sent view.** A new `sent` key on each `[account.*]` names that account's + sent folder, and a Sent button beside Inbox, Unread and Important shows what + you sent across every account that configures one. Selecting an account + narrows it to that account. The button is absent entirely when no account has + the key. +- Sent mail is shown as a flat list rather than as threads, and the cards name + the **recipients** instead of the sender, which is you on every row. + Selecting one opens what you sent, not the conversation your message started. - `[general] date_format`, an optional pattern for the date on a thread card. Absent or empty keeps the system locale's short format, which is unchanged and remains the default. A pattern containing no date or time field is @@ -165,6 +165,7 @@ name = Your Name address = you@example.org maildir = work-mail ; relative to notmuch's database.path drafts = Drafts ; recorded for v2; unused today +sent = Sent ; optional; enables the Sent button for this account label = W ; optional chip text; defaults to the key color = #2f6fa8 ; optional chip colour; generated when unset channel = work ; optional mbsync channel; defaults to the key @@ -203,6 +204,44 @@ 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. +### Sent mail + +A **Sent** button appears beside the saved queries once at least one account +carries a `sent` key naming its sent folder, relative to that account's +`maildir`: + +```ini +[account.work] +maildir = work-mail +sent = Sent + +[account.webmail] +maildir = webmail +sent = [Provider]/Posta inviata ; nested and localised folders are fine + +[account.list-only] +maildir = list-only +; no sent key: this account is simply left out of the Sent view +``` + +The button composes its query from those keys every time you press it, rather +than storing one, so adding an account or correcting a folder name is a config +edit and nothing else. With no account selected it shows every configured +account's sent mail; selecting one narrows it to that account. An account +without the key is omitted silently, since keeping no sent mail locally is a +legitimate setup rather than a mistake. + +Sent mail is presented differently from the rest, because it reads differently: + +- **A flat list, not threads.** A message you sent otherwise drags in the + replies you received, and a view labelled Sent then shows conversations. +- **Cards name the recipients**, not the sender, which is you on every row. +- **Selecting one opens what you sent**, rather than the whole conversation it + started. + +This applies only to the Sent button. The same query typed into the bar by hand +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. 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 534dc6e..a187874 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 @@ -119,7 +119,7 @@ taking that too literally. | 60 | Next thread dead-ends on the last reply of an expanded thread | defect | XS | **done**; already fixed by 5487d58, see below | | 61 | `test_mainwindow` fails intermittently, about 1 run in 20 | testing | S | open; predates the card list, reproduced on f72dba9 | | 62 | No config option for the date format on a card | presentation | XS | **done** 2026-08-11 | -| 63 | No way to see sent mail, and no filter for it | workflow | M | open; specified 2026-08-11 in `specs/2026-08-11-sent-mail-design.md` | +| 63 | No way to see sent mail, and no filter for it | workflow | M | **done** 2026-08-11; see `specs/2026-08-11-sent-mail-design.md` | | 64 | The Sync button carries a mailbox icon, not a refresh one | presentation | XS | **done** 2026-08-11 | | 65 | No full code review and optimization pass | correctness | ? | open, unspecified | diff --git a/docs/superpowers/specs/2026-08-11-sent-mail-design.md b/docs/superpowers/specs/2026-08-11-sent-mail-design.md index 53fbb6c..4934241 100644 --- a/docs/superpowers/specs/2026-08-11-sent-mail-design.md +++ b/docs/superpowers/specs/2026-08-11-sent-mail-design.md @@ -1,6 +1,7 @@ # Sent mail: a per-account folder, a composed button, and recipients on the card -**Status:** specified 2026-08-11, not implemented. +**Status:** specified and built 2026-08-11, hand-verified by the user. See +"Outcome" at the end for what the spec did not anticipate. **Resolves:** backlog item 63. **Size:** M, revised up from the backlog's S. The query half is the S that was scoped correctly; the recipients half is its own piece of work. @@ -114,6 +115,18 @@ Built beside the saved-query buttons, running the OR of every non-empty `ThreadSummary` gains a recipients summary, filled in the worker, shown by `CardDelegate` in the sender's place when the row belongs to a Sent view. +**The fold must be OPT-IN per query, and this is not a preference.** Measured +2026-08-11 against the real database: `notmuch_message_get_header(m, "To")` is +NOT served from the index, it reads the message file. Folding it for every +thread of a 4411-thread inbox took **38.2 seconds**, 8.7 ms per thread. The +same fold over the 601-thread Sent view took **663 ms**, 1.1 ms per thread, +which the existing 200-thread batching hides. + +So the worker takes a flag on the query, set only when the query is a Sent one, +and skips the walk entirely otherwise. A version that always folds turns an +instant inbox into a 38-second one, and it would look correct in every test: +the data is right, only the cost is wrong. + ## Constraints **Do not invent a tag qtmaildir applies itself.** v1 is read-and-organize; @@ -178,3 +191,50 @@ named `signals`, which Qt defines as a macro. The first and last are the ones that fail loudest if the quoting is wrong, and they are the reason to write them before the UI work rather than after. + +## Outcome (done 2026-08-11) + +Built in the three pieces above and hand-verified by the user. Four things the +spec did not anticipate, each found by using it rather than by reading code. + +**A flat list, which the spec never mentioned.** The user's first report was +that the Sent view showed the replies they had RECEIVED. That is correct +behaviour, since the query matches messages and the list groups them into +threads, but it is not what a Sent view is for: their stated mental model is +that sent mail "lives on its own". `ThreadListModel::setFlatMode()` makes +`hasChildren()` and `ReplyCountRole` answer differently and nothing else +changes. It is one flag on the existing model rather than a second model or a +filtered query, which was the user's condition for building it at all. + +**Flat mode cannot leak, and that is structural rather than careful.** +`runQuery()` sets the mode on EVERY run, so any query that is not the Sent +button restores the tree on its way through. The window test mutates this +directly: making the flag one-way leaves it passing every model test and +flattens the whole application from the first Sent click. + +**The pane needed its own fix, and the flat list is why.** With the list flat +and correct, selecting a Sent row still opened the whole conversation. The +single-message path needs `ThreadNode::first`, which is only filled when a +thread is EXPANDED, so in a flat list it is always empty and every selection +falls through to `loadThread`. That now takes `matchedOnly`, dropping the +messages that did not match rather than rendering them as stubs. The per-message +`matched` flag it needs was already computed. + +**The recipient fold is cheaper than the spec's measurement.** 251 ms for the +601-thread Sent view against the 663 ms measured while specifying, because the +worker stops at the first usable `To` per thread rather than reading every +message. The inbox, with the flag off, is unchanged at 148 ms for 4411 threads. +The opt-in is mutation-tested: always folding fails with "the To header was read +for a query that never asked for it". + +**One mutation survived, and the comment was corrected rather than the code.** +Removing the `haveMatchSet` guard beside `matchedOnly` changes nothing, because +`ref.matched` is already true for every message when no query was given. The +guard is redundant today and kept as a stated invariant at the point that +depends on it; the test that appeared to cover it now says plainly that it does +not. + +**Known limit, accepted.** A flat row is still one row per THREAD, not per sent +message: 795 sent messages live in 601 threads here, so a thread written to +twice appears once, dated by its newest match. The model is thread-keyed +throughout, so per-message rows would be a different piece of work. diff --git a/src/config.cpp b/src/config.cpp index 9e223f8..47fadec 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -47,6 +47,37 @@ QString Account::scopedQuery(const QString &query) const return QStringLiteral("%1 and (%2)").arg(prefix, query); } +QString Account::sentQuery() const +{ + if (sent.isEmpty()) + return QString(); + + // The QUOTES are load-bearing, not decoration. A real provider nests its + // sent folder under a bracketed parent, "[Provider]/Posta inviata", and + // "[" and "]" are Xapian syntax: unquoted, the term is parsed rather than + // matched and the query silently returns nothing while looking correct. + // + // The path is user config and is interpolated into a query, so this is the + // only place that composition happens; a caller building it by hand would + // be a second chance to forget the quotes. + return QStringLiteral("path:\"%1/%2/**\"").arg(maildir, sent); +} + +QString Config::allSentQuery() const +{ + // Collect first, join after. Appending "or" per account and trimming the + // result is the version that produced the defect this guards: an account + // with no sent key contributes an empty term, notmuch accepts the bare + // "or" without complaint, and the query quietly means something else. + QStringList parts; + for (const Account &account : m_accounts) { + const QString query = account.sentQuery(); + if (!query.isEmpty()) + parts.append(query); + } + return parts.join(QStringLiteral(" or ")); +} + QString Config::defaultPath() { const QString base = @@ -281,6 +312,12 @@ void Config::load(const QString &path) account.maildir = settings.value(QStringLiteral("maildir")).toString(); account.drafts = settings.value(QStringLiteral("drafts")).toString(); + // Optional, and absent for an account that keeps no sent mail locally. + // Trimmed because a trailing space would land inside the quoted path + // and match nothing, which is invisible in a config file. + account.sent = + settings.value(QStringLiteral("sent")).toString().trimmed(); + // Both optional, and both describe this account's chip in the thread // list. An account tag is a different taxonomy from a functional one, // saying which mailbox a thread arrived in rather than what state it diff --git a/src/config.h b/src/config.h index bcbfb34..e3c5b6e 100644 --- a/src/config.h +++ b/src/config.h @@ -36,6 +36,18 @@ struct Account QString maildir; ///< Relative to notmuch's database.path. QString drafts; ///< Unused in v1; send is v2. + /// The account's sent folder, relative to maildir. Optional and empty for + /// an account that has none, which is a real case rather than a + /// misconfiguration: an account may keep no sent mail locally at all. + /// + /// A key rather than a <maildir>/Sent convention because the folder is not + /// uniform across providers. Measured across one real setup: two accounts + /// use `Sent`, two nest a localised name under a bracketed parent + /// (`[Provider]/Posta inviata`), and one has no sent folder whatsoever. A + /// convention would produce an empty view for the nested ones and a wrong + /// one for the account that has none. + QString sent; + /// Chip colour in the thread list. Invalid when unset, in which case one /// is generated from the account tag's name. QColor color; @@ -62,6 +74,13 @@ struct Account /// Restricts a notmuch query to this account's subtree. QString scopedQuery(const QString &query) const; + + /// Matches this account's sent mail, or empty when `sent` is unset. + /// + /// Composes with scopedQuery() rather than replacing it: the account + /// selector wraps whatever query runs, so a Sent view under one account + /// intersects to that account's sent mail and cannot leak another's. + QString sentQuery() const; }; struct SavedQuery @@ -114,6 +133,18 @@ public: /// Optional alternate notmuch config file. Empty means "let notmuch decide". QString notmuchConfig() const { return m_notmuchConfig; } + /// Matches every configured account's sent mail, or empty when no account + /// configures one. + /// + /// Joins only the NON-EMPTY sentQuery() results. An account without a + /// `sent` key contributes nothing, and joining it anyway would leave a bare + /// `or` in the query. notmuch does not reject that: `A or or B` returns + /// 190 messages where the correct pair returns 211, measured directly. A + /// malformed query that still returns plausible mail is the failure that + /// ships, which is why the join lives here and is tested rather than being + /// open-coded at the call site. + QString allSentQuery() const; + /// A QDateTime::toString() pattern for the date on a card, or empty for the /// system locale's short format. /// diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 374f228..be6cfdb 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -457,6 +457,7 @@ void MainWindow::buildUi() this, &MainWindow::runCurrentQuery); m_queryEdit = new QLineEdit(central); + m_queryEdit->setObjectName(QStringLiteral("queryEdit")); m_queryEdit->setPlaceholderText(tr("notmuch query, e.g. tag:inbox")); // Qt draws the clear button inside the field and shows it only when there // is text, themed by the desktop. A hand-rolled button beside the bar would @@ -553,6 +554,28 @@ void MainWindow::buildUi() }); 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); // Thread list and message pane. @@ -1471,8 +1494,14 @@ void MainWindow::showWarnings() problems.join(QLatin1Char('\n'))); } -void MainWindow::runCurrentQuery() +void MainWindow::runQuery(FlatResult flat) { + // 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, + // so the flag cannot survive into the next view. + m_sentView = flat == FlatResult::Yes; + m_model->setFlatMode(m_sentView); + QString query = m_queryEdit->text().trimmed(); const QString accountKey = m_accountBox->currentData().toString(); @@ -1516,10 +1545,13 @@ void MainWindow::runCurrentQuery() const auto sort = m_sortOrder->currentIndex() == 1 ? NotmuchWorker::OldestFirst : NotmuchWorker::NewestFirst; + // Recipients only for the Sent view: the fold reads message FILES, which + // is tens of seconds over an inbox. See ThreadSummary::recipients. QMetaObject::invokeMethod(m_worker, "runQuery", Qt::QueuedConnection, Q_ARG(QString, query), Q_ARG(quint64, m_generation), - Q_ARG(NotmuchWorker::SortOrder, sort)); + Q_ARG(NotmuchWorker::SortOrder, sort), + Q_ARG(bool, m_sentView)); } void MainWindow::onThreadsReady(const QVector<ThreadSummary> &threads, @@ -1863,10 +1895,16 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, m_currentMessageId.clear(); m_currentMessageThreadId.clear(); + // In the Sent view the pane shows only what matched, which is what the + // user sent. Without this the flat list is right and the pane still opens + // the whole conversation, replies included, under a heading that says + // Sent: the row was never expanded, so the model never learned the + // thread's first message and this is the only path a Sent row can take. QMetaObject::invokeMethod(m_worker, "loadThread", Qt::QueuedConnection, Q_ARG(QString, m_currentThreadId), Q_ARG(QString, m_lastQuery), - Q_ARG(quint64, m_generation)); + Q_ARG(quint64, m_generation), + Q_ARG(bool, m_sentView)); } void MainWindow::onMessageLoaded(const QVector<MessageRef> &messages, @@ -2250,10 +2288,14 @@ void MainWindow::refreshCurrentQuery() const auto sort = m_sortOrder->currentIndex() == 1 ? NotmuchWorker::OldestFirst : NotmuchWorker::NewestFirst; + // The SAME recipients flag the visible view was built with. A refresh that + // dropped it would quietly replace a Sent view's recipients with empty + // strings on the first background sync, while the user was reading it. QMetaObject::invokeMethod(m_worker, "runQuery", Qt::QueuedConnection, Q_ARG(QString, m_lastQuery), Q_ARG(quint64, m_refreshGeneration), - Q_ARG(NotmuchWorker::SortOrder, sort)); + Q_ARG(NotmuchWorker::SortOrder, sort), + Q_ARG(bool, m_sentView)); } void MainWindow::updateStaleThreadNotice() diff --git a/src/mainwindow.h b/src/mainwindow.h index a7ea5c3..6e90ba1 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -173,8 +173,25 @@ protected: /// this the action fires from inside the bar and the query never runs. bool eventFilter(QObject *watched, QEvent *event) override; +public: + /// Whether the result of a query is shown as a flat list of threads rather + /// than as an expandable tree. + /// + /// Only the Sent button asks for Yes. Every other route runs the no-arg + /// slot, which passes No, so flat mode cannot outlive the view that asked + /// for it: the same query typed by hand comes back as a tree. + enum class FlatResult { No, Yes }; + Q_ENUM(FlatResult) + +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); + private slots: - void runCurrentQuery(); + void runCurrentQuery() { runQuery(FlatResult::No); } /// Brings back a thread that stopped matching, and restores the reader's /// place inside it. @@ -642,6 +659,15 @@ private: /// the whole result set rather than the batches that have arrived so far. /// Gates mark_all_read, which cannot honestly say "all" before then. bool m_queryComplete = false; + + /// Whether the CURRENT view is the Sent one, and therefore whether it is + /// flat and carries recipients. + /// + /// Held rather than recomputed because a background refresh re-runs the + /// same query without going through the Sent button: without this, the + /// first cron sync would silently turn a Sent view back into a tree of + /// senders while the user was reading it. + bool m_sentView = false; QString m_lastQuery; QString m_currentThreadId; diff --git a/src/mimeparser.cpp b/src/mimeparser.cpp index 4ad617e..ecf56f1 100644 --- a/src/mimeparser.cpp +++ b/src/mimeparser.cpp @@ -213,6 +213,65 @@ QString Attachment::saveWithoutOverwriting(const QString &directory, return target; } +QString recipientSummary(const QString &rawTo, int maxNames) +{ + const QByteArray utf8 = rawTo.trimmed().toUtf8(); + if (utf8.isEmpty()) + return {}; + + // Returns NULL rather than an empty list for input it cannot make anything + // of, including the empty string. Guarded above and again here: the header + // is untrusted and this is the crash if it is not. + InternetAddressList *list = internet_address_list_parse(nullptr, + utf8.constData()); + if (!list) + return {}; + + QStringList names; + int total = 0; + const int count = internet_address_list_length(list); + for (int i = 0; i < count; ++i) { + InternetAddress *address = internet_address_list_get_address(list, i); + if (!address) + continue; + + // A group (`undisclosed-recipients:;`) has a name but no address, and + // its members, if any, are a list of their own. Only mailboxes are + // counted: naming the group would print "undisclosed-recipients" as + // though it were a person. + if (!INTERNET_ADDRESS_IS_MAILBOX(address)) + continue; + + ++total; + if (names.size() >= maxNames) + continue; + + const char *name = internet_address_get_name(address); + const QString display = name ? QString::fromUtf8(name).trimmed() + : QString(); + if (!display.isEmpty()) { + names.append(display); + continue; + } + const char *addr = internet_address_mailbox_get_addr( + INTERNET_ADDRESS_MAILBOX(address)); + if (addr) + names.append(QString::fromUtf8(addr)); + else + --total; // Neither a name nor an address: nothing to show. + } + g_object_unref(list); + + if (names.isEmpty()) + return {}; + + QString summary = names.join(QStringLiteral(", ")); + const int hidden = total - names.size(); + if (hidden > 0) + summary += QStringLiteral(" +%1").arg(hidden); + return summary; +} + QString attachmentFolderName(const QString &rfc822Date, const QString &subject) { // The date prefix sorts chronologically in a file manager. A Date: header diff --git a/src/mimeparser.h b/src/mimeparser.h index af7619f..9fceb2f 100644 --- a/src/mimeparser.h +++ b/src/mimeparser.h @@ -94,6 +94,23 @@ struct Attachment /// subject can be far longer than that. QString attachmentFolderName(const QString &rfc822Date, const QString &subject); +/// A one-line summary of a raw To: header, for the sender's place on a card in +/// a Sent view. +/// +/// Parsed with GMime rather than split on commas: a display name may CONTAIN a +/// comma, so `"Rossi, Mario" <m@example.org>, info@example.net` is two +/// addresses and naive splitting reports three. +/// +/// Each address renders as its display name, falling back to the address when +/// it has none, so a list does not mix "Mario Rossi" with a bare address. With +/// more than `maxNames` recipients the rest collapse into "+N", mirroring the +/// tag strip's overflow chip rather than being elided mid-name. +/// +/// The header is untrusted and may be empty, malformed, or a group such as +/// `undisclosed-recipients:;`. Returns an empty string when nothing usable can +/// be read, never a partial parse. +QString recipientSummary(const QString &rawTo, int maxNames = 2); + struct ParsedMessage { bool ok = false; diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index d34c032..24f3fd9 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -24,6 +24,7 @@ #include <cstdlib> +#include "mimeparser.h" #include "nmraii.h" namespace { @@ -46,6 +47,45 @@ QStringList tagsOf(notmuch_thread_t *thread) return result; } +/// Who a thread's messages were addressed to, summarised for one line. +/// +/// EXPENSIVE, and only called when a query asks. "To" is not served from +/// notmuch's index, so every call here reads message FILES: 8.7 ms per thread +/// measured against a real database, which is 38 seconds over a 4411-thread +/// inbox. See ThreadSummary::recipients. +/// +/// The messages come from the THREAD and are owned by it, freed when it is +/// freed (notmuch.h:1637). They are therefore held raw and never wrapped in +/// NmMessage, which would call notmuch_message_destroy on memory the thread +/// frees again, and the walk finishes before the caller drops the thread. This +/// is the same rule walkReplies follows, and getting it wrong is a double-free +/// rather than a leak. +QString recipientsOf(notmuch_thread_t *thread) +{ + // The first message with a usable To wins. A thread is one conversation, + // and the alternative, folding every message's recipients together, is the + // participants-list problem item 2 rejected: it produces a union that + // misdescribes itself the moment a thread has replies going both ways. + notmuch_messages_t *messages = notmuch_thread_get_messages(thread); + for (; notmuch_messages_valid(messages); + notmuch_messages_move_to_next(messages)) { + notmuch_message_t *message = notmuch_messages_get(messages); + if (!message) + continue; + + // Returns "" for a missing header and NULL on error, and the two mean + // different things only to notmuch: both are "nothing to show" here. + const char *to = notmuch_message_get_header(message, "To"); + if (!to || !*to) + continue; + + const QString summary = recipientSummary(QString::fromUtf8(to)); + if (!summary.isEmpty()) + return summary; + } + return QString(); +} + /// Collects the message ids a query matches. Returns false if the query could /// not be run at all, which is different from a query that matched nothing. bool collectMessageIds(notmuch_database_t *db, const QString &query, @@ -187,7 +227,7 @@ void NotmuchWorker::close() } void NotmuchWorker::runQuery(const QString &query, quint64 generation, - SortOrder sort) + SortOrder sort, bool withRecipients) { if (!openReadOnly()) return; @@ -231,6 +271,8 @@ void NotmuchWorker::runQuery(const QString &query, quint64 generation, summary.totalCount = notmuch_thread_get_total_messages(thread.get()); summary.matchedCount = notmuch_thread_get_matched_messages(thread.get()); summary.tags = tagsOf(thread.get()); + if (withRecipients) + summary.recipients = recipientsOf(thread.get()); batch.append(summary); ++total; @@ -250,7 +292,7 @@ void NotmuchWorker::runQuery(const QString &query, quint64 generation, void NotmuchWorker::loadThread(const QString &threadId, const QString &matchQuery, - quint64 generation) + quint64 generation, bool matchedOnly) { if (!openReadOnly()) return; @@ -303,6 +345,17 @@ void NotmuchWorker::loadThread(const QString &threadId, ref.filePath = QString::fromUtf8(notmuch_message_get_filename(message.get())); ref.tags = tagsOf(message.get()); ref.matched = !haveMatchSet || matchedIds.contains(ref.messageId); + + // Dropped rather than rendered as a stub. + // + // haveMatchSet is redundant here and kept deliberately: ref.matched is + // already true for every message when no query was given, so the two + // conditions cannot disagree today. It states the invariant this + // depends on at the point that depends on it, so a later change to how + // ref.matched is computed cannot silently empty the pane. + if (matchedOnly && haveMatchSet && !ref.matched) + continue; + result.append(ref); } diff --git a/src/notmuchworker.h b/src/notmuchworker.h index 1d8c8c0..2dd766f 100644 --- a/src/notmuchworker.h +++ b/src/notmuchworker.h @@ -60,14 +60,25 @@ public: public slots: /// Runs a query. generation lets the UI discard results from a superseded /// query without the worker needing to know about cancellation. + /// `withRecipients` fills ThreadSummary::recipients by reading each + /// thread's To headers. OFF by default and deliberately opt-in: To is not + /// in notmuch's index, so this reads message FILES, at roughly 8.7 ms per + /// thread. Only a Sent query asks for it; turning it on for an inbox query + /// costs tens of seconds and changes nothing a user can see. void runQuery(const QString &query, quint64 generation, - SortOrder sort = NewestFirst); + SortOrder sort = NewestFirst, bool withRecipients = false); /// Loads the messages of one thread, oldest first. matchQuery is the /// user's current query; messages matching it render expanded, the rest /// as stubs. + /// `matchedOnly` drops the messages that did not match `matchQuery` rather + /// than rendering them as stubs. For the Sent view, where the thread is not + /// the unit the user is reading: a sent message pulls in the replies it + /// received, and a pane claiming to show what they sent then shows a + /// conversation. Ignored when `matchQuery` is empty, since nothing was + /// filtered and every message counts as matched. void loadThread(const QString &threadId, const QString &matchQuery, - quint64 generation); + quint64 generation, bool matchedOnly = false); /// Loads a thread as a reply TREE, for the message rows in the list. /// diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index 33356f0..5874648 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -176,6 +176,21 @@ QModelIndex ThreadListModel::parent(const QModelIndex &child) const return createIndex(static_cast<int>(id), 0, static_cast<quintptr>(-1)); } +void ThreadListModel::setFlatMode(bool flat) +{ + if (m_flatMode == flat) + return; + + // A full reset, not dataChanged. Flat mode changes what hasChildren() and + // rowCount() answer for every thread row, and a view that has already + // expanded one is holding indexes below it: dataChanged says "these rows + // are different", not "the shape under them is gone", and leaves the view + // drawing children the model no longer offers. + beginResetModel(); + m_flatMode = flat; + endResetModel(); +} + int ThreadListModel::rowCount(const QModelIndex &parent) const { if (!parent.isValid()) @@ -190,6 +205,11 @@ int ThreadListModel::rowCount(const QModelIndex &parent) const if (parent.row() < 0 || parent.row() >= m_threads.size()) return 0; + // Hidden, not discarded: leaving flat mode restores the tree with no + // reload, and an expansion loaded before the switch is still there. + if (m_flatMode) + return 0; + return m_threads.at(parent.row()).children.size(); } @@ -209,6 +229,10 @@ bool ThreadListModel::hasChildren(const QModelIndex &parent) const if (parent.row() < 0 || parent.row() >= m_threads.size()) return false; + // No expander in a flat list, whatever the thread turns out to contain. + if (m_flatMode) + return false; + const ThreadNode &node = m_threads.at(parent.row()); // Once loaded the children are the truth, including "there are none", which @@ -469,6 +493,15 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const // twice on the same card. return thread.subject; case SendersRole: + // Recipients in their place when the query supplied them, which only a + // Sent query does. The sender there is the user on every row and says + // nothing; who it went TO is the question the view exists to answer. + // + // Falls back to authors when the fold found no usable To, so a message + // with a malformed or absent recipient header shows the sender rather + // than a blank line where a name belongs. + if (!thread.recipients.isEmpty()) + return thread.recipients; return thread.authors; case DateRole: // The QDateTime itself. Formatting belongs to the delegate now: the @@ -480,6 +513,12 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const case IsFlaggedRole: return thread.isFlagged(); case ReplyCountRole: + // Zero in a flat list, so the card draws no expander pill. The count + // and hasChildren() must agree: a card advertising "3 replies" that + // cannot be opened is the inert-glyph defect this project has already + // shipped once. + if (m_flatMode) + return 0; // totalCount includes the root message, which is the card itself. return qMax(0, thread.totalCount - 1); case DateFormatRole: diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index 777841e..8b8f414 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -170,6 +170,24 @@ public: /// The pattern DateFormatRole answers with. Empty means the system format. void setDateFormat(const QString &format) { m_dateFormat = format; } + /// One row per thread, with no expander and no reply count. + /// + /// For the Sent view, where a thread is the wrong unit: the user's model of + /// "what I sent" is a list, and a matching sent message otherwise drags in + /// the replies they RECEIVED, under a view that claims to be their outbox. + /// + /// A flag on this model rather than a second model or a filtered query, and + /// that is what keeps it from leaking: it is off by default, only the Sent + /// button turns it on, and every other query turns it off again. The + /// expander already comes from hasChildren() and the card's count from + /// ReplyCountRole, so flat mode is those two answering differently and + /// nothing else changes. + /// + /// The children are not discarded, only hidden. Leaving flat mode restores + /// the tree without reloading anything. + void setFlatMode(bool flat); + bool flatMode() const { return m_flatMode; } + QModelIndex index(int row, int column, const QModelIndex &parent = {}) const override; QModelIndex parent(const QModelIndex &child) const override; @@ -284,4 +302,5 @@ private: QVector<ThreadNode> m_threads; const TagColors *m_tagColors = nullptr; QString m_dateFormat; + bool m_flatMode = false; }; diff --git a/src/types.h b/src/types.h index d04670e..2211619 100644 --- a/src/types.h +++ b/src/types.h @@ -33,6 +33,18 @@ struct ThreadSummary int matchedCount = 0; QStringList tags; + /// Who the thread's messages were sent TO, summarised for one line. + /// + /// Empty unless the query asked for it, and that is a performance + /// contract rather than a default: To is NOT served from notmuch's index, + /// so filling this reads every message file. Measured at 8.7 ms per thread, + /// which is 38 seconds over a 4411-thread inbox and 663 ms over a + /// 601-thread sent view. Only a Sent query asks. + /// + /// Shown in the sender's place there, where `authors` is the user on every + /// row and carries nothing. + QString recipients; + bool isUnread() const { return tags.contains(QStringLiteral("unread")); } bool isFlagged() const { return tags.contains(QStringLiteral("flagged")); } bool isDeleted() const { return tags.contains(QStringLiteral("deleted")); } diff --git a/tests/notmuchfixture.h b/tests/notmuchfixture.h index 4d2e59b..bb25526 100644 --- a/tests/notmuchfixture.h +++ b/tests/notmuchfixture.h @@ -47,10 +47,13 @@ public: /// Writes one message into <folder>/cur (or new/ when unread). /// /// Returns false if the file could not be written. Call index() afterwards. + /// `to` defaults to a single generic recipient. Pass one explicitly to + /// exercise the recipient summary, which is the only thing that reads it. bool addMessage(const QString &folder, const QString &messageId, const QString &subject, const QString &from, const QString &date, const QString &body, - bool unread = true, const QString &inReplyTo = QString()) + bool unread = true, const QString &inReplyTo = QString(), + const QString &to = QStringLiteral("you@example.org")) { // Unread messages must not carry the maildir "S" flag, so they go to // new/ where no flags exist at all. @@ -77,7 +80,7 @@ public: QTextStream out(&file); out << "From: " << from << "\n" - << "To: you@example.org\n" + << "To: " << to << "\n" << "Subject: " << subject << "\n" << "Message-ID: <" << messageId << ">\n" << "Date: " << date << "\n"; diff --git a/tests/test_config.cpp b/tests/test_config.cpp index b9321e0..60ed071 100644 --- a/tests/test_config.cpp +++ b/tests/test_config.cpp @@ -64,6 +64,13 @@ private slots: void malformedExtraMimetypeIsSkipped(); void syncChannelDefaultsToTheAccountKey(); void syncChannelIsActuallyRead(); + void sentQueryIsEmptyWithoutTheKey(); + void sentQueryComposesThePath(); + void sentQuerySurvivesABracketedPath(); + void sentQueryComposesWithScopedQuery(); + void allSentQueryIsEmptyWhenNoAccountHasOne(); + void allSentQuerySkipsAccountsWithoutTheKey(); + void allSentQueryJoinsEveryConfiguredAccount(); }; static QString writeIni(const QTemporaryDir &dir, const QString &body) @@ -755,5 +762,159 @@ void TestConfig::syncChannelIsActuallyRead() QStringLiteral("mail-firstlast")); } +void TestConfig::sentQueryIsEmptyWithoutTheKey() +{ + // Optional exactly as drafts is. A real account can legitimately have no + // sent folder at all, and the Sent view omits it silently rather than + // reporting a config problem on every launch about nothing. + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral( + "[account.provider-c]\n" + "maildir = provider-c\n"))); + + QCOMPARE(config.accounts().size(), 1); + QVERIFY(config.accounts().at(0).sentQuery().isEmpty()); + QVERIFY(config.problems().isEmpty()); +} + +void TestConfig::sentQueryComposesThePath() +{ + // Relative to maildir, the same way the account's own scope is, so the two + // cannot disagree about where the account lives. + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral( + "[account.webmail-primary]\n" + "maildir = webmail-primary\n" + "sent = Sent\n"))); + + QCOMPARE(config.accounts().at(0).sentQuery(), + QStringLiteral("path:\"webmail-primary/Sent/**\"")); +} + +void TestConfig::sentQuerySurvivesABracketedPath() +{ + // The load-bearing case, and the reason this is a config key rather than a + // <maildir>/Sent convention. A real provider nests its sent folder under a + // BRACKETED parent and localises the name: "[Provider]/Posta inviata". + // + // "[" and "]" are Xapian syntax. The quotes around the whole path are what + // make the query work at all, and an implementation that built this without + // them returns nothing while looking entirely plausible. + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral( + "[account.provider-a]\n" + "maildir = provider-a\n" + "sent = [Provider]/Posta inviata\n"))); + + const QString query = config.accounts().at(0).sentQuery(); + QCOMPARE(query, + QStringLiteral("path:\"provider-a/[Provider]/Posta inviata/**\"")); + + // Stated separately from the QCOMPARE above: the quoting is the property + // that matters, and a later change to the surrounding syntax must not be + // able to drop it while still matching a rewritten expected string. + QVERIFY2(query.contains(QStringLiteral("\"provider-a/[Provider]")), + "the composed path is not quoted, so Xapian will read the " + "brackets as syntax and the query will match nothing"); +} + +void TestConfig::sentQueryComposesWithScopedQuery() +{ + // A Sent view under one account must not show another account's sent mail. + // The account selector wraps whatever query runs, so the composed sent + // query has to survive being scoped rather than bypassing it. + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral( + "[account.webmail-primary]\n" + "maildir = webmail-primary\n" + "sent = Sent\n"))); + + const Account account = config.accounts().at(0); + const QString scoped = account.scopedQuery(account.sentQuery()); + + QCOMPARE(scoped, + QStringLiteral("path:\"webmail-primary/**\" and " + "(path:\"webmail-primary/Sent/**\")")); +} + +void TestConfig::allSentQueryIsEmptyWhenNoAccountHasOne() +{ + // Empty rather than a query matching nothing, so the caller can hide the + // Sent button entirely instead of offering one that finds no mail. + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral( + "[account.provider-c]\n" + "maildir = provider-c\n"))); + + QVERIFY(config.allSentQuery().isEmpty()); +} + +void TestConfig::allSentQuerySkipsAccountsWithoutTheKey() +{ + // Joining an account with no `sent` key would leave a bare "or" in the + // query, and notmuch does not reject that: it silently returns a DIFFERENT + // result. Measured directly against a real database, `A or or B` returns + // 190 where the correct pair returns 211. + // + // A malformed query that still returns plausible mail is the failure that + // ships, so this asserts the shape of the string rather than a count. + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral( + "[account.webmail-primary]\n" + "maildir = webmail-primary\n" + "sent = Sent\n" + "\n" + "[account.provider-c]\n" + "maildir = provider-c\n" + "\n" + "[account.webmail-secondary]\n" + "maildir = webmail-secondary\n" + "sent = Sent\n"))); + + const QString all = config.allSentQuery(); + + QVERIFY2(!all.contains(QStringLiteral("or or")), + "an account without a sent key left a bare 'or' in the query"); + QVERIFY2(!all.trimmed().endsWith(QStringLiteral("or")), + "the query ends in a dangling 'or'"); + QVERIFY2(!all.trimmed().startsWith(QStringLiteral("or")), + "the query starts with a dangling 'or'"); + QVERIFY(!all.contains(QStringLiteral("provider-c"))); + + // Exactly two terms joined, one per account that configures the key. + QCOMPARE(all.count(QStringLiteral("path:")), 2); + QCOMPARE(all.count(QStringLiteral(" or ")), 1); +} + +void TestConfig::allSentQueryJoinsEveryConfiguredAccount() +{ + // Including a bracketed provider path, which is the case the quoting + // exists for and the one most likely to be broken by a later rewrite of + // this composition. + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral( + "[account.webmail-primary]\n" + "maildir = webmail-primary\n" + "sent = Sent\n" + "\n" + "[account.provider-a]\n" + "maildir = provider-a\n" + "sent = [Provider]/Posta inviata\n"))); + + const QString all = config.allSentQuery(); + + QVERIFY(all.contains(QStringLiteral("path:\"webmail-primary/Sent/**\""))); + QVERIFY(all.contains( + QStringLiteral("path:\"provider-a/[Provider]/Posta inviata/**\""))); + QCOMPARE(all.count(QStringLiteral(" or ")), 1); +} + QTEST_MAIN(TestConfig) #include "test_config.moc" diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 504c10c..faa8481 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -170,6 +170,10 @@ private slots: void theImportantActionIsLabelledImportant(); void theImportantActionStillWritesTheFlaggedTag(); void theToolbarUsesTheConfiguredIconSize(); + void thereIsNoSentButtonWithoutASentKey(); + void theSentButtonRunsEveryConfiguredAccount(); + void theSentButtonSurvivesABracketedPath(); + void flatModeDoesNotSurviveTheNextQuery(); void noTwoActionsShareAnIcon(); }; @@ -4540,6 +4544,149 @@ void TestMainWindow::theToolbarUsesTheConfiguredIconSize() QCOMPARE(toolBar->iconSize(), QSize(40, 40)); } +namespace { + +/// A config whose accounts carry the given maildir/sent pairs. An empty `sent` +/// writes no key at all, which is the account-without-a-sent-folder case. +QString writeSentConfig(const QTemporaryDir &dir, + const QList<QPair<QString, QString>> &accounts) +{ + const QString path = dir.filePath(QStringLiteral("qtmaildir.conf")); + QSettings s(path, QSettings::IniFormat); + for (const auto &account : accounts) { + s.beginGroup(QStringLiteral("account.") + account.first); + s.setValue(QStringLiteral("maildir"), account.first); + if (!account.second.isEmpty()) + s.setValue(QStringLiteral("sent"), account.second); + s.endGroup(); + } + s.sync(); + return path; +} + +} // namespace + +void TestMainWindow::thereIsNoSentButtonWithoutASentKey() +{ + // Hidden entirely rather than present and finding nothing. An account may + // legitimately keep no sent mail locally, and a button that always returns + // an empty list reads as a broken feature rather than an absent one. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + config.load(writeSentConfig(dir, {{QStringLiteral("provider-c"), {}}})); + QVERIFY(config.allSentQuery().isEmpty()); + + MainWindow window(config); + QVERIFY(!window.findChild<QPushButton *>(QStringLiteral("sentButton"))); +} + +void TestMainWindow::theSentButtonRunsEveryConfiguredAccount() +{ + // The button composes its query rather than storing one, which is the whole + // reason it is not a [queries] entry: a saved query is a fixed string and + // would not gain the third account here without the user editing it. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + config.load(writeSentConfig(dir, { + {QStringLiteral("webmail-primary"), QStringLiteral("Sent")}, + {QStringLiteral("provider-c"), {}}, + {QStringLiteral("webmail-secondary"), QStringLiteral("Sent")}, + })); + + MainWindow window(config); + auto *button = window.findChild<QPushButton *>(QStringLiteral("sentButton")); + QVERIFY(button); + + auto *queryEdit = window.findChild<QLineEdit *>(QStringLiteral("queryEdit")); + QVERIFY(queryEdit); + + button->click(); + const QString query = queryEdit->text(); + + QVERIFY(query.contains(QStringLiteral("webmail-primary/Sent"))); + QVERIFY(query.contains(QStringLiteral("webmail-secondary/Sent"))); + + // The account with no key contributes nothing, and leaves no bare "or" + // behind: notmuch accepts that and silently returns a different result. + QVERIFY(!query.contains(QStringLiteral("provider-c"))); + QVERIFY(!query.contains(QStringLiteral("or or"))); + QCOMPARE(query.count(QStringLiteral(" or ")), 1); +} + +void TestMainWindow::theSentButtonSurvivesABracketedPath() +{ + // A real provider nests its sent folder under a bracketed parent, and "[" + // and "]" are Xapian syntax. The quoting has to survive the trip from the + // config through Account::sentQuery() into the query bar; unquoted, the + // query looks plausible and matches nothing. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + config.load(writeSentConfig(dir, { + {QStringLiteral("provider-a"), QStringLiteral("[Provider]/Posta inviata")}, + })); + + MainWindow window(config); + auto *button = window.findChild<QPushButton *>(QStringLiteral("sentButton")); + QVERIFY(button); + auto *queryEdit = window.findChild<QLineEdit *>(QStringLiteral("queryEdit")); + QVERIFY(queryEdit); + + button->click(); + QCOMPARE(queryEdit->text(), + QStringLiteral("path:\"provider-a/[Provider]/Posta inviata/**\"")); +} + +void TestMainWindow::flatModeDoesNotSurviveTheNextQuery() +{ + // The condition the user set for this feature: a flat Sent list is fine, a + // flat anything-else is not. Asserted at the window rather than the model, + // because the leak this guards against is in the WIRING, not in the model: + // setFlatMode(true) from the button with no matching false anywhere else + // passes every model test and flattens the app from the first Sent click + // until it restarts. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + Config config; + config.load(writeSentConfig(dir, { + {QStringLiteral("webmail-primary"), QStringLiteral("Sent")}, + })); + + MainWindow window(config); + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *button = window.findChild<QPushButton *>(QStringLiteral("sentButton")); + QVERIFY(button); + auto *queryEdit = window.findChild<QLineEdit *>(QStringLiteral("queryEdit")); + QVERIFY(queryEdit); + + QVERIFY2(!model->flatMode(), "the model starts flat"); + + button->click(); + QVERIFY2(model->flatMode(), "the Sent button did not flatten the list"); + + // Any other query restores the tree. Typed by hand rather than through a + // saved-query button, since that is the route with no flag of its own and + // therefore the one most likely to be forgotten. + queryEdit->setText(QStringLiteral("tag:inbox")); + QMetaObject::invokeMethod(&window, "runCurrentQuery"); + QVERIFY2(!model->flatMode(), + "flat mode survived into an ordinary query, so every view after " + "one Sent click lost its replies"); + + // And back, so the button still works after the round trip. + button->click(); + QVERIFY(model->flatMode()); + + // Even the SAME query typed by hand comes back as a tree: the flag follows + // the button, not the text, which is the rule the user chose. + queryEdit->setText(config.allSentQuery()); + QMetaObject::invokeMethod(&window, "runCurrentQuery"); + QVERIFY(!model->flatMode()); +} + void TestMainWindow::noTwoActionsShareAnIcon() { // Reported by the user against the icons shipped in 0.12.0: Archive and diff --git a/tests/test_mimeparser.cpp b/tests/test_mimeparser.cpp index d16735f..bac71f6 100644 --- a/tests/test_mimeparser.cpp +++ b/tests/test_mimeparser.cpp @@ -41,6 +41,10 @@ private slots: void safeFilenameStripsPathComponents(); void pathInsideDirectoryRejectsSiblingPrefix(); void attachmentFolderNameIsASinglePlainComponent(); + void recipientSummaryPrefersDisplayNames(); + void recipientSummaryKeepsACommaInsideADisplayName(); + void recipientSummaryCollapsesTheOverflow(); + void recipientSummarySurvivesUnusableInput(); void folderNameSurvivesATimezoneComment(); void savingABatchNeverOverwrites(); @@ -409,5 +413,77 @@ void TestMimeParser::savingABatchNeverOverwrites() qPrintable(second_tar)); } +void TestMimeParser::recipientSummaryPrefersDisplayNames() +{ + // A display name where there is one, the address where there is not, so a + // list does not mix "Mario Rossi" with a bare address for no reason the + // reader can see. + QCOMPARE(recipientSummary( + QStringLiteral("Mario Rossi <mario@example.org>")), + QStringLiteral("Mario Rossi")); + + QCOMPARE(recipientSummary(QStringLiteral("info@example.net")), + QStringLiteral("info@example.net")); + + QCOMPARE(recipientSummary(QStringLiteral( + "Mario Rossi <mario@example.org>, info@example.net")), + QStringLiteral("Mario Rossi, info@example.net")); +} + +void TestMimeParser::recipientSummaryKeepsACommaInsideADisplayName() +{ + // The reason this uses GMime rather than QString::split(','). A quoted + // display name may CONTAIN a comma, and splitting reports three recipients + // where there are two, with "Mario" alone as one of them. + const QString summary = recipientSummary(QStringLiteral( + "\"Rossi, Mario\" <mario@example.org>, info@example.net")); + + QCOMPARE(summary, QStringLiteral("Rossi, Mario, info@example.net")); + + // Stated separately, because the QCOMPARE above would also pass a naive + // implementation that happened to rejoin the pieces in the same order. + QVERIFY2(!summary.contains(QStringLiteral("+")), + "a comma inside a display name was counted as another recipient"); +} + +void TestMimeParser::recipientSummaryCollapsesTheOverflow() +{ + // "+N" rather than eliding mid-name, mirroring the tag strip's overflow + // chip: a card has one line for this and a truncated name is worse than an + // honest count. + QCOMPARE(recipientSummary(QStringLiteral( + "a@example.org, b@example.org, c@example.org, " + "d@example.org")), + QStringLiteral("a@example.org, b@example.org +2")); + + // Exactly at the limit does not collapse: a "+0" would be absurd. + QCOMPARE(recipientSummary( + QStringLiteral("a@example.org, b@example.org")), + QStringLiteral("a@example.org, b@example.org")); +} + +void TestMimeParser::recipientSummarySurvivesUnusableInput() +{ + // The header is untrusted and every one of these is real mail. + // + // The empty string is the one that crashes if unguarded: + // internet_address_list_parse returns NULL for it rather than an empty + // list, verified against GMime directly. + QVERIFY(recipientSummary(QString()).isEmpty()); + QVERIFY(recipientSummary(QStringLiteral("")).isEmpty()); + QVERIFY(recipientSummary(QStringLiteral(" ")).isEmpty()); + + // Not a crash and not a lie: a group with no members has no names to show. + const QString group = + recipientSummary(QStringLiteral("undisclosed-recipients:;")); + QVERIFY2(!group.contains(QStringLiteral("@")), + qPrintable(QStringLiteral("a memberless group produced an " + "address: %1").arg(group))); + + // Garbage parses to something or to nothing, but never to a crash. + recipientSummary(QStringLiteral("<<<>>>")); + recipientSummary(QStringLiteral("\"unterminated <a@example.org>")); +} + QTEST_MAIN(TestMimeParser) #include "test_mimeparser.moc" diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index 88dcf0b..fe0247c 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -65,6 +65,13 @@ private slots: void loadThreadTreeReportsReplyDepth(); void loadThreadTreeCarriesTheFactsARowNeeds(); + void loadThreadMatchedOnlyDropsTheRest(); + void loadThreadMatchedOnlyWithNoQueryKeepsEverything(); + + void recipientsAreAbsentUnlessAskedFor(); + void recipientsAreFoldedWhenAskedFor(); + void recipientsCrossAQueuedCall(); + void requestCountsAnswersOneCountPerQuery(); void requestCountsKeepsPositionOnAnInvalidQuery(); void requestDatabaseStatsCountsMessagesNotThreads(); @@ -74,10 +81,12 @@ private: /// Tags of one message, read back through a fresh worker query. QStringList tagsOf(const QString &messageId); QVector<MessageRef> messagesOfThread(const QString &threadId, - const QString &matchQuery = QString()); + const QString &matchQuery = QString(), + bool matchedOnly = false); QVector<ThreadSummary> runQuery( const QString &query, - NotmuchWorker::SortOrder sort = NotmuchWorker::NewestFirst); + NotmuchWorker::SortOrder sort = NotmuchWorker::NewestFirst, + bool withRecipients = false); QString threadIdOf(const QString &subject); NotmuchFixture m_fixture; @@ -114,17 +123,38 @@ void TestNotmuchWorker::initTestCase() QStringLiteral("Thu, 4 Jun 2026 10:00:00 +0000"), QStringLiteral("fourth message"), false)); + // Thread D: in a "sent" folder, with real recipients. The To header is the + // only thing that distinguishes these from the threads above, and it is + // what the recipient fold reads. + QVERIFY(m_fixture.addMessage(QStringLiteral("sent"), QStringLiteral("d1@example.org"), + QStringLiteral("Preventivo"), + QStringLiteral("You <you@example.org>"), + QStringLiteral("Fri, 5 Jun 2026 10:00:00 +0000"), + QStringLiteral("fifth message"), false, QString(), + QStringLiteral("Mario Rossi <mario@example.org>"))); + + // Thread E: several recipients, one of them with a comma inside a quoted + // display name, which is what defeats splitting on commas. + QVERIFY(m_fixture.addMessage(QStringLiteral("sent"), QStringLiteral("e1@example.org"), + QStringLiteral("Riunione"), + QStringLiteral("You <you@example.org>"), + QStringLiteral("Sat, 6 Jun 2026 10:00:00 +0000"), + QStringLiteral("sixth message"), false, QString(), + QStringLiteral("\"Rossi, Mario\" <mario@example.org>, " + "info@example.net, " + "third@example.org"))); + QVERIFY2(m_fixture.index(), qPrintable(m_fixture.error())); } QVector<ThreadSummary> TestNotmuchWorker::runQuery( - const QString &query, NotmuchWorker::SortOrder sort) + const QString &query, NotmuchWorker::SortOrder sort, bool withRecipients) { NotmuchWorker worker(m_fixture.configPath()); QSignalSpy ready(&worker, &NotmuchWorker::threadsReady); QSignalSpy finished(&worker, &NotmuchWorker::queryFinished); - worker.runQuery(query, 1, sort); + worker.runQuery(query, 1, sort, withRecipients); QVector<ThreadSummary> all; for (const QList<QVariant> &args : ready) @@ -143,11 +173,12 @@ QString TestNotmuchWorker::threadIdOf(const QString &subject) } QVector<MessageRef> TestNotmuchWorker::messagesOfThread(const QString &threadId, - const QString &matchQuery) + const QString &matchQuery, + bool matchedOnly) { NotmuchWorker worker(m_fixture.configPath()); QSignalSpy loaded(&worker, &NotmuchWorker::threadLoaded); - worker.loadThread(threadId, matchQuery, 1); + worker.loadThread(threadId, matchQuery, 1, matchedOnly); if (loaded.isEmpty()) return {}; return loaded.first().at(0).value<QVector<MessageRef>>(); @@ -256,7 +287,7 @@ void TestNotmuchWorker::loadThreadTreeCarriesTheFactsARowNeeds() void TestNotmuchWorker::queryReturnsAllThreads() { const QVector<ThreadSummary> threads = runQuery(QStringLiteral("*")); - QCOMPARE(threads.size(), 3); + QCOMPARE(threads.size(), 5); } void TestNotmuchWorker::queryFiltersByTag() @@ -325,7 +356,7 @@ void TestNotmuchWorker::queryPassesGenerationThrough() QCOMPARE(ready.size(), 1); QCOMPARE(ready.first().at(1).value<quint64>(), quint64(42)); QCOMPARE(finished.size(), 1); - QCOMPARE(finished.first().at(0).toInt(), 3); + QCOMPARE(finished.first().at(0).toInt(), 5); QCOMPARE(finished.first().at(1).value<quint64>(), quint64(42)); } @@ -513,7 +544,7 @@ void TestNotmuchWorker::queryStillWorksAfterWrite() worker.runQuery(QStringLiteral("*"), 2); QCOMPARE(ready.size(), 2); - QCOMPARE(ready.at(1).at(0).value<QVector<ThreadSummary>>().size(), 3); + QCOMPARE(ready.at(1).at(0).value<QVector<ThreadSummary>>().size(), 5); worker.applyTags(change.inverted()); } @@ -611,6 +642,113 @@ void TestNotmuchWorker::requestAllTagsOnUnreadableConfigEmitsError() QVERIFY(ready.isEmpty()); } +void TestNotmuchWorker::loadThreadMatchedOnlyDropsTheRest() +{ + // Thread A is two messages, and only the reply carries "hamsterwheel". + const QString threadId = threadIdOf(QStringLiteral("Release notes")); + QVERIFY(!threadId.isEmpty()); + + // Without the flag: both messages, the non-matching one marked as a stub. + // This is the reading pane's normal behaviour and must not change. + const QVector<MessageRef> whole = + messagesOfThread(threadId, QStringLiteral("hamsterwheel")); + QCOMPARE(whole.size(), 2); + + // With it: only the message that matched. The pane in a Sent view shows + // what the user sent, not the conversation their message started. + const QVector<MessageRef> matched = + messagesOfThread(threadId, QStringLiteral("hamsterwheel"), true); + QCOMPARE(matched.size(), 1); + QVERIFY(matched.at(0).matched); + QCOMPARE(matched.at(0).messageId, QStringLiteral("a2@example.org")); +} + +void TestNotmuchWorker::loadThreadMatchedOnlyWithNoQueryKeepsEverything() +{ + // No query means nothing was filtered, so every message counts as matched + // and the flag has nothing to drop. + // + // This does NOT prove the haveMatchSet guard in loadThread: ref.matched is + // already true for every message in this case, so removing that guard + // leaves this passing, confirmed by mutation. It pins the BEHAVIOUR, which + // is what a caller depends on, and the guard is a stated invariant rather + // than a branch a test can reach. + const QString threadId = threadIdOf(QStringLiteral("Release notes")); + QVERIFY(!threadId.isEmpty()); + + const QVector<MessageRef> all = messagesOfThread(threadId, QString(), true); + QCOMPARE(all.size(), 2); +} + +void TestNotmuchWorker::recipientsAreAbsentUnlessAskedFor() +{ + // Opt-in, and this is a PERFORMANCE contract rather than a preference. + // notmuch_message_get_header(m, "To") is not served from the index, it + // reads the message file: measured 2026-08-11 against a real database, + // folding every thread of a 4411-thread inbox took 38.2 seconds, 8.7 ms + // per thread, against 1.1 ms per thread over the 601-thread sent view. + // + // A version that always folds is correct in every other respect, which is + // exactly why it needs a test: nothing else here would notice. + const QVector<ThreadSummary> threads = + runQuery(QStringLiteral("subject:Preventivo")); + + QCOMPARE(threads.size(), 1); + QVERIFY2(threads.at(0).recipients.isEmpty(), + "the To header was read for a query that never asked for it"); +} + +void TestNotmuchWorker::recipientsAreFoldedWhenAskedFor() +{ + const QVector<ThreadSummary> one = + runQuery(QStringLiteral("subject:Preventivo"), + NotmuchWorker::NewestFirst, true); + QCOMPARE(one.size(), 1); + QCOMPARE(one.at(0).recipients, QStringLiteral("Mario Rossi")); + + // The comma-inside-a-display-name case, end to end through the worker + // rather than only against recipientSummary(): the header survives being + // written to a real maildir, indexed, and read back out of notmuch. + const QVector<ThreadSummary> many = + runQuery(QStringLiteral("subject:Riunione"), + NotmuchWorker::NewestFirst, true); + QCOMPARE(many.size(), 1); + + const QString summary = many.at(0).recipients; + QVERIFY2(summary.startsWith(QStringLiteral("Rossi, Mario")), + qPrintable(QStringLiteral("lost the quoted display name: %1") + .arg(summary))); + QVERIFY2(summary.endsWith(QStringLiteral("+1")), + qPrintable(QStringLiteral("three recipients did not collapse to " + "two plus one: %1").arg(summary))); +} + +void TestNotmuchWorker::recipientsCrossAQueuedCall() +{ + // The trap CLAUDE.md records for SortOrder, in the shape it takes for this + // argument. A bool is a registered metatype already, so this cannot fail + // the way an unregistered enum would, and the test exists to prove that + // rather than to assume it: the flag arriving as a default-constructed + // false would silently give an empty recipients column and nothing else. + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy ready(&worker, &NotmuchWorker::threadsReady); + + QVERIFY(QMetaObject::invokeMethod( + &worker, "runQuery", Qt::DirectConnection, + Q_ARG(QString, QStringLiteral("subject:Preventivo")), + Q_ARG(quint64, 1), + Q_ARG(NotmuchWorker::SortOrder, NotmuchWorker::NewestFirst), + Q_ARG(bool, true))); + + QVector<ThreadSummary> all; + for (const QList<QVariant> &args : ready) + all += args.at(0).value<QVector<ThreadSummary>>(); + + QCOMPARE(all.size(), 1); + QVERIFY2(!all.at(0).recipients.isEmpty(), + "the recipients flag was dropped crossing invokeMethod"); +} + void TestNotmuchWorker::requestCountsAnswersOneCountPerQuery() { NotmuchWorker worker(m_fixture.configPath()); @@ -626,7 +764,7 @@ void TestNotmuchWorker::requestCountsAnswersOneCountPerQuery() // Threads, not messages: thread A holds two messages and must count once, // which is the number the pane's "N in inbox" line claims to be showing. const QVector<int> counts = spy.at(0).at(0).value<QVector<int>>(); - QCOMPARE(counts, QVector<int>({ 1, 3, 0 })); + QCOMPARE(counts, QVector<int>({ 1, 5, 0 })); } void TestNotmuchWorker::requestCountsKeepsPositionOnAnInvalidQuery() @@ -656,7 +794,7 @@ void TestNotmuchWorker::requestCountsKeepsPositionOnAnInvalidQuery() // The queries either side keep their own answers, which is the property // the pane depends on. QCOMPARE(counts.at(0), 1); - QCOMPARE(counts.at(2), 3); + QCOMPARE(counts.at(2), 5); } void TestNotmuchWorker::requestDatabaseStatsCountsMessagesNotThreads() @@ -676,8 +814,8 @@ void TestNotmuchWorker::requestDatabaseStatsCountsMessagesNotThreads() // 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); + QCOMPARE(stats.messages, 6); + QCOMPARE(stats.threads, 5); 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"); diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index 947a6b3..1fb8a1f 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -81,6 +81,9 @@ private slots: void reconcileWithAnIdenticalResultChangesNothing(); void reconcileMovesAThreadBumpedByANewReply(); void reconcileKeepsAMovedRowsPersistentIndex(); + void flatModeOffersNoExpanderAndNoReplyCount(); + void flatModeIsOffByDefaultAndReversible(); + void recipientsReplaceTheSenderWhenPresent(); }; static ThreadSummary makeThread(const QString &id, const QString &subject) @@ -1420,5 +1423,106 @@ void TestThreadListModel::reconcileKeepsAMovedRowsPersistentIndex() QCOMPARE(moved.row(), 2); } +void TestThreadListModel::flatModeOffersNoExpanderAndNoReplyCount() +{ + // A sent message lives on its own: the user's mental model of "what I sent" + // is a list, not a set of conversations, and a thread pulled in whole shows + // the replies they received under a view that claims to be their outbox. + // + // Deliberately not a second model or a filtered query. The expander is + // driven by hasChildren() and the card's count by ReplyCountRole, both + // already here, so flat mode is those two answering differently. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), + QStringLiteral("Subject")) }); + + const QModelIndex thread = model.index(0, 0); + QVERIFY(thread.isValid()); + + // The tree shape, before anything is turned off. Guards the assertions + // below: a test whose subject was already flat would pass either way. + QVERIFY2(model.hasChildren(thread), + "the fixture thread is not expandable, so this proves nothing"); + QCOMPARE(model.data(thread, ThreadListModel::ReplyCountRole).toInt(), 1); + + model.setFlatMode(true); + + QVERIFY2(!model.hasChildren(thread), + "a flat list still offered an expander"); + QCOMPARE(model.data(thread, ThreadListModel::ReplyCountRole).toInt(), 0); + + // rowCount has to agree, or the view draws an expander it cannot open, or + // opens onto rows the card said were not there. + QCOMPARE(model.rowCount(thread), 0); + + // The thread itself is still a row. Flat means one row per thread, not + // fewer threads. + QCOMPARE(model.rowCount(), 1); +} + +void TestThreadListModel::flatModeIsOffByDefaultAndReversible() +{ + // The whole condition the user set for this feature: it must not leak into + // any other view. Off by default is what guarantees that, and returning to + // false has to restore the tree rather than leaving the model flattened + // for the next query. + ThreadListModel model; + model.appendBatch({ makeThread(QStringLiteral("t1"), + QStringLiteral("Subject")) }); + + const QModelIndex thread = model.index(0, 0); + QVERIFY2(model.hasChildren(thread), + "a fresh model is flat, so every ordinary view lost its replies"); + + model.setFlatMode(true); + QVERIFY(!model.hasChildren(thread)); + + model.setFlatMode(false); + QVERIFY2(model.hasChildren(thread), + "leaving flat mode did not restore the tree"); + QCOMPARE(model.data(thread, ThreadListModel::ReplyCountRole).toInt(), 1); +} + +void TestThreadListModel::recipientsReplaceTheSenderWhenPresent() +{ + // In a Sent view the sender is the user on every row, so the card shows + // who it went TO instead. One role, so the delegate needs no branch and + // cannot disagree with the model about which name a row is showing. + ThreadListModel model; + + ThreadSummary sent = makeThread(QStringLiteral("t1"), + QStringLiteral("Preventivo")); + sent.authors = QStringLiteral("You"); + sent.recipients = QStringLiteral("Mario Rossi"); + + // No recipients: an ordinary view, where authors is the answer. Same + // fixture otherwise, so the difference is the field and nothing else. + ThreadSummary received = makeThread(QStringLiteral("t2"), + QStringLiteral("Newsletter")); + received.authors = QStringLiteral("Carol"); + + model.appendBatch({ sent, received }); + + QCOMPARE(model.data(model.index(0, 0), + ThreadListModel::SendersRole).toString(), + QStringLiteral("Mario Rossi")); + QCOMPARE(model.data(model.index(1, 0), + ThreadListModel::SendersRole).toString(), + QStringLiteral("Carol")); + + // A thread whose To could not be parsed falls back rather than showing an + // empty name. The fold returns an empty string for a malformed or absent + // header, and a blank where a sender belongs reads as a rendering fault. + ThreadSummary unparseable = makeThread(QStringLiteral("t3"), + QStringLiteral("Broken")); + unparseable.authors = QStringLiteral("You"); + unparseable.recipients = QString(); + model.appendBatch({ unparseable }); + + QCOMPARE(model.data(model.index(2, 0), + ThreadListModel::SendersRole).toString(), + QStringLiteral("You")); +} + QTEST_MAIN(TestThreadListModel) #include "test_threadlistmodel.moc" |