From 84e3205ddcba3263e6c07fa314437318309d4b76 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Fri, 21 Aug 2026 20:12:30 +0200 Subject: feat(compose): register the six compose actions, item 123 Handlers are empty for now; this commit is the registration, so the three coverage tests guard every later task rather than being satisfied at the end. Two corrections to the spec, both found in the code rather than assumed. It calls for a new top-level Message menu and one already exists, so these join it; two menus named Message would be a defect. And it says every action needs a binding, which item 132 changed while this was being planned: save_message ships with no chord, since it is the rarely-used escape hatch and menu reachability is now the rule that must hold. reply_no_quote shares reply's icon and is added to the no-duplicate-icons exception list for the same reason the five thread actions are: it never reaches the toolbar, and a menu entry always carries its text. That list is renamed menuOnlySharedIconActions, after the property that earns the exemption rather than the tier that first needed it. Bindings are provisional. The user intends to rework them, and Ctrl+Alt+R for reply_no_quote is an imperfect fit since that tier elsewhere means a wider scope rather than a variant. The six labels went through a mnemonic pass that nothing enforced before. Four of them collided inside the Message menu on first writing, and the whole class was invisible to a green suite: Qt does not error on a duplicate mnemonic, it cycles the highlight instead of activating, so the key simply stops working. Item 57 had already decided this rule by rejecting a label that would have collided, but it lived in prose and in one test's comment, which is precisely why it was broken again here. noMenuHasTwoEntriesSharingAMnemonic() enforces it now, scoped per menu since a mnemonic resolves among the open menu's entries, and keyed on QKeySequence::mnemonic() rather than on parsing & by hand, because && is a literal ampersand and only Qt answers which key it will dispatch. Three pre-existing collisions are a named freeze list rather than a silent fix or a narrowed test: Alt+R three ways and Alt+S twice in Message, Alt+O in View. Renaming entries a user has had in their fingers since 0.1.0 belongs to the shortcuts rework, and the freeze is written as exact groups so a new entry joining any of them still fails. Two of the test's own design choices came from mutation checks that failed for the right reason while reporting the wrong thing. Reporting collisions as pairs was order-dependent, so a new colliding entry re-keyed a frozen pair and the fresh defect read as "a frozen collision no longer happens"; matching frozen entries by whole string broke the same way, since a growing group stopped matching its frozen text. It reports whole groups and matches on menu plus key. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FXF741wz4SY7j5dqvAxMU5 --- src/keymap.cpp | 33 +++++++ src/mainwindow.cpp | 87 ++++++++++++++++++ src/mainwindow.h | 21 +++++ tests/test_mainwindow.cpp | 195 +++++++++++++++++++++++++++++++++++++++- translations/qtmaildir_it_IT.ts | 48 ++++++++++ 5 files changed, 380 insertions(+), 4 deletions(-) diff --git a/src/keymap.cpp b/src/keymap.cpp index 76c6b60..0df8450 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -54,6 +54,16 @@ QStringList KeyMap::knownActions() QStringLiteral("spam_thread"), QStringLiteral("toggle_unread_thread"), QStringLiteral("flag_thread"), + // Compose and send (item 123). save_message deliberately carries no + // default chord: since item 132 a shortcut is a chosen subset rather + // than a requirement, and writing the raw message to a file is the + // rarely-used escape hatch. Menu reachability is the rule that holds. + QStringLiteral("compose"), + QStringLiteral("reply"), + QStringLiteral("reply_all"), + QStringLiteral("reply_no_quote"), + QStringLiteral("forward"), + QStringLiteral("save_message"), QStringLiteral("focus_query"), QStringLiteral("complete_query"), QStringLiteral("save_query"), @@ -98,6 +108,29 @@ QList> KeyMap::defaultBindings() { QStringLiteral("Alt+Down"), QStringLiteral("next_thread") }, { QStringLiteral("Alt+Up"), QStringLiteral("prev_thread") }, { QStringLiteral("Return"), QStringLiteral("open_thread") }, + // Compose and send (item 123), listed where the Message menu presents + // them: composing sits above organising. + // + // PROVISIONAL. The user intends to rework the bindings, and + // Ctrl+Alt+R for reply_no_quote is an imperfect fit: the Ctrl+Alt tier + // elsewhere means a WIDER SCOPE (the five whole-thread actions), not a + // variant of the same scope. + // + // Each was checked against every sequence in this table, not merely + // against the lines above it: these sit near the top, so most of the + // table is BELOW them, Ctrl+Shift+U and Ctrl+Shift+S among it. + // Checking only upwards would miss exactly those. The near misses: + // Ctrl+R is restore, Ctrl+A is select_all and Ctrl+Alt+S is + // spam_thread, so none of these five is a reuse. + // + // save_message gets none. Item 132 made a chord a chosen subset rather + // than a requirement, and this is the escape hatch nobody presses a + // key for. + { QStringLiteral("Ctrl+N"), QStringLiteral("compose") }, + { QStringLiteral("Ctrl+Shift+R"), QStringLiteral("reply") }, + { QStringLiteral("Ctrl+Shift+A"), QStringLiteral("reply_all") }, + { QStringLiteral("Ctrl+Alt+R"), QStringLiteral("reply_no_quote") }, + { QStringLiteral("Ctrl+Shift+F"), QStringLiteral("forward") }, { QStringLiteral("Ctrl+E"), QStringLiteral("archive") }, // Del FIRST, and the order matters twice over. defaultSequenceFor() // returns the first match, and sequenceFor() prefers any binding that diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index dc416ca..5155c09 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -758,6 +758,27 @@ void MainWindow::buildUi() setWindowTitle(QStringLiteral("qtmaildir %1").arg(QTMAILDIR_VERSION)); } +// The six compose handlers, empty until the composer exists (item 123). +// +// Deliberately empty rather than absent. Registering the actions first means +// everyKnownActionIsRegistered, everyActionCarriesAnIcon and +// everyActionIsReachableFromAMenu cover them while the composer is being +// built; a menu entry that does nothing yet is a smaller defect than an action +// nobody can reach, which is what those tests exist to catch. +void MainWindow::composeNew() +{ +} + +void MainWindow::composeReply(ComposeContext::Kind kind, bool quote) +{ + Q_UNUSED(kind); + Q_UNUSED(quote); +} + +void MainWindow::saveDisplayedMessage() +{ +} + QAction *MainWindow::addAction(const QString &name, const QString &text, const QString &description, const std::function &handler) @@ -1124,6 +1145,34 @@ void MainWindow::registerActions() addAction(QStringLiteral("quit"), tr("&Quit"), tr("Quit qtmaildir"), [this]() { close(); }); + // Compose and send (item 123). The handlers are empty: this is the + // registration, so the three coverage tests + // (everyKnownActionIsRegistered, everyActionCarriesAnIcon and + // everyActionIsReachableFromAMenu) cover the composer from the first + // commit rather than being satisfied once it is finished. + // + // Reply and reply-without-quoting are the same Kind with and without a + // seeded body, which is why the quoting is a parameter rather than a + // fourth Kind: the recipients, the subject prefix and the threading + // headers are identical, and only the body differs. + addAction(QStringLiteral("compose"), tr("&New message"), + tr("Compose a new message"), [this]() { composeNew(); }); + addAction(QStringLiteral("reply"), tr("Re&ply"), + tr("Reply to the displayed message"), + [this]() { composeReply(ComposeContext::Kind::Reply, true); }); + addAction(QStringLiteral("reply_all"), tr("Reply to a&ll"), + tr("Reply to the sender and every other recipient"), + [this]() { composeReply(ComposeContext::Kind::ReplyAll, true); }); + addAction(QStringLiteral("reply_no_quote"), tr("Reply without "ing"), + tr("Reply with an empty body"), + [this]() { composeReply(ComposeContext::Kind::Reply, false); }); + addAction(QStringLiteral("forward"), tr("&Forward"), + tr("Forward the displayed message"), + [this]() { composeReply(ComposeContext::Kind::Forward, true); }); + addAction(QStringLiteral("save_message"), tr("Sa&ve message as..."), + tr("Write the raw message to a file"), + [this]() { saveDisplayedMessage(); }); + // A binding the user wrote for an action that does not exist would be // silently dead. KeyMap warns about unknown names, but only a check here // catches the reverse: a known action nothing implements. @@ -1154,6 +1203,18 @@ void MainWindow::buildMenus() editMenu->addAction(m_actions.value(QStringLiteral("select_all"))); auto *messageMenu = menuBar()->addMenu(tr("&Message")); + // Composing sits above organising (item 123). The spec called for a new + // top-level Message menu and this one already existed, so the six join it: + // two menus named Message would be a defect. + messageMenu->addAction(m_actions.value(QStringLiteral("compose"))); + messageMenu->addSeparator(); + messageMenu->addAction(m_actions.value(QStringLiteral("reply"))); + messageMenu->addAction(m_actions.value(QStringLiteral("reply_all"))); + messageMenu->addAction(m_actions.value(QStringLiteral("reply_no_quote"))); + messageMenu->addAction(m_actions.value(QStringLiteral("forward"))); + messageMenu->addSeparator(); + messageMenu->addAction(m_actions.value(QStringLiteral("save_message"))); + messageMenu->addSeparator(); messageMenu->addAction(m_actions.value(QStringLiteral("archive"))); messageMenu->addAction(m_actions.value(QStringLiteral("delete"))); // Beside Delete, whose inverse it is. Greyed outside the trash view @@ -1282,6 +1343,22 @@ void MainWindow::buildMenus() { QStringLiteral("spam_thread"), QStringLiteral("mail-mark-junk") }, { QStringLiteral("toggle_unread_thread"), QStringLiteral("mail-mark-unread") }, { QStringLiteral("flag_thread"), QStringLiteral("mail-mark-important") }, + + // Compose and send (item 123). reply_no_quote SHARES reply's icon for + // the same reason the five above share theirs: it never reaches the + // toolbar, it is a menu entry that always carries its text, and + // "Reply without quoting" beside the reply icon is the honest pairing. + // It is named in the exception list in noTwoActionsShareAnIcon(), so + // putting it on the toolbar fails that test rather than passing + // silently. + { QStringLiteral("compose"), QStringLiteral("mail-message-new") }, + { QStringLiteral("reply"), QStringLiteral("mail-reply-sender") }, + { QStringLiteral("reply_all"), QStringLiteral("mail-reply-all") }, + { QStringLiteral("reply_no_quote"), QStringLiteral("mail-reply-sender") }, + { QStringLiteral("forward"), QStringLiteral("mail-forward") }, + // NOT bookmark-new, which save_query uses: this really does write a + // file the user names, which is exactly what the disk shape means. + { QStringLiteral("save_message"), QStringLiteral("document-save-as") }, }; for (auto it = themeIcons.cbegin(); it != themeIcons.cend(); ++it) { QAction *action = m_actions.value(it.key()); @@ -1340,6 +1417,16 @@ void MainWindow::buildMenus() // anything this code can see. const int iconSize = m_config.toolbarIconSize(); toolBar->setIconSize(QSize(iconSize, iconSize)); + + // First, because composing and replying are what a user reaches for most + // (item 123). These TWO only: the other four are menu-and-key, which is + // what keeps the no-duplicate-icons rule satisfiable, since reply_no_quote + // shares reply's icon and an icon-only toolbar would make the two buttons + // indistinguishable. + toolBar->addAction(m_actions.value(QStringLiteral("compose"))); + toolBar->addAction(m_actions.value(QStringLiteral("reply"))); + toolBar->addSeparator(); + QAction *syncAction = m_actions.value(QStringLiteral("sync")); // Carried over from the QPushButton this replaced: with no command // configured the control is disabled, and the tooltip is the only thing diff --git a/src/mainwindow.h b/src/mainwindow.h index 8e483d2..a3cd0ec 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -594,6 +594,27 @@ private: /// that populates. void showMaildirOverview(); + /// Opens a composer on a blank message (item 123). + /// + /// Empty for now. This is the registration commit: the six actions exist, + /// carry icons, sit in the Message menu and are covered by the three + /// coverage tests, so those tests guard the composer while it is built + /// rather than being satisfied once at the end. ComposeWindow does not + /// exist yet. + void composeNew(); + + /// Opens a composer seeded from the displayed message (item 123). + /// + /// `kind` chooses reply, reply-all or forward; `quote` is what separates + /// reply from reply-without-quoting, which are the same kind with and + /// without a seeded body. Empty for now, as above. + void composeReply(ComposeContext::Kind kind, bool quote); + + /// Writes the displayed message's raw file somewhere the user chooses. + /// + /// Empty for now, as above. + void saveDisplayedMessage(); + /// 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, diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 4d70a29..0ad45ee 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -331,6 +331,7 @@ private slots: void everyActionCarriesAnIcon(); void everyActionIsReachableFromAMenu(); + void noMenuHasTwoEntriesSharingAMnemonic(); void theToolbarDoesNotOverrideTheDesktopButtonStyle(); void theImportantActionIsLabelledImportant(); void theImportantActionStillWritesTheFlaggedTag(); @@ -6444,6 +6445,185 @@ void TestMainWindow::everyActionIsReachableFromAMenu() .arg(unreachable.join(QStringLiteral(", "))))); } +void TestMainWindow::noMenuHasTwoEntriesSharingAMnemonic() +{ + // The sibling of everyActionIsReachableFromAMenu(), and it exists because + // the rule it enforces had lived only in prose and in one other test's + // COMMENT, and was duly broken the first time a batch of entries was added + // to a menu (item 123: `&Reply` against the pre-existing `&Restore from + // trash`, both Alt+R). + // + // Qt does not error on a duplicate mnemonic. It CYCLES between the + // colliding entries instead of activating either, so the key silently + // stops working and merely moves a highlight. That is worse than it + // sounds in the Message menu, where `restore` is deliberately greyed + // outside the trash view: the ordinary case was pressing Alt+R and landing + // on a disabled entry. + // + // Item 57 already decided this is a property rather than a taste. It + // rejected the label "Starred" for `flag` precisely because it would have + // collided with `Mark &spam`, and theImportantActionIsLabelledImportant() + // pins the surviving label with that reasoning in its comment. A decision + // recorded only in prose is one nobody re-derives. + // + // Scoped PER MENU, which is what the collision actually is: a mnemonic is + // resolved among the entries of the menu that is open, so the same letter + // in File and in View is not a conflict. + const Config config; + MainWindow window(config); + + auto *bar = window.menuBar(); + QVERIFY(bar); + + // The menu bar's own top-level titles are one such scope too, so the walk + // starts by treating the bar as a menu and then descends. + QList>> scopes; + scopes.append({ QStringLiteral("the menu bar"), bar->actions() }); + + QList pending; + const auto topLevel = bar->actions(); + for (QAction *action : topLevel) { + if (action->menu()) + pending.append(action->menu()); + } + QVERIFY2(!pending.isEmpty(), "the menu bar holds no menus"); + + while (!pending.isEmpty()) { + QMenu *menu = pending.takeFirst(); + const auto entries = menu->actions(); + scopes.append({ menu->title(), entries }); + for (QAction *entry : entries) { + if (QMenu *sub = entry->menu()) + pending.append(sub); + } + } + + // The four collisions that PREDATE this test, measured by running it + // against the tree before item 123 touched any label. They are listed + // rather than fixed, and rather than being hidden by narrowing the test, + // because renaming a shipped menu entry is the user's call and not a + // test's: three of them are in menus a user has had in their fingers + // since 0.1.0. + // + // Listed as exact pairs, not as "ignore Alt+R", so this is a freeze and + // not an amnesty: a NEW entry colliding on any of these same keys still + // fails, because its pair is not on this list. Fixing one is then a + // one-line deletion here, which is the point of writing them out. + // Written as the FULL GROUP of labels sharing one key in one menu, not as + // a pair. A pair is keyed on which entry the walk happened to see first, + // so adding a colliding entry ABOVE a frozen one silently re-pairs it and + // the new defect gets reported as "a frozen collision no longer happens", + // which names the wrong thing entirely. Measured: reinstating `&Reply` + // did exactly that before this was changed. A group is order-independent, + // so a new entry grows the group and fails as a new collision. + static const QStringList knownPreExistingCollisions = { + QStringLiteral("&Message: Alt+R shared by \"&Restore from trash\", \"Mark all &read\", \"Tagging &rules...\""), + QStringLiteral("&Message: Alt+S shared by \"Mark &spam\", \"Find &stranded deleted mail\""), + QStringLiteral("&View: Alt+O shared by \"&Open thread\", \"Zoom &out\""), + }; + + QStringList collisions; + int compared = 0; + + for (const auto &scope : scopes) { + // Keyed on the mnemonic Qt itself derives, not on a hand-parsed '&'. + // The question is which key Qt will dispatch, and only Qt answers it: + // "&&" is a literal ampersand and carries no mnemonic at all. + // + // A QMap rather than a QHash so the groups come out in a stable key + // order, which is what lets the frozen list above be written once and + // stay matching. + QMap byMnemonic; + for (QAction *entry : scope.second) { + if (entry->isSeparator()) + continue; + const QKeySequence mnemonic = QKeySequence::mnemonic(entry->text()); + if (mnemonic.isEmpty()) + continue; + ++compared; + byMnemonic[mnemonic.toString(QKeySequence::NativeText)] + .append(QStringLiteral("\"%1\"").arg(entry->text())); + } + + for (auto it = byMnemonic.cbegin(); it != byMnemonic.cend(); ++it) { + if (it.value().size() < 2) + continue; + // Names the menu, the key and EVERY label in the group, so a + // future failure says what to rename without anyone going looking. + collisions.append(QStringLiteral("%1: %2 shared by %3") + .arg(scope.first, it.key(), + it.value().join(QStringLiteral(", ")))); + } + } + + // The guard, and it is not ceremonial: every assertion below is a loop + // that reports success when it runs zero times. A walk that found no + // mnemonics at all would pass this test against any label whatsoever. + QVERIFY2(compared > 20, + qPrintable(QStringLiteral("only %1 menu entries carried a " + "mnemonic, so this probe measured " + "almost nothing") + .arg(compared))); + + // Matched on the menu and key only, with the labels compared separately + // below. Comparing whole strings made a GROWING group read as a frozen one + // disappearing: adding `&Reply` took Alt+R from three labels to four, the + // frozen three-label string stopped matching, and the failure said "this + // collision no longer happens" about the very key that had just got worse. + // Measured twice, once per attempt, which is why the two questions are + // asked separately. + const auto scopeAndKey = [](const QString &collision) { + return collision.left(collision.indexOf(QStringLiteral(" shared by "))); + }; + + QHash frozen; + for (const QString &known : knownPreExistingCollisions) + frozen.insert(scopeAndKey(known), known); + + QStringList unexpected; + QSet stillPresent; + for (const QString &collision : collisions) { + const QString key = scopeAndKey(collision); + const auto known = frozen.constFind(key); + if (known == frozen.constEnd()) { + // A collision on a key nothing froze: entirely new. + unexpected.append(collision); + continue; + } + stillPresent.insert(key); + if (*known != collision) { + // The key was already colliding, but the CAST has changed, which + // for a frozen entry means an entry joined it. Reported as the + // new collision it is, naming both what was frozen and what is + // there now. + unexpected.append( + QStringLiteral("%1 (frozen as [%2], now [%3])") + .arg(key, *known, collision)); + } + } + + // A frozen entry that has since been FIXED must not stay on the list + // silently, or the list becomes a place stale claims accumulate. + QStringList stale; + for (const QString &known : knownPreExistingCollisions) { + if (!stillPresent.contains(scopeAndKey(known))) + stale.append(known); + } + QVERIFY2(stale.isEmpty(), + qPrintable(QStringLiteral("%1 frozen collision(s) no longer " + "happen, so delete them from " + "knownPreExistingCollisions: %2") + .arg(stale.size()) + .arg(stale.join(QStringLiteral("; "))))); + + QVERIFY2(unexpected.isEmpty(), + qPrintable(QStringLiteral("%1 menu mnemonic collision(s), where " + "Qt cycles the highlight instead of " + "activating: %2") + .arg(unexpected.size()) + .arg(unexpected.join(QStringLiteral("; "))))); +} + void TestMainWindow::everyActionCarriesAnIcon() { // Item 56. The complaint was inconsistency, not absence: eight actions had @@ -6967,15 +7147,22 @@ void TestMainWindow::noTwoActionsShareAnIcon() // the words saying which. Giving them five invented shapes would be less // clear than the pairing. // + // reply_no_quote joined them in item 123 for exactly the same reason: it + // shares reply's icon, it is a menu entry that always carries its text, + // and it is not on the toolbar. The list is therefore no longer only the + // thread tier, which is why it is named for the PROPERTY that earns the + // exemption rather than for the tier that first needed it. + // // Named as an exception list rather than by asking the toolbar what it // holds, so that PUTTING one of these on the toolbar fails this test // rather than silently passing it. - static const QStringList menuOnlyThreadActions = { + static const QStringList menuOnlySharedIconActions = { QStringLiteral("archive_thread"), QStringLiteral("delete_thread"), QStringLiteral("spam_thread"), QStringLiteral("toggle_unread_thread"), QStringLiteral("flag_thread"), + QStringLiteral("reply_no_quote"), }; const Config config; @@ -6986,7 +7173,7 @@ void TestMainWindow::noTwoActionsShareAnIcon() // may sit on the toolbar. auto *toolBar = window.findChild(); QVERIFY(toolBar); - for (const QString &name : menuOnlyThreadActions) { + for (const QString &name : menuOnlySharedIconActions) { auto *action = window.findChild(name); QVERIFY2(action, qPrintable(QStringLiteral("no action named %1").arg(name))); QVERIFY2(!toolBar->actions().contains(action), @@ -7006,7 +7193,7 @@ void TestMainWindow::noTwoActionsShareAnIcon() QVERIFY2(action, qPrintable(QStringLiteral("no action named %1").arg(name))); if (!action->icon().isNull()) ++withIcons; - if (menuOnlyThreadActions.contains(name)) + if (menuOnlySharedIconActions.contains(name)) continue; if (action->icon().isNull()) continue; @@ -7033,7 +7220,7 @@ void TestMainWindow::noTwoActionsShareAnIcon() // And the exception list did not swallow the comparison itself. QCOMPARE(compared, KeyMap::knownActions().size() - - menuOnlyThreadActions.size()); + - menuOnlySharedIconActions.size()); QVERIFY2(collisions.isEmpty(), qPrintable(QStringLiteral("actions sharing one icon: %1") diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts index 489c62d..a45b4f2 100644 --- a/translations/qtmaildir_it_IT.ts +++ b/translations/qtmaildir_it_IT.ts @@ -280,6 +280,22 @@ Add or remove the deleted tag Aggiunge o rimuove l'etichetta deleted + + Re&ply + Ris&pondi + + + Reply to a&ll + Rispondi a t&utti + + + Reply without &quoting + Rispon&di senza citare + + + Sa&ve message as... + Sal&va messaggio con nome... + Changes made here that a sync has not yet carried to the mail store. An external notmuch run can clear them without this count noticing. Modifiche fatte qui che nessuna sincronizzazione ha ancora trasferito all'archivio di posta. Un'esecuzione esterna di notmuch può azzerarle senza che questo conteggio se ne accorga. @@ -591,6 +607,38 @@ Quit qtmaildir Esce da qtmaildir + + &New message + Nuovo &messaggio + + + Compose a new message + Componi un nuovo messaggio + + + Reply to the displayed message + Rispondi al messaggio visualizzato + + + Reply to the sender and every other recipient + Rispondi al mittente e a ogni altro destinatario + + + Reply with an empty body + Rispondi con un corpo vuoto + + + &Forward + In&oltra + + + Forward the displayed message + Inoltra il messaggio visualizzato + + + Write the raw message to a file + Scrive il messaggio grezzo su un file + &File &File -- cgit v1.2.3