diff options
| author | Danilo M. <danix@danix.xyz> | 2026-08-09 10:08:45 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-08-09 10:08:45 +0200 |
| commit | 190b6276f09495269d3dd869cf2d9d6bc6883681 (patch) | |
| tree | 1775baf57ee7bc0e38e7de0a98ebc16855707063 | |
| parent | 7ed455d04ee1eb689d533333968cb8e97166aeba (diff) | |
| download | qtmaildir-190b6276f09495269d3dd869cf2d9d6bc6883681.tar.gz qtmaildir-190b6276f09495269d3dd869cf2d9d6bc6883681.zip | |
fix(ui): give Archive its own icon
Item 59, reported by the user against 0.12.0. Archive and Mark all read
both used `mail-mark-read`, so with the toolbar following a desktop set
to icon-only the two buttons were indistinguishable, despite doing
different things: archive removes `inbox` from the selection, mark all
read removes `unread` from the whole view.
Introduced by item 56 in the same session. `archive` had that name from
before, when only eight actions carried icons, and item 56 assigned it
to `mark_all_read` as well without checking the table for duplicates.
`mail-archive` is also the more accurate name, since `mail-mark-read`
describes read state rather than archiving.
The test asserts the class rather than the reported pair: a hand-written
table of twenty-four names has more plausible duplicates in it, so
noTwoActionsShareAnIcon compares every action against every other. It
compares cacheKey() rather than the theme name, because two different
names resolving to the same art are equally ambiguous on screen, and it
guards on every action having an icon first, since on a theme that
resolves nothing the comparison loop would never run and pass vacuously.
Mutation-checked with a different collision.
Item 56's own probe is what let this through: it verified every name
resolves to non-null art, which is true of two names resolving to the
same art. Resolving and being distinguishable are separate properties.
The replacement was picked by rendering both at 24px and comparing the
images.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| -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. |
