diff options
| -rw-r--r-- | CHANGELOG.md | 8 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 46 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 5 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 53 |
4 files changed, 111 insertions, 1 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index da1dcba..7f42995 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,14 @@ point at which they are stable. ## [Unreleased] +### Fixed + +- **Archive and Mark all read no longer share an icon.** Both used the same one + in 0.12.0, which was harmless while the toolbar showed text beside every icon + and ambiguous once it follows a desktop set to icon-only. Archive now uses a + distinct icon, and a test compares every action's icon against every other so + the next duplicate fails the build rather than shipping. + ## [0.12.0] - 2026-08-09 Syncing gets narrower and more honest: a sync fetches only the accounts you 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 ba0ade5..f559bf1 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 @@ -116,6 +116,7 @@ taking that too literally. | 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** | | 58 | `message_zoom` documents a 0.5 to 3.0 range and enforces none of it | correctness | XS | open | +| 59 | Archive and Mark all read shipped with the same icon | presentation | XS | **done** | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -3576,6 +3577,51 @@ numbers. `messageZoomDefaultsAndValidates` exists and passes today, so it is asserting only on the parse and not on the range. +## 59. Archive and Mark all read shipped with the same icon + +**Observed (user, 2026-08-09), against 0.12.0:** "Archive" and "Mark all read" +share the same icon, and in an icon-only setup they are not distinguishable. + +**Cause.** Introduced by item 56, in this session. The `themeIcons` table gave +`archive` the name `mail-mark-read` (it predates item 56, from when only eight +actions had icons and `mark_all_read` had none), and item 56 then assigned +`mail-mark-read` to `mark_all_read` as well without checking the table for +duplicates. Twenty-four entries were added or reviewed by hand and this one +overlap was not noticed. + +It only became visible because of the other half of item 56. While the toolbar +forced `TextBesideIcon` the label disambiguated the two buttons; once it follows +a desktop set to icon-only, the icon **is** the whole control, and two buttons +whose consequences differ (`archive` removes `inbox` from the selection, +`mark_all_read` removes `unread` from the entire view) looked identical. + +**Fix.** `archive` now uses `mail-archive`, which is also the more accurate +name: `mail-mark-read` describes read state, which is what `mark_all_read` +does, not what archiving does. + +**The verification is the point of this entry.** A test for the reported pair +would have been worthless, since the defect is the class and not the instance: a +hand-written table of twenty-four names has more plausible duplicates in it. +`noTwoActionsShareAnIcon` compares every action against every other and names +any pair that matches. Two details matter: + +- It compares `QIcon::cacheKey()`, not the theme name, which the window does not + keep. Two *different* names that resolve to the same art on some theme are + equally ambiguous on screen, and that is what the user actually sees. +- It carries a guard requiring every action to have an icon before comparing. + On a theme that resolves nothing, every icon is null, the loop body never + runs, and the assertion would pass having compared nothing. + +Mutation-checked by introducing a *different* collision (`zoom_out` pointed at +`zoom-in`); the test named that pair rather than the one it was written for. + +**Also worth recording: the icon-name probe endorsed the wrong thing.** Item 56 +verified that every name resolves to non-null art, and that check passes +happily for two names resolving to the *same* art. Resolving and being +distinguishable are separate properties, and only the first was tested. The +candidate replacement was therefore checked by rendering both icons at 24px and +comparing the images, not by asking whether the name existed. + ## 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 a176d71..cb02652 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -917,7 +917,10 @@ void MainWindow::buildMenus() // 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") }, + // NOT mail-mark-read, which mark_all_read below uses. The two shared it + // in 0.12.0, and with the toolbar icon-only the icon is the whole + // control: two buttons with different consequences looked identical. + { QStringLiteral("archive"), QStringLiteral("mail-archive") }, { QStringLiteral("delete"), QStringLiteral("edit-delete") }, { QStringLiteral("undo"), QStringLiteral("edit-undo") }, { QStringLiteral("spam"), QStringLiteral("mail-mark-junk") }, diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 84a3a7d..fc7409c 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -121,6 +121,7 @@ private slots: void theImportantActionIsLabelledImportant(); void theImportantActionStillWritesTheFlaggedTag(); void theToolbarUsesTheConfiguredIconSize(); + void noTwoActionsShareAnIcon(); }; void TestMainWindow::everyKnownActionIsRegistered() @@ -2542,6 +2543,58 @@ void TestMainWindow::theToolbarUsesTheConfiguredIconSize() QCOMPARE(toolBar->iconSize(), QSize(40, 40)); } +void TestMainWindow::noTwoActionsShareAnIcon() +{ + // Reported by the user against the icons shipped in 0.12.0: Archive and + // Mark all read both used `mail-mark-read`. With the toolbar following a + // desktop set to icon-only, the icon is the entire control, so two buttons + // with different consequences were indistinguishable. + // + // Asserted over every action rather than that one pair, because the defect + // is the class and not the instance: the icon table is hand-written and + // twenty-four entries long, so the next duplicate is a plausible typo. + // + // Compared by cacheKey() rather than by the theme NAME, which this window + // does not keep. Two distinct names that resolve to the same art on a given + // theme are just as ambiguous on screen, and that is what the user sees. + const Config config; + MainWindow window(config); + + QHash<qint64, QString> owners; + QStringList collisions; + int withIcons = 0; + + 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()) + continue; + + ++withIcons; + const qint64 key = action->icon().cacheKey(); + const auto existing = owners.constFind(key); + if (existing != owners.constEnd()) { + collisions.append(QStringLiteral("%1 and %2") + .arg(existing.value(), name)); + } else { + owners.insert(key, name); + } + } + + // The guard. On a theme that resolves nothing every icon is null, the loop + // body never runs, and the assertion below would pass having compared + // nothing at all. + QVERIFY2(withIcons >= KeyMap::knownActions().size(), + qPrintable(QStringLiteral("only %1 of %2 actions had an icon to " + "compare") + .arg(withIcons) + .arg(KeyMap::knownActions().size()))); + + QVERIFY2(collisions.isEmpty(), + qPrintable(QStringLiteral("actions sharing one icon: %1") + .arg(collisions.join(QStringLiteral("; "))))); +} + // 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. |