diff options
| -rw-r--r-- | CHANGELOG.md | 26 | ||||
| -rw-r--r-- | README.md | 4 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 47 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 50 | ||||
| -rw-r--r-- | src/threadlistmodel.cpp | 5 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 135 |
6 files changed, 258 insertions, 9 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 851e318..99de25c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,32 @@ point at which they are stable. - **`assets/mailsync.sh` takes channel names as arguments**, syncing all channels when given none. A replacement sync script that ignores its arguments still works, it just always syncs everything. +- **Every action now carries an icon**, where before only eight of twenty-four + did and adjacent menu entries disagreed with each other. Icons come from the + desktop's icon theme; one the theme does not provide falls back to text alone. +- **An optional `[sync] log` key**, naming the sync script's log file. It + defaults to where `assets/mailsync.sh` writes, and only needs setting if you + changed the script's `LOGFILE`. + +### Changed + +- **"Flag" is now "Important"**, on the menu entry, the undo history and the + star column's tooltip. `Ctrl+I` is unchanged, and so is the `flagged` tag + itself: neomutt, your saved queries and anything else reading the same Maildir + keep working. The `flag` action name in `[keys]` is also unchanged, so + existing bindings are untouched. +- **The toolbar follows your desktop's toolbar button setting** instead of + always showing text beside icons. If your desktop is set to "Icon only", the + toolbar is now icons only; it previously ignored that. + +### Fixed + +- **A sync run from cron now clears the unsynced-changes indicator.** Edits made + in the application reach the mail store through any sync, but only a sync + started from the window cleared the count, so the indicator kept reporting + work that had already gone out and the quit prompt offered to sync for it. + A failed sync, or one whose outcome cannot be read, still leaves the count + standing. ## [0.11.0] - 2026-08-07 @@ -258,7 +258,7 @@ and costs no extra query. ## Tagging -Archive, delete, spam, flag and toggle-unread write fixed tags. For anything +Archive, delete, spam, mark-important and toggle-unread write fixed tags. For anything else, **Ctrl+T** opens a dialog over the selected threads: type tags to add or remove, separated by commas, or clear a checkbox to drop a tag already present. @@ -412,7 +412,7 @@ Defaults, all rebindable through `[keys]`: | `Ctrl+Shift+S` | `spam` | Add `spam`, remove `inbox` | | `Ctrl+U` | `toggle_unread` | Toggle `unread` | | `Ctrl+Shift+U` | `mark_all_read` | Remove `unread` from every thread in the view | -| `Ctrl+I` | `flag` | Add `flagged` | +| `Ctrl+I` | `flag` | Mark important (adds `flagged`) | | `Ctrl+L` | `focus_query` | Focus and select the query bar | | `Ctrl+Space` | `complete_query` | Focus the query bar and offer completions | | `Ctrl+H` | `toggle_html` | Switch the thread between HTML and plain text | 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 33d6a61..52cc023 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 @@ -113,8 +113,8 @@ taking that too literally. | 53 | Message rows still read as a table, not as a conversation | presentation | ? | open, unspecified | | 54 | A cron sync carries the edits but the count still says pending | correctness | S | **done** | | 55 | In a narrow window the message pane is invisible | presentation | XS | open | -| 56 | No action carries an icon, so the toolbar reserves space for nothing | presentation | S | open | -| 57 | "Flag" would read better as "Important" or "Starred" | presentation | XS | open | +| 56 | No action carries an icon, so the toolbar reserves space for nothing | presentation | S | **done** | +| 57 | "Flag" would read better as "Important" or "Starred" | presentation | XS | **done** | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -3457,6 +3457,29 @@ on a machine whose icon theme resolves the names, so it is an assertion about this desktop as much as about the code, and it says nothing about whether the icon chosen is the *right* one. +**Built 2026-08-09, with item 57.** The `themeIcons` table now covers all 24 +registered actions; the fifteen names added were probed against a live theme +before being written, not taken from the freedesktop spec on faith. The null +guard is kept, so a theme missing a name still degrades to text. + +`setToolButtonStyle()` now reads `QStyle::SH_ToolButtonStyle` instead of +asserting `TextBesideIcon`. Removing the call entirely was considered and +rejected: verified empirically that a bare `QToolBar` defaults to +`Qt::ToolButtonIconOnly` rather than to the platform hint, which would have +ignored the user's setting just as thoroughly in the other direction. + +**This changes the toolbar's appearance on the developer's desktop.** Its hint +reads `0` (`ToolButtonIconOnly`), so the toolbar shows icons without text where +it previously showed both. That is the setting being honoured, which is what the +note asked for, but it is a visible change rather than a silent one. + +Two tests: `everyActionCarriesAnIcon`, which iterates +`KeyMap::knownActions()` and names every action missing one (it reported all +sixteen before the change), and +`theToolbarDoesNotOverrideTheDesktopButtonStyle`. The first carries a guard +asserting the window really registered its actions, so it cannot pass by +iterating an empty list. + ## 57. "Flag" would read better as "Important" or "Starred" **Observed (user, 2026-08-09):** "Flagged to be renamed as 'Important' or @@ -3499,6 +3522,26 @@ whatever the icon depicts (a star suggests "Starred"). - `src/tagdialog.cpp:211` mentions `flagged` in a comment about token completion. That is the tag, not a label, and must not be touched. +**Built 2026-08-09, with item 56.** The user chose "Important". Changed: the +action text to `tr("&Important")`, its status tip, the undo description to +`tr("Mark important")`, and the flag column's tooltip +(`src/threadlistmodel.cpp:192`), which still read "Flagged". The README's +keybinding row and tagging prose followed. + +Unchanged, deliberately: the action **name** `flag`, which is the key a user +writes in `[keys]` and whose rename would silently break every existing +binding; the tag `flagged`; `ThreadSummary::isFlagged()`; the `[tagcolors]` +entry; and the `Flagged = tag:flagged` saved query in the README's sample +config, which is a user's own query name rather than one of our labels. + +`Ctrl+I` was already the binding, which happens to fit the new word. + +`theImportantActionStillWritesTheFlaggedTag` is the test that matters: it +triggers the action and asserts on the tag the model actually received, with a +guard proving the thread did not already carry it. Mutating the tag to +`important` fails it, and also fails two pre-existing held-edit tests, which is +independent confirmation that `flagged` is load-bearing across the suite. + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 96e2a79..865c691 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -681,9 +681,17 @@ void MainWindow::registerActions() tagSelected({ QStringLiteral("spam") }, { QStringLiteral("inbox") }, tr("Mark spam")); }); - addAction(QStringLiteral("flag"), tr("&Flag"), - tr("Add the flagged tag"), [this]() { - tagSelected({ QStringLiteral("flagged") }, {}, tr("Flag")); + // Item 57. The LABEL is "Important"; the action name and the tag are both + // still `flag`/`flagged`, deliberately. The name is what a user writes in + // the config's [keys] section, and `flagged` is a notmuch tag that neomutt, + // the user's saved queries and ThreadSummary::isFlagged() all read. Only + // the wording the user sees changes. + // + // &I rather than &S: the Message menu already has "Mark &spam", so + // "Starred" would have needed an accelerator from inside the word. + addAction(QStringLiteral("flag"), tr("&Important"), + tr("Mark the selected threads as important"), [this]() { + tagSelected({ QStringLiteral("flagged") }, {}, tr("Mark important")); }); addAction(QStringLiteral("toggle_unread"), tr("Toggle &unread"), tr("Toggle the unread tag"), [this]() { @@ -899,6 +907,14 @@ void MainWindow::buildMenus() // Standard names from the icon theme, so the buttons match the rest of the // desktop rather than shipping bespoke art. A theme that lacks one leaves // that action with text alone, which still works. + // Item 56: every registered action, not a subset. Eight of these carried an + // icon and sixteen did not, which reads worse than none having one: two + // adjacent entries in the same menu disagreed, and the toolbar's + // TextBesideIcon style laid out an empty slot for each of the sixteen. + // + // Names are freedesktop ones, and were probed against a real icon theme + // rather than taken from the spec on faith. A name the running theme lacks + // still degrades to text through the null check below. const QHash<QString, QString> themeIcons = { { QStringLiteral("sync"), QStringLiteral("mail-receive") }, { QStringLiteral("archive"), QStringLiteral("mail-mark-read") }, @@ -908,6 +924,23 @@ void MainWindow::buildMenus() { QStringLiteral("flag"), QStringLiteral("mail-mark-important") }, { QStringLiteral("quit"), QStringLiteral("application-exit") }, { QStringLiteral("focus_query"), QStringLiteral("edit-find") }, + + { QStringLiteral("next_thread"), QStringLiteral("go-down") }, + { QStringLiteral("prev_thread"), QStringLiteral("go-up") }, + { QStringLiteral("open_thread"), QStringLiteral("document-open") }, + { QStringLiteral("toggle_unread"), QStringLiteral("mail-mark-unread") }, + { QStringLiteral("mark_all_read"), QStringLiteral("mail-mark-read") }, + { QStringLiteral("edit_tags"), QStringLiteral("tag") }, + { QStringLiteral("complete_query"), QStringLiteral("edit-find-replace") }, + { QStringLiteral("select_all"), QStringLiteral("edit-select-all") }, + { QStringLiteral("clear_pane"), QStringLiteral("edit-clear") }, + { QStringLiteral("clear_selection"), QStringLiteral("edit-clear-all") }, + { QStringLiteral("toggle_html"), QStringLiteral("text-html") }, + { QStringLiteral("load_remote"), QStringLiteral("image-loading") }, + { QStringLiteral("message_details"), QStringLiteral("dialog-information") }, + { QStringLiteral("zoom_in"), QStringLiteral("zoom-in") }, + { QStringLiteral("zoom_out"), QStringLiteral("zoom-out") }, + { QStringLiteral("zoom_reset"), QStringLiteral("zoom-original") }, }; for (auto it = themeIcons.cbegin(); it != themeIcons.cend(); ++it) { QAction *action = m_actions.value(it.key()); @@ -945,7 +978,16 @@ void MainWindow::buildMenus() // unreadable as no toolbar. auto *toolBar = addToolBar(tr("Main")); toolBar->setObjectName(QStringLiteral("main_toolbar")); - toolBar->setToolButtonStyle(Qt::ToolButtonTextBesideIcon); + // Item 56, second half: the user asked that buttons honour the desktop's + // "Icon only" setting. They cannot while this asserts a style of its own. + // Qt exposes the desktop's preference as SH_ToolButtonStyle, and a + // hardcoded setToolButtonStyle() overrides it whatever the user chose. + // + // Read rather than dropped entirely: with no call at all a QToolBar + // defaults to Qt::ToolButtonIconOnly rather than to the platform's hint, + // which would ignore the setting just as thoroughly in the other direction. + toolBar->setToolButtonStyle(static_cast<Qt::ToolButtonStyle>( + style()->styleHint(QStyle::SH_ToolButtonStyle, nullptr, toolBar))); 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/threadlistmodel.cpp b/src/threadlistmodel.cpp index c675488..4bafca8 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -189,8 +189,11 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const if (role == Qt::ToolTipRole && index.column() == AttachmentColumn) return thread.hasAttachment() ? tr("Has an attachment") : QVariant(); + // "Important", matching the action's own wording (item 57). The underlying + // tag is still `flagged` and isFlagged() still tests for it; only what the + // user reads changed. if (role == Qt::ToolTipRole && index.column() == FlagColumn) - return thread.isFlagged() ? tr("Flagged") : QVariant(); + return thread.isFlagged() ? tr("Important") : QVariant(); // Both marker columns: a glyph reads as a marker only when it sits in the // middle of its column rather than against the text beside it. diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index e40fd3f..4a911c9 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -34,7 +34,9 @@ #include <QStandardPaths> #include <QTemporaryDir> +#include <QStyle> #include <QTableView> +#include <QToolBar> #include <QTimer> #include "config.h" @@ -113,6 +115,11 @@ private slots: void anUnknownExternalStateClearsNothing(); void aSuccessfulCronSyncDrainsTheEditedAccounts(); void aCronSyncDoesNotClearAnEditMadeWhileItRan(); + + void everyActionCarriesAnIcon(); + void theToolbarDoesNotOverrideTheDesktopButtonStyle(); + void theImportantActionIsLabelledImportant(); + void theImportantActionStillWritesTheFlaggedTag(); }; void TestMainWindow::everyKnownActionIsRegistered() @@ -2378,6 +2385,134 @@ void TestMainWindow::aCronSyncDoesNotClearAnEditMadeWhileItRan() "an edit made after the sync ended was swallowed by it"); } +// Items 56 and 57. + +void TestMainWindow::everyActionCarriesAnIcon() +{ + // Item 56. The complaint was inconsistency, not absence: eight actions had + // themed icons and the other sixteen had none, so adjacent entries in one + // menu disagreed, and the toolbar's TextBesideIcon style laid out an empty + // slot for each of the sixteen. + // + // What this test can and cannot prove is worth stating, because it is + // weaker than it looks. QIcon::fromTheme() resolves against the icon theme + // of the machine running the test, so a PASS says "this desktop has art for + // every name assigned", not "every name is correct" and not "the icon suits + // the action". A machine with a sparse theme fails this through no fault of + // the code. It is still worth having: it catches the actual regression, + // which is an action registered with no name assigned at all. + const Config config; + MainWindow window(config); + + // The guard. Without it a MainWindow that registered nothing would pass an + // empty loop, which is the classic way a "for each" assertion goes vacuous. + const QList<QAction *> actions = + window.findChildren<QAction *>(QString(), Qt::FindDirectChildrenOnly); + QVERIFY2(actions.size() >= KeyMap::knownActions().size(), + qPrintable(QStringLiteral("expected at least %1 actions, found %2") + .arg(KeyMap::knownActions().size()) + .arg(actions.size()))); + + QStringList missing; + for (const QString &name : KeyMap::knownActions()) { + auto *action = window.findChild<QAction *>(name); + QVERIFY2(action, qPrintable(QStringLiteral("no action named %1").arg(name))); + if (action->icon().isNull()) + missing.append(name); + } + + QVERIFY2(missing.isEmpty(), + qPrintable(QStringLiteral("%1 action(s) have no icon: %2") + .arg(missing.size()) + .arg(missing.join(QStringLiteral(", "))))); +} + +void TestMainWindow::theToolbarDoesNotOverrideTheDesktopButtonStyle() +{ + // The second half of the user's note: "Buttons should honor the 'Icon only' + // option". They cannot while the toolbar asserts its own style. Qt takes + // the desktop's preference from the platform theme and exposes it as + // SH_ToolButtonStyle; a hardcoded setToolButtonStyle() overrides it, so the + // user's setting has no effect whatever it is set to. + const Config config; + MainWindow window(config); + + auto *toolBar = window.findChild<QToolBar *>(QStringLiteral("main_toolbar")); + QVERIFY(toolBar); + + const auto expected = static_cast<Qt::ToolButtonStyle>( + window.style()->styleHint(QStyle::SH_ToolButtonStyle, nullptr, toolBar)); + + QCOMPARE(toolBar->toolButtonStyle(), expected); +} + +void TestMainWindow::theImportantActionIsLabelledImportant() +{ + // Item 57. The user picked "Important" over "Starred": the Message menu + // already has `Mark &spam`, so "Starred" would have had to take an + // accelerator from inside the word, while "Important" takes a free &I. + const Config config; + MainWindow window(config); + + // The action NAME is unchanged on purpose. It is the key a user writes in + // the config's [keys] section, so renaming it would silently break every + // existing binding for a change that is only about wording. + auto *action = window.findChild<QAction *>(QStringLiteral("flag")); + QVERIFY(action); + + QVERIFY2(action->text().contains(QStringLiteral("Important")), + qPrintable(QStringLiteral("the action still reads '%1'") + .arg(action->text()))); + QVERIFY2(!action->text().contains(QStringLiteral("Flag")), + qPrintable(QStringLiteral("the action still reads '%1'") + .arg(action->text()))); + + // The accelerator the item chose, and the reason "Starred" was rejected. + QCOMPARE(action->text(), QStringLiteral("&Important")); +} + +void TestMainWindow::theImportantActionStillWritesTheFlaggedTag() +{ + // The rename is a LABEL change and must not reach the mail store. `flagged` + // is a notmuch tag: neomutt reads it, the user's saved queries match on it, + // ThreadSummary::isFlagged() tests for it and TagColors colours it. A + // rename that followed the label through to the tag would rewrite the store + // and desynchronise every other tool that reads the same Maildir. + // + // Asserted on the ids and tags actually sent to the worker, which is the + // only place the distinction is observable. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTableView *>(); + QVERIFY(view); + + model->appendBatch({ makeThread(QStringLiteral("t1"), + { QStringLiteral("inbox") }) }); + + // The guard: the thread must NOT already carry the tag, or the assertion + // below would pass against an action that did nothing at all. + QVERIFY(!model->threadAt(0).isFlagged()); + + view->selectRow(0); + + auto *action = window.findChild<QAction *>(QStringLiteral("flag")); + QVERIFY(action); + action->trigger(); + + // sendThreadTagChange() applies the change to the model optimistically, so + // the tag the action really wrote is observable here without a worker. + QVERIFY2(model->threadAt(0).isFlagged(), + "the renamed action no longer writes the `flagged` tag"); + QVERIFY2(model->threadAt(0).tags.contains(QStringLiteral("flagged")), + "the tag written was not `flagged`"); + QVERIFY2(!model->threadAt(0).tags.contains(QStringLiteral("important")), + "the rename reached the mail store: an `important` tag was " + "written, which no other tool reading this Maildir knows"); +} + // Constructing a MainWindow needs a QApplication and a platform plugin. The // test has no display under ctest, so it runs offscreen unless the caller // asked for something else. |