diff options
| -rw-r--r-- | src/keymap.cpp | 33 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 87 | ||||
| -rw-r--r-- | src/mainwindow.h | 21 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 195 | ||||
| -rw-r--r-- | 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<QPair<QString, QString>> 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<void()> &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<QPair<QString, QList<QAction *>>> scopes; + scopes.append({ QStringLiteral("the menu bar"), bar->actions() }); + + QList<QMenu *> 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<QString, QStringList> 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<QString, QString> frozen; + for (const QString &known : knownPreExistingCollisions) + frozen.insert(scopeAndKey(known), known); + + QStringList unexpected; + QSet<QString> 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<QToolBar *>(); QVERIFY(toolBar); - for (const QString &name : menuOnlyThreadActions) { + for (const QString &name : menuOnlySharedIconActions) { auto *action = window.findChild<QAction *>(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 @@ -281,6 +281,22 @@ <translation>Aggiunge o rimuove l'etichetta deleted</translation> </message> <message> + <source>Re&ply</source> + <translation>Ris&pondi</translation> + </message> + <message> + <source>Reply to a&ll</source> + <translation>Rispondi a t&utti</translation> + </message> + <message> + <source>Reply without &quoting</source> + <translation>Rispon&di senza citare</translation> + </message> + <message> + <source>Sa&ve message as...</source> + <translation>Sal&va messaggio con nome...</translation> + </message> + <message> <source>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.</source> <translation>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.</translation> </message> @@ -592,6 +608,38 @@ <translation>Esce da qtmaildir</translation> </message> <message> + <source>&New message</source> + <translation>Nuovo &messaggio</translation> + </message> + <message> + <source>Compose a new message</source> + <translation>Componi un nuovo messaggio</translation> + </message> + <message> + <source>Reply to the displayed message</source> + <translation>Rispondi al messaggio visualizzato</translation> + </message> + <message> + <source>Reply to the sender and every other recipient</source> + <translation>Rispondi al mittente e a ogni altro destinatario</translation> + </message> + <message> + <source>Reply with an empty body</source> + <translation>Rispondi con un corpo vuoto</translation> + </message> + <message> + <source>&Forward</source> + <translation>In&oltra</translation> + </message> + <message> + <source>Forward the displayed message</source> + <translation>Inoltra il messaggio visualizzato</translation> + </message> + <message> + <source>Write the raw message to a file</source> + <translation>Scrive il messaggio grezzo su un file</translation> + </message> + <message> <source>&File</source> <translation>&File</translation> </message> |