diff options
| -rw-r--r-- | README.md | 13 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 33 | ||||
| -rw-r--r-- | src/keymap.cpp | 5 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 6 | ||||
| -rw-r--r-- | src/messageview.cpp | 115 | ||||
| -rw-r--r-- | src/messageview.h | 11 | ||||
| -rw-r--r-- | tests/test_messageview.cpp | 166 |
7 files changed, 336 insertions, 13 deletions
@@ -237,6 +237,18 @@ an attachment, so it is visible without opening the thread. It comes from the `attachment` tag notmuch applies while indexing, not from parsing the message, and costs no extra query. +## Message details + +The strip above the message pane says what it can say without guessing. A +thread holding **one message** shows its From, To and Cc under the subject. +A thread holding **several** shows the subject and the message count only: +From, To and Cc differ from message to message, and once you have replied there +is no single recipient the thread is addressed to, so naming one would be a +guess dressed as a fact. + +The **Details...** button beside the subject, or `Ctrl+Shift+D`, opens the full +headers of every message in the thread, numbered, as read-only plain text. + Under the message pane, one **Attachments (N)** button opens a list of the thread's attachments: which message each came from, its filename and its size, with a **Save** for each. With more than one, **Save all** writes them into a @@ -272,6 +284,7 @@ Defaults, all rebindable through `[keys]`: | `Ctrl+Space` | `complete_query` | Focus the query bar and offer completions | | `Ctrl+H` | `toggle_html` | Switch the thread between HTML and plain text | | `Ctrl+M` | `load_remote` | Load remote images for the current thread | +| `Ctrl+Shift+D` | `message_details` | Show the full headers of every message in the thread | | `Ctrl+Z` | `undo` | Undo the last tag change | | `Ctrl+G` | `sync` | Run the configured sync command | | `Ctrl+Q` | `quit` | Quit | 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 e945aeb..35d5937 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 @@ -41,7 +41,7 @@ taking that too literally. | # | Item | Cluster | Size | Status | |---|------|---------|------|--------| | 1 | Splitter/column widths do not survive restart | persistence | S | **done** | -| 2 | No way to see full message details (From/To/Cc/Subject) | information | M | open | +| 2 | No way to see full message details (From/To/Cc/Subject) | information | M | **done** | | 3 | Too few clickable affordances, shortcuts are the only route | discoverability | M | **done** | | 4 | Message-pane font size does not survive restart | persistence | S | **done** | | 5 | Thread list is cramped, poor readability | presentation | S | open | @@ -188,6 +188,12 @@ to compute they can be displayed as they stand. Splitting them into address lists, which would have needed GMime's `internet_address_list_parse` to survive a display name containing a comma, is not part of this item. +**Deferred, not rejected (user, 2026-08-04): a participants line for threads.** +The union-of-recipients idea is worth revisiting as its own pass, where it can +be designed as a participants list rather than smuggled in under a "To:" label +that misdescribes it. It needs the address parsing above, so it is a genuine +piece of work rather than a display tweak. Build this item as specced first. + **Noted for later, not now:** the user's mental model of the thread view differs from what was built. That is a separate refactor and should not be folded into this item. @@ -199,6 +205,31 @@ does. A `From` display name containing markup must never be able to inject into the label. The raw-header dialog should use `Qt::PlainText` and sidestep the question entirely. +### Outcome (done) + +Built as decided. `MessageView::updateHeader()` branches on the item count: one +message shows From, To and Cc under the subject, several show the subject and +the count exactly as before. `showDetailsDialog()` lists every message's +Subject, From, To, Cc, Date and Message-Id, numbered when there is more than +one, in a read-only `QPlainTextEdit`. A `Details...` button sits to the right of +the header, and `message_details` binds it to `Ctrl+Shift+D` (shifted because +`Ctrl+D` is delete, and the destructive binding keeps the key it had). + +- **Rendered and inspected**, not only asserted: both header shapes were grabbed + to PNG and looked at. The single-message case shows three rows under the + subject, the thread case shows the count and no recipients. +- **An empty Cc omits its row** rather than printing a label with nothing after + it, which reads as a rendering fault. +- **A test caught a latent flaw in an older test.** `attachmentButtonLabels()` + identified attachment buttons by excluding the one other button's label, so + the new details button was counted as an attachment the moment it existed. + It now finds the bar by object name and looks only at its children, which is + what it should have done: an exclusion list silently adopts every button + added later. + +**No address parsing was needed**, as the decision above anticipated. The header +prints `ParsedMessage::to` and `::cc` as they stand. + ## 3, 8, 9. Discoverability: menu bar, toolbar, shortcut reference Grouped because they are one piece of work. Item 3 is the complaint, items 8 diff --git a/src/keymap.cpp b/src/keymap.cpp index d8aacc5..b18f5d5 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -36,6 +36,7 @@ QStringList KeyMap::knownActions() QStringLiteral("complete_query"), QStringLiteral("toggle_html"), QStringLiteral("load_remote"), + QStringLiteral("message_details"), QStringLiteral("zoom_in"), QStringLiteral("zoom_out"), QStringLiteral("zoom_reset"), @@ -71,6 +72,10 @@ QList<QPair<QString, QString>> KeyMap::defaultBindings() { QStringLiteral("Ctrl+Space"), QStringLiteral("complete_query") }, { QStringLiteral("Ctrl+H"), QStringLiteral("toggle_html") }, { QStringLiteral("Ctrl+M"), QStringLiteral("load_remote") }, + // Shifted because Ctrl+D is delete. Both are "D for details/delete" + // words, and the destructive one keeps the unshifted key it already + // had rather than being moved to make room. + { QStringLiteral("Ctrl+Shift+D"), QStringLiteral("message_details") }, // Ctrl++ is what the '+' key really delivers on a layout where '+' is // unshifted, an Italian one among them, confirmed against the actual // keyboard. QTest::keyClick() cannot reproduce it, so a synthetic-input diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 0f169d9..a0ae7c4 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -458,6 +458,11 @@ void MainWindow::registerActions() tr("Load remote images for the current thread"), [this]() { m_messageView->loadRemoteContent(); }); + addAction(QStringLiteral("message_details"), tr("Message &details"), + tr("Show the full headers of every message in the thread"), + [this]() { + m_messageView->showDetailsDialog(); + }); addAction(QStringLiteral("zoom_in"), tr("Zoom &in"), tr("Enlarge the message text"), [this]() { m_messageView->zoomIn(); @@ -539,6 +544,7 @@ void MainWindow::buildMenus() viewMenu->addSeparator(); viewMenu->addAction(m_actions.value(QStringLiteral("toggle_html"))); viewMenu->addAction(m_actions.value(QStringLiteral("load_remote"))); + viewMenu->addAction(m_actions.value(QStringLiteral("message_details"))); viewMenu->addSeparator(); viewMenu->addAction(m_actions.value(QStringLiteral("zoom_in"))); viewMenu->addAction(m_actions.value(QStringLiteral("zoom_out"))); diff --git a/src/messageview.cpp b/src/messageview.cpp index 4423791..d7167bf 100644 --- a/src/messageview.cpp +++ b/src/messageview.cpp @@ -22,6 +22,8 @@ #include <QDesktopServices> #include <QDialog> #include <QDialogButtonBox> +#include <QFontDatabase> +#include <QPlainTextEdit> #include <QDir> #include <QFileDialog> #include <QHBoxLayout> @@ -124,6 +126,22 @@ MessageView::MessageView(QWidget *parent) m_headerLabel->setWordWrap(true); m_headerLabel->setTextInteractionFlags(Qt::TextSelectableByMouse); + // To the right of the header, per the user's decision: the summary answers + // "who is this from", this answers "what actually happened to it". A button + // and not only a shortcut, since "everything needs a memorized key" is the + // complaint this whole backlog started from. + m_detailsButton = new QPushButton(tr("Details..."), this); + m_detailsButton->setObjectName(QStringLiteral("messageDetails")); + m_detailsButton->setToolTip(tr("Show the full headers of every message")); + connect(m_detailsButton, &QPushButton::clicked, + this, &MessageView::showDetailsDialog); + m_detailsButton->hide(); + + auto *headerRow = new QHBoxLayout; + headerRow->addWidget(m_headerLabel, 1); + // Top-aligned so it stays put as the header grows to four rows. + headerRow->addWidget(m_detailsButton, 0, Qt::AlignTop); + m_blockedLabel = new QLabel(tr("Remote content blocked"), this); m_loadRemoteButton = new QPushButton(tr("Load remote content"), this); connect(m_loadRemoteButton, &QPushButton::clicked, @@ -135,6 +153,7 @@ MessageView::MessageView(QWidget *parent) blockedRow->addStretch(); m_attachmentBar = new QWidget(this); + m_attachmentBar->setObjectName(QStringLiteral("attachmentBar")); new QHBoxLayout(m_attachmentBar); // Tags live under the message rather than in the thread list, where @@ -143,7 +162,7 @@ MessageView::MessageView(QWidget *parent) m_tagStrip->hide(); auto *layout = new QVBoxLayout(this); - layout->addWidget(m_headerLabel); + layout->addLayout(headerRow); layout->addLayout(blockedRow); layout->addWidget(m_view, 1); layout->addWidget(m_attachmentBar); @@ -260,17 +279,103 @@ void MessageView::updateHeader() { if (m_items.isEmpty()) { m_headerLabel->clear(); + m_detailsButton->hide(); return; } + m_detailsButton->show(); + // The thread's subject comes from its first message; later replies carry // Re: prefixes that add nothing. const QString subject = m_items.first().message.subject; - m_headerLabel->setText( - QStringLiteral("<b>%1</b><br><small>%2</small>") - .arg(subject.toHtmlEscaped(), - tr("%n message(s) in thread", "", m_items.size()))); + QString text = QStringLiteral("<b>%1</b>").arg(subject.toHtmlEscaped()); + + // The header adapts to what it can say honestly. From, To and Cc are + // per-message, and the pane shows a whole thread, so they are only + // unambiguous when the thread holds exactly one message. For a real thread + // the recipient differs message to message (once the user replies, one is + // addressed to them and the next to the other party), and neither the union + // nor the intersection is "the" recipient. Rather than pick one or compute + // a participants list, the thread case says only the subject and the count, + // and the per-message detail belongs to the dialog. + if (m_items.size() == 1) { + const ParsedMessage &message = m_items.first().message; + + // Every value here is attacker-controlled and the label is RichText, so + // escaping is not cosmetic: an unescaped From injects markup into the + // application's own chrome rather than into the sandboxed page. + auto row = [&text](const QString &label, const QString &value) { + if (value.isEmpty()) + return; // An empty row reads as a rendering fault. + text += QStringLiteral("<br><small>%1 %2</small>") + .arg(label.toHtmlEscaped(), value.toHtmlEscaped()); + }; + + row(tr("From:"), message.from); + row(tr("To:"), message.to); + row(tr("Cc:"), message.cc); + } else { + text += QStringLiteral("<br><small>%1</small>") + .arg(tr("%n message(s) in thread", "", m_items.size())); + } + + m_headerLabel->setText(text); +} + +void MessageView::showDetailsDialog() +{ + if (m_items.isEmpty()) + return; + + QDialog dialog(this); + dialog.setWindowTitle(tr("Message details")); + + auto *layout = new QVBoxLayout(&dialog); + + auto *details = new QPlainTextEdit(&dialog); + details->setReadOnly(true); + // A monospaced font keeps a long Received chain readable as the wrapped + // record it is. + details->setFont(QFontDatabase::systemFont(QFontDatabase::FixedFont)); + details->setLineWrapMode(QPlainTextEdit::NoWrap); + + // setPlainText, and a QPlainTextEdit rather than a label: this dialog shows + // header values verbatim, and those come from strangers. Plain text cannot + // interpret markup, so there is nothing here to escape and nothing that + // could render. + QString text; + for (int i = 0; i < m_items.size(); ++i) { + const ParsedMessage &message = m_items.at(i).message; + + if (i > 0) + text += QLatin1Char('\n'); + if (m_items.size() > 1) + text += tr("--- Message %1 of %2 ---") + .arg(i + 1).arg(m_items.size()) + QLatin1Char('\n'); + + auto line = [&text](const QString &label, const QString &value) { + if (!value.isEmpty()) + text += label + QLatin1Char(' ') + value + QLatin1Char('\n'); + }; + + line(tr("Subject:"), message.subject); + line(tr("From:"), message.from); + line(tr("To:"), message.to); + line(tr("Cc:"), message.cc); + line(tr("Date:"), message.date); + line(tr("Message-Id:"), message.messageId); + } + details->setPlainText(text); + + layout->addWidget(details); + + auto *buttons = new QDialogButtonBox(QDialogButtonBox::Close, &dialog); + connect(buttons, &QDialogButtonBox::rejected, &dialog, &QDialog::reject); + layout->addWidget(buttons); + + dialog.resize(700, 400); + dialog.exec(); } void MessageView::render() diff --git a/src/messageview.h b/src/messageview.h index 244a1ba..1be17f8 100644 --- a/src/messageview.h +++ b/src/messageview.h @@ -65,6 +65,15 @@ public: /// Tags of the thread on display, shown as chips along the bottom. void setTags(const QStringList &tags); + /// The full headers of every message in the thread, read-only. Also + /// reachable from the button beside the header; public so the window's + /// message_details action can call it. + /// + /// Plain text, not rich: header values are attacker-controlled and this + /// dialog exists to show them verbatim, so the format that cannot + /// interpret markup is the right one. + void showDetailsDialog(); + /// The body zoom factor. Chromium's own range is roughly 0.25 to 5.0; /// these are tighter, since a pane at either extreme is unusable and the /// only visible way back is a menu entry the user cannot read. @@ -126,6 +135,7 @@ private: /// Every attachment in the thread, in the order the messages render. QList<Attachment> allAttachments() const; + QList<ThreadRenderItem> m_items; bool m_preferHtml = true; @@ -137,6 +147,7 @@ private: QLabel *m_headerLabel = nullptr; QLabel *m_blockedLabel = nullptr; QPushButton *m_loadRemoteButton = nullptr; + QPushButton *m_detailsButton = nullptr; QWidget *m_attachmentBar = nullptr; TagStrip *m_tagStrip = nullptr; }; diff --git a/tests/test_messageview.cpp b/tests/test_messageview.cpp index 278270a..57d7b42 100644 --- a/tests/test_messageview.cpp +++ b/tests/test_messageview.cpp @@ -16,6 +16,7 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ +#include <QLabel> #include <QPushButton> #include <QSignalSpy> #include <QWebEngineUrlScheme> @@ -41,6 +42,11 @@ private slots: void zoomSurvivesANewDocument(); void attachmentBarOffersEveryAttachment(); void attachmentBarClearsBetweenThreads(); + void singleMessageHeaderShowsFromToAndCc(); + void threadHeaderShowsOnlySubjectAndCount(); + void headerEscapesUntrustedValues(); + void headerOmitsAnAbsentCc(); + void detailsDialogIsOfferedForEveryThread(); private: QWebEngineView *webViewOf(MessageView *view) const @@ -233,16 +239,20 @@ void TestMessageView::zoomSurvivesANewDocument() QCOMPARE(view.zoomFactor(), 1.5); } -/// The buttons in the attachment bar, by their label. Excludes the -/// "Load remote content" button, which lives in the same pane but is not part -/// of the bar. +/// The buttons in the attachment bar, by their label. +/// +/// Identified by the bar being their parent, not by excluding the labels of +/// the other buttons in the pane: an exclusion list silently adopts every +/// button added later, and it did, counting the details button as an +/// attachment the moment one was added beside the header. static QStringList attachmentButtonLabels(MessageView *view) { QStringList labels; - for (QPushButton *button : view->findChildren<QPushButton *>()) { - if (button->text() != QStringLiteral("Load remote content")) - labels.append(button->text()); - } + QWidget *bar = view->findChild<QWidget *>(QStringLiteral("attachmentBar")); + if (!bar) + return labels; + for (QPushButton *button : bar->findChildren<QPushButton *>()) + labels.append(button->text()); return labels; } @@ -340,5 +350,147 @@ void TestMessageView::attachmentBarClearsBetweenThreads() QVERIFY(attachmentButtonLabels(&view).isEmpty()); } +/// The header strip's text. It is rich text, so the assertions below are +/// against markup as well as content. +static QString headerTextOf(MessageView *view) +{ + for (QLabel *label : view->findChildren<QLabel *>()) { + if (label->textFormat() == Qt::RichText) + return label->text(); + } + return QString(); +} + +/// One message, from the same shape the other tests build. +static ThreadRenderItem oneMessage() +{ + ParsedMessage message; + message.ok = true; + message.from = QStringLiteral("Sender <sender@example.org>"); + message.to = QStringLiteral("Recipient <recipient@example.org>"); + message.cc = QStringLiteral("Copied <copied@example.org>"); + message.subject = QStringLiteral("Quarterly report"); + message.date = QStringLiteral("Mon, 4 Aug 2026 09:00:00 +0200"); + message.plainBody = QStringLiteral("body"); + + ThreadRenderItem item; + item.message = message; + item.cidPrefix = QStringLiteral("m0"); + item.expanded = true; + return item; +} + +void TestMessageView::singleMessageHeaderShowsFromToAndCc() +{ + // MimeParser filled To and Cc all along; HtmlBuilder simply never + // interpolated them, so they were parsed and dropped. With one message in + // the thread every field is unambiguous, which is why this is the case that + // shows them. + MessageView view; + view.showThread({ oneMessage() }); + + const QString header = headerTextOf(&view); + QVERIFY2(header.contains(QStringLiteral("sender@example.org")), + qPrintable(QStringLiteral("no From in '%1'").arg(header))); + QVERIFY2(header.contains(QStringLiteral("recipient@example.org")), + qPrintable(QStringLiteral("no To in '%1'").arg(header))); + QVERIFY2(header.contains(QStringLiteral("copied@example.org")), + qPrintable(QStringLiteral("no Cc in '%1'").arg(header))); + QVERIFY(header.contains(QStringLiteral("Quarterly report"))); +} + +void TestMessageView::threadHeaderShowsOnlySubjectAndCount() +{ + // A thread's To differs per message: once the user replies, one message is + // addressed to them and the next to the other party. Rather than pick a + // message arbitrarily or compute a participants list, the thread header + // says only what it can say honestly. Per-message detail is the dialog's + // job. This test is what stops a recipient line reappearing here. + ThreadRenderItem first = oneMessage(); + + ThreadRenderItem second = oneMessage(); + second.message.from = QStringLiteral("Recipient <recipient@example.org>"); + second.message.to = QStringLiteral("Sender <sender@example.org>"); + second.message.cc = QString(); + second.cidPrefix = QStringLiteral("m1"); + + MessageView view; + view.showThread({ first, second }); + + const QString header = headerTextOf(&view); + QVERIFY(header.contains(QStringLiteral("Quarterly report"))); + QVERIFY2(!header.contains(QStringLiteral("recipient@example.org")), + qPrintable(QStringLiteral("a recipient leaked into '%1'") + .arg(header))); + QVERIFY2(!header.contains(QStringLiteral("copied@example.org")), + qPrintable(QStringLiteral("a Cc leaked into '%1'").arg(header))); +} + +void TestMessageView::headerEscapesUntrustedValues() +{ + // Every one of these values comes from a stranger, and the label is + // Qt::RichText, so an unescaped From is markup injection into the chrome of + // the application rather than into the sandboxed page. + ThreadRenderItem item = oneMessage(); + item.message.from = + QStringLiteral("<b>bold</b> <script>x</script> <evil@example.org>"); + item.message.to = QStringLiteral("<i>italic</i> <to@example.org>"); + item.message.cc = QStringLiteral("<u>under</u> <cc@example.org>"); + item.message.subject = QStringLiteral("<h1>huge</h1>"); + + MessageView view; + view.showThread({ item }); + + const QString header = headerTextOf(&view); + QVERIFY2(!header.contains(QStringLiteral("<b>bold</b>")), + qPrintable(QStringLiteral("unescaped From in '%1'").arg(header))); + QVERIFY(!header.contains(QStringLiteral("<script>"))); + QVERIFY(!header.contains(QStringLiteral("<i>italic</i>"))); + QVERIFY(!header.contains(QStringLiteral("<u>under</u>"))); + QVERIFY(!header.contains(QStringLiteral("<h1>huge</h1>"))); + + // Escaped, not merely stripped: the text must still be readable. + QVERIFY(header.contains(QStringLiteral("<b>bold</b>"))); +} + +void TestMessageView::headerOmitsAnAbsentCc() +{ + // Most mail carries no Cc. An empty label with nothing after it reads as a + // rendering fault, so the row is omitted rather than left blank. + ThreadRenderItem item = oneMessage(); + item.message.cc = QString(); + + MessageView view; + view.showThread({ item }); + + const QString header = headerTextOf(&view); + QVERIFY(header.contains(QStringLiteral("recipient@example.org"))); + QVERIFY2(!header.contains(QStringLiteral("Cc")), + qPrintable(QStringLiteral("empty Cc row left in '%1'") + .arg(header))); +} + +void TestMessageView::detailsDialogIsOfferedForEveryThread() +{ + // The button is the discoverable half of the feature: the shortcut alone + // repeats the complaint that started this backlog. It must be present for a + // thread as well as a single message, since a thread is exactly the case + // where the header withholds the most. + MessageView view; + view.showThread({ oneMessage() }); + QVERIFY(view.findChild<QPushButton *>(QStringLiteral("messageDetails"))); + + ThreadRenderItem second = oneMessage(); + second.cidPrefix = QStringLiteral("m1"); + view.showThread({ oneMessage(), second }); + QVERIFY(view.findChild<QPushButton *>(QStringLiteral("messageDetails"))); + + // And it goes away when there is nothing to describe. + view.clear(); + QPushButton *button = + view.findChild<QPushButton *>(QStringLiteral("messageDetails")); + QVERIFY(!button || !button->isVisible()); +} + QTEST_MAIN(TestMessageView) #include "test_messageview.moc" |