From 2e66bb8b66df016164d44bf72a2d53b8c4a67dde Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sun, 9 Aug 2026 09:45:32 +0200 Subject: feat(ui): icons on every action, and rename Flag to Important Items 56 and 57, done together because both touch the action registry. 56. The complaint was inconsistency, not absence: eight of twenty-four actions had themed icons, so two adjacent entries in one menu disagreed and the toolbar laid out an empty slot for the other sixteen. The themeIcons table now covers them all. The fifteen names added were probed against a live icon theme first rather than taken from the freedesktop spec on faith, and the existing null-icon guard still lets a theme that lacks one fall back to text. The second half of the note asked that buttons honour the desktop's "Icon only" setting. They could not: the hardcoded setToolButtonStyle overrode it whatever the user had chosen. It now reads SH_ToolButtonStyle. Dropping the call entirely was tried and rejected, since a bare QToolBar defaults to ToolButtonIconOnly rather than to the platform hint, which ignores the setting just as thoroughly the other way. This is a visible change: on a desktop set to "Icon only" the toolbar now shows icons without text. 57. "Important" over "Starred", the user's pick; the Message menu already has "Mark &spam", so "Starred" would have needed an accelerator from inside the word. Changed the action text, its status tip, the undo description and the star column's tooltip, which still read "Flagged". The tag stays `flagged`. It is wire format that neomutt, the user's saved queries and ThreadSummary::isFlagged() all read, and following the label through to the tag would rewrite the mail store and desynchronise every other tool over the same Maildir. The action name stays `flag` too, since that is the key users write in [keys]. Four tests. everyActionCarriesAnIcon names every action missing one and guards against passing on an empty list; it reported all sixteen before the change. theImportantActionStillWritesTheFlaggedTag asserts on the tag the model actually received, and mutating it to `important` fails that test plus two pre-existing held-edit tests. Also adds the changelog entry for the cron-sync indicator fix, which the commit that made it omitted. Co-Authored-By: Claude Opus 5 --- tests/test_mainwindow.cpp | 135 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 135 insertions(+) (limited to 'tests/test_mainwindow.cpp') 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 #include +#include #include +#include #include #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 actions = + window.findChildren(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(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(QStringLiteral("main_toolbar")); + QVERIFY(toolBar); + + const auto expected = static_cast( + 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(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(); + QVERIFY(model); + auto *view = window.findChild(); + 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(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. -- cgit v1.2.3