diff options
| -rw-r--r-- | CHANGELOG.md | 7 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 64 | ||||
| -rw-r--r-- | src/htmlbuilder.cpp | 103 | ||||
| -rw-r--r-- | src/htmlbuilder.h | 40 | ||||
| -rw-r--r-- | src/messageview.cpp | 16 | ||||
| -rw-r--r-- | src/messageview.h | 8 | ||||
| -rw-r--r-- | tests/test_htmlbuilder.cpp | 121 |
7 files changed, 340 insertions, 19 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 94ae6c9..ef2b5be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,13 @@ point at which they are stable. ### Fixed +- **The message pane follows the desktop theme.** Its stylesheet hardcoded + light-theme greys and set no background at all, so plain-text mail rendered + as black on white inside a dark window. The colours now derive from the + palette, with the secondary ones blended from it rather than fixed, since a + grey chosen to read as subtle on white is nearly invisible on near-black. A + message that brings its own HTML still brings its own colours: that styling + is deliberately left alone. - **A message whose HTML body carries a `Content-Id` renders**, instead of opening blank with the app reporting no HTML part. A content id makes a part referenceable, not undisplayable, and setting one on the body is legal and 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 2bcdd0a..faf510a 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 @@ -60,6 +60,7 @@ taking that too literally. | 9 | No in-app view of configured shortcuts | discoverability | S | **done** | | 10 | Reaching an account's inbox takes two steps | workflow | S | **postponed** (partly done) | | 11 | Icon, `.desktop` file, SlackBuild | packaging | M | **done** | +| 12 | Message pane is light-theme only | presentation | S | **done** | | 13 | No visual feedback that an action stuck | feedback | S | **done** | | 14 | Tag column unreadable, tags need another home | presentation | M | **done** | | 15 | Attachments are parsed but unreachable from the UI | information | M | **done** | @@ -433,10 +434,12 @@ unchanged. Regression test in `test_config`. **Caution:** do not hardcode colors. The app should follow the desktop palette; a hand-picked grey that looks right on a light theme is unreadable on a dark -one. Use `QPalette` roles. This applies to `HtmlBuilder`'s CSS too, which -currently hardcodes `#bbb`, `#555`, `#000`, `#666`, `#ddd` and no background, -and will look wrong under a dark theme. That is arguably its own item; see -item 12 below if it gets split out. +one. Use `QPalette` roles. This applied to `HtmlBuilder`'s CSS too, which was +split out as item 12 and **done on 2026-08-07**: its colours now derive from +the palette, with the secondary ones blended rather than fixed. The same rule +governs whatever this item adds to the thread list, and item 12's test, which +asserts that no colour appears that the palette did not supply, is the pattern +to copy. ### Refined by the user, 2026-08-04 @@ -623,6 +626,57 @@ Packaging, independent of everything above, and can proceed in parallel. --- +## 12. Message pane is light-theme only + +**Split from item 5**, which recorded the rule against hardcoded colours. +Listed in the deferred table until it was picked up on 2026-08-07. + +**Observed:** the user runs a dark desktop (`color-scheme: prefer-dark`), and +plain-text mail rendered as black on white inside a dark window. + +**Cause (verified in code):** `kStyle` in `src/htmlbuilder.cpp` hardcoded +`#bbb`, `#555`, `#000`, `#666`, `#ddd` and `#4a6f8a`, and set **no background +at all**, so the web view's own default showed through whatever the desktop +was. + +**Approach as built.** A `HtmlBuilder::Palette` struct passed into +`build`/`buildThread`, derived from a `QPalette` by `paletteFrom()`. Passed in +rather than read from `qApp` inside the builder, so the stylesheet can be +tested against a known palette with no running application. + +- **`Base` and `Text`, not `Window` and `WindowText`.** The pane is a content + surface like a text edit, and on many themes `Base` differs from `Window`. +- **The derived colours are blends, not fixed greys.** This is the part that + makes it work both ways round: a `#555` chosen to read as "subtle" on white + is nearly invisible on `#2b2b2b`. `dim` and `border` are mixes of text and + background, so they land at the right contrast whichever way the theme goes. +- The quote colour keeps its hue, since "this is quoted" is carried by being a + different colour rather than a dimmer one, but it is pulled toward the + background so it stays readable rather than glowing on dark. + +Measured on the user's actual theme: background `#2b2b2b`, text `#dedede`, dim +`#969696`, border `#585858`, quote `#6490b0`. + +**Scope, and it is asserted in a test so it cannot drift.** A message that +brings its own HTML brings its own colours, and those are left alone. +Rewriting a sender's styling would break layouts that depend on it, and a +newsletter that sets a white background is entitled to stay white. This item +themes the plain-text render and the chrome around messages, nothing else. So +HTML-heavy mail will still look light, correctly. + +**`MessageView` passes its own widget palette**, not the application's: a style +sheet or a themed parent can give the pane different colours from `qApp`. It +also re-renders on `QEvent::PaletteChange`, because the document's colours are +baked into its stylesheet at build time and it does not restyle itself the way +a widget does; without that, switching the desktop theme would leave the open +thread on the old palette until the next selection. + +**Verification.** The load-bearing test asserts the **negative**: no hex colour +appears in the `<style>` block that the palette did not supply. A test that +only checks the palette's colours are present passes with a leftover literal +still in place, and a single leftover literal is the entire defect. Confirmed +by mutation: putting one hardcoded colour back fails it. + ## 13. No visual feedback that an action stuck **Observed:** selecting a thread and hitting Delete changed nothing on screen. @@ -2497,7 +2551,7 @@ sequence, appended as they arise. | # | Item | Why here | |---|------|----------| -| 12 | `HtmlBuilder` CSS is light-theme only | Split from item 5. Hardcoded greys and no background color; a dark desktop theme will render message bodies badly. Fix likely means passing palette-derived colors into the CSS, which affects `HtmlBuilder`'s tests. | +| 12 | `HtmlBuilder` CSS is light-theme only | **Done 2026-08-07**, and moved to the main status table. Kept listed here so the split from item 5 stays traceable. | ## Adding to this document diff --git a/src/htmlbuilder.cpp b/src/htmlbuilder.cpp index ccb9171..c8f039f 100644 --- a/src/htmlbuilder.cpp +++ b/src/htmlbuilder.cpp @@ -18,25 +18,92 @@ #include "htmlbuilder.h" +#include <QCoreApplication> +#include <QGuiApplication> #include <QRegularExpression> namespace { -const char *kStyle = R"CSS( -body { font-family: sans-serif; font-size: 10pt; margin: 12px; } +/// The stylesheet, with every colour supplied by the caller. +/// +/// %1 background, %2 text, %3 quote, %4 border, %5 dim. +const char *kStyleTemplate = R"CSS( +body { font-family: sans-serif; font-size: 10pt; margin: 12px; + background: %1; color: %2; } pre.plain { white-space: pre-wrap; word-wrap: break-word; font-family: monospace; margin: 0; } -span.quote { color: #4a6f8a; } -.message { border-top: 1px solid #bbb; padding: 10px 0; } +span.quote { color: %3; } +.message { border-top: 1px solid %4; padding: 10px 0; } .message:first-child { border-top: none; } -.msg-header { font-size: 9pt; color: #555; margin-bottom: 8px; } -.msg-header .who { font-weight: bold; color: #000; } -.stub { font-size: 9pt; color: #666; padding: 4px 0; - border-top: 1px solid #ddd; } +.msg-header { font-size: 9pt; color: %5; margin-bottom: 8px; } +.msg-header .who { font-weight: bold; color: %2; } +.stub { font-size: 9pt; color: %5; padding: 4px 0; + border-top: 1px solid %4; } )CSS"; +/// Mixes two colours, `weight` being how much of `a` survives. +/// +/// Blending is what makes the derived colours theme-correct. A fixed grey is +/// only "subtle" against the background it was chosen for: #555 reads as a +/// quiet label on white and nearly vanishes on near-black. +QColor blend(const QColor &a, const QColor &b, qreal weight) +{ + const qreal inverse = 1.0 - weight; + return QColor::fromRgbF(a.redF() * weight + b.redF() * inverse, + a.greenF() * weight + b.greenF() * inverse, + a.blueF() * weight + b.blueF() * inverse); +} + } // namespace +HtmlBuilder::Palette HtmlBuilder::paletteFrom(const QPalette &palette) +{ + // Base and Text, not Window and WindowText: the pane is a content surface + // like a text edit, and on many themes Base differs from Window. + const QColor background = palette.color(QPalette::Base); + const QColor text = palette.color(QPalette::Text); + + Palette result; + result.background = background; + result.text = text; + + // Both derived from the pair, so they land at the right contrast whichever + // way round the theme is. + result.dim = blend(text, background, 0.6); + result.border = blend(text, background, 0.25); + + // The quote colour keeps its hue, since "this is quoted" is carried by the + // colour being different rather than by it being dimmer, but it is pulled + // toward the background so it stays readable on a dark theme instead of + // glowing. + const QColor quoteHue(0x4a, 0x6f, 0x8a); + result.quote = background.lightnessF() < 0.5 + ? blend(quoteHue.lighter(160), background, 0.75) + : blend(quoteHue, background, 0.85); + + return result; +} + +HtmlBuilder::Palette HtmlBuilder::defaultPalette() +{ + if (const QGuiApplication *app = + qobject_cast<QGuiApplication *>(QCoreApplication::instance())) + return paletteFrom(app->palette()); + + // No GUI application: only reachable from a test that did not pass a + // palette. A default-constructed QPalette is light, which matches what + // this code did before it was themed at all. + return paletteFrom(QPalette()); +} + +QString HtmlBuilder::styleSheet(const Palette &palette) +{ + return QString::fromUtf8(kStyleTemplate) + .arg(palette.background.name(), palette.text.name(), + palette.quote.name(), palette.border.name(), + palette.dim.name()); +} + QString HtmlBuilder::renderPlain(const QString &text) { QString out; @@ -61,12 +128,12 @@ QString HtmlBuilder::renderPlain(const QString &text) return out; } -QString HtmlBuilder::document(const QString &bodyHtml) +QString HtmlBuilder::document(const QString &bodyHtml, const Palette &palette) { return QStringLiteral( "<!DOCTYPE html><html><head><meta charset=\"utf-8\">" "<style>%1</style></head><body>%2</body></html>") - .arg(QString::fromUtf8(kStyle), bodyHtml); + .arg(styleSheet(palette), bodyHtml); } QString HtmlBuilder::namespaceCids(const QString &html, const QString &prefix) @@ -218,14 +285,26 @@ QString HtmlBuilder::renderStub(const ParsedMessage &message) QString HtmlBuilder::build(const ParsedMessage &message, Mode mode) { + return build(message, mode, defaultPalette()); +} + +QString HtmlBuilder::build(const ParsedMessage &message, Mode mode, + const Palette &palette) +{ ThreadRenderItem item; item.message = message; item.expanded = true; - return document(renderBody(item, mode)); + return document(renderBody(item, mode), palette); } QString HtmlBuilder::buildThread(const QList<ThreadRenderItem> &items, Mode mode) { + return buildThread(items, mode, defaultPalette()); +} + +QString HtmlBuilder::buildThread(const QList<ThreadRenderItem> &items, Mode mode, + const Palette &palette) +{ QString body; for (int i = 0; i < items.size(); ++i) { @@ -246,5 +325,5 @@ QString HtmlBuilder::buildThread(const QList<ThreadRenderItem> &items, Mode mode renderBody(item, mode)); } - return document(body); + return document(body, palette); } diff --git a/src/htmlbuilder.h b/src/htmlbuilder.h index 6e306dd..75fd1f8 100644 --- a/src/htmlbuilder.h +++ b/src/htmlbuilder.h @@ -18,7 +18,9 @@ #pragma once +#include <QColor> #include <QList> +#include <QPalette> #include "mimeparser.h" @@ -60,11 +62,46 @@ public: ForcePlain, ///< Always render the plain part, escaped. }; + /// The colours the document's own stylesheet uses. + /// + /// Passed in rather than read from qApp inside the builder, so the CSS can + /// be tested against a known palette without a running application, and so + /// nothing here depends on widget state. + /// + /// **Scope.** These style the chrome around messages and the plain-text + /// render. A message that brings its own HTML brings its own colours, and + /// those are deliberately left alone: rewriting a sender's styling would + /// break layouts that depend on it, and a newsletter that sets a white + /// background is entitled to stay white. + struct Palette { + QColor background; ///< The pane itself. + QColor text; ///< Body text. + QColor dim; ///< Headers and stubs: present but secondary. + QColor border; ///< Rules between messages. + QColor quote; ///< Quoted lines in plain text. + }; + + /// Derives the document palette from a widget palette. + /// + /// The dim and border colours are blends rather than fixed greys, which is + /// what makes this work on a dark theme: a hardcoded #555 that reads as + /// "subtle" on white is nearly invisible on near-black. + static Palette paletteFrom(const QPalette &palette); + + /// The palette used when a caller supplies none: the running application's. + /// Falls back to a light default with no QApplication, which only happens + /// in a test that did not ask for a palette. + static Palette defaultPalette(); + /// Single message, used for the error card and for tests. static QString build(const ParsedMessage &message, Mode mode); + static QString build(const ParsedMessage &message, Mode mode, + const Palette &palette); /// The whole thread, oldest first. static QString buildThread(const QList<ThreadRenderItem> &items, Mode mode); + static QString buildThread(const QList<ThreadRenderItem> &items, Mode mode, + const Palette &palette); /// Rewrites cid: URLs in an HTML body to their namespaced form. static QString namespaceCids(const QString &html, const QString &prefix); @@ -73,5 +110,6 @@ private: static QString renderPlain(const QString &text); static QString renderBody(const ThreadRenderItem &item, Mode mode); static QString renderStub(const ParsedMessage &message); - static QString document(const QString &bodyHtml); + static QString document(const QString &bodyHtml, const Palette &palette); + static QString styleSheet(const Palette &palette); }; diff --git a/src/messageview.cpp b/src/messageview.cpp index d7167bf..10b21a9 100644 --- a/src/messageview.cpp +++ b/src/messageview.cpp @@ -378,12 +378,26 @@ void MessageView::showDetailsDialog() dialog.exec(); } +void MessageView::changeEvent(QEvent *event) +{ + QWidget::changeEvent(event); + + // Only when there is something to re-render: rendering an empty item list + // would replace a deliberately blank pane with an empty document. + if (event->type() == QEvent::PaletteChange && !m_items.isEmpty()) + render(); +} + void MessageView::render() { const HtmlBuilder::Mode mode = m_preferHtml ? HtmlBuilder::PreferHtml : HtmlBuilder::ForcePlain; - setDocument(HtmlBuilder::buildThread(m_items, mode)); + // This widget's palette, not the application's: a style sheet or a themed + // parent can give the pane different colours from qApp, and the document + // has to match the frame it sits in rather than the app default. + setDocument(HtmlBuilder::buildThread(m_items, mode, + HtmlBuilder::paletteFrom(palette()))); rebuildAttachmentBar(); // Blocking is discovered during load, so check shortly afterwards. diff --git a/src/messageview.h b/src/messageview.h index 1be17f8..c55f5c5 100644 --- a/src/messageview.h +++ b/src/messageview.h @@ -105,6 +105,14 @@ protected: /// rather than one widget. bool eventFilter(QObject *watched, QEvent *event) override; + /// Re-renders when the desktop theme changes. + /// + /// The document's colours are baked into its stylesheet at build time, so + /// unlike a widget it does not restyle itself: switching the desktop from + /// light to dark would otherwise leave the open thread on the old palette + /// until the next selection. + void changeEvent(QEvent *event) override; + private: void render(); void updateHeader(); diff --git a/tests/test_htmlbuilder.cpp b/tests/test_htmlbuilder.cpp index 04bd294..7fa45a2 100644 --- a/tests/test_htmlbuilder.cpp +++ b/tests/test_htmlbuilder.cpp @@ -16,6 +16,8 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ +#include <QPalette> +#include <QRegularExpression> #include <QSet> #include <QtTest> #include "cidschemehandler.h" @@ -42,6 +44,12 @@ private slots: void namespacesMultipleCidRefsOnOneLine(); void namespacesWhitespaceAroundEquals(); void namespacedKeyRejectsPrefixContainingSeparator(); + + // Theming. + void aDarkPaletteProducesADarkDocument(); + void everyColourComesFromThePalette(); + void theBodyAlwaysGetsABackground(); + void aSendersOwnHtmlIsNotRecoloured(); }; void TestHtmlBuilder::escapesPlainText() @@ -276,5 +284,118 @@ void TestHtmlBuilder::namespacedKeyRejectsPrefixContainingSeparator() QVERIFY(html.contains(QStringLiteral("cid:%1").arg(expectedKey))); } +/// A palette with unmistakable colours, so a hardcoded value cannot pass by +/// coincidentally resembling a real theme. +static QPalette makeTestPalette(const QColor &window, const QColor &text) +{ + QPalette palette; + palette.setColor(QPalette::Base, window); + palette.setColor(QPalette::Window, window); + palette.setColor(QPalette::Text, text); + palette.setColor(QPalette::WindowText, text); + return palette; +} + +void TestHtmlBuilder::aDarkPaletteProducesADarkDocument() +{ + // The whole point of the item: the CSS was hardcoded light, so a user on a + // dark desktop read plain-text mail as black on white inside a dark window. + ParsedMessage msg; + msg.plainBody = QStringLiteral("hello"); + + const HtmlBuilder::Palette dark = HtmlBuilder::paletteFrom( + makeTestPalette(QColor(0x12, 0x34, 0x56), QColor(0xab, 0xcd, 0xef))); + + const QString html = + HtmlBuilder::build(msg, HtmlBuilder::ForcePlain, dark); + + QVERIFY2(html.contains(QStringLiteral("#123456")), qPrintable(html)); + QVERIFY2(html.contains(QStringLiteral("#abcdef")), qPrintable(html)); +} + +void TestHtmlBuilder::everyColourComesFromThePalette() +{ + // A single leftover literal is the whole defect, and it survives a test + // that only checks the palette colours are present. Assert the negative: + // no hex colour appears that the palette did not put there. + ParsedMessage msg; + msg.plainBody = QStringLiteral("> quoted\nplain"); + + const HtmlBuilder::Palette dark = HtmlBuilder::paletteFrom( + makeTestPalette(QColor(0x12, 0x34, 0x56), QColor(0xab, 0xcd, 0xef))); + const QString html = + HtmlBuilder::build(msg, HtmlBuilder::ForcePlain, dark); + + // Only the <style> block: a sender's own HTML is not ours to police, and + // the body of this message carries no colours anyway. + const qsizetype start = html.indexOf(QStringLiteral("<style>")); + const qsizetype end = html.indexOf(QStringLiteral("</style>")); + QVERIFY(start >= 0 && end > start); + const QString style = html.mid(start, end - start); + + static const QRegularExpression hex(QStringLiteral("#[0-9a-fA-F]{3,8}\\b")); + auto it = hex.globalMatch(style); + QSet<QString> found; + while (it.hasNext()) + found.insert(it.next().captured(0).toLower()); + + // Every colour in the stylesheet must be one the palette supplied. The + // derived ones (a border, a dimmed label) are blends of those, so they are + // listed by the builder rather than being free-floating literals. + const QSet<QString> allowed = { + dark.background.name().toLower(), dark.text.name().toLower(), + dark.dim.name().toLower(), dark.border.name().toLower(), + dark.quote.name().toLower(), + }; + + for (const QString &colour : found) { + QVERIFY2(allowed.contains(colour), + qPrintable(QStringLiteral("stylesheet carries '%1', which the " + "palette did not supply: a " + "hardcoded colour survives") + .arg(colour))); + } +} + +void TestHtmlBuilder::theBodyAlwaysGetsABackground() +{ + // The original CSS set no background at all, which is why the pane was + // white: the web view's default showed through regardless of the desktop. + ParsedMessage msg; + msg.plainBody = QStringLiteral("hello"); + + const HtmlBuilder::Palette dark = HtmlBuilder::paletteFrom( + makeTestPalette(QColor(0x12, 0x34, 0x56), QColor(0xab, 0xcd, 0xef))); + const QString html = + HtmlBuilder::build(msg, HtmlBuilder::ForcePlain, dark); + + const qsizetype start = html.indexOf(QStringLiteral("<style>")); + const qsizetype end = html.indexOf(QStringLiteral("</style>")); + const QString style = html.mid(start, end - start); + + static const QRegularExpression bodyRule( + QStringLiteral("body\\s*\\{[^}]*background[^}]*\\}")); + QVERIFY2(bodyRule.match(style).hasMatch(), + qPrintable(QStringLiteral("body has no background rule:\n") + style)); +} + +void TestHtmlBuilder::aSendersOwnHtmlIsNotRecoloured() +{ + // Scope, asserted so it does not drift: an HTML message brings its own + // styling and this change must not start rewriting it. A newsletter that + // sets its own white background stays white, and that is correct. + ParsedMessage msg; + msg.htmlBody = QStringLiteral( + "<div style=\"background:#ffffff;color:#000000\">hi</div>"); + + const HtmlBuilder::Palette dark = HtmlBuilder::paletteFrom( + makeTestPalette(QColor(0x12, 0x34, 0x56), QColor(0xab, 0xcd, 0xef))); + const QString html = + HtmlBuilder::build(msg, HtmlBuilder::PreferHtml, dark); + + QVERIFY(html.contains(QStringLiteral("background:#ffffff"))); + QVERIFY(html.contains(QStringLiteral("color:#000000"))); +} + QTEST_MAIN(TestHtmlBuilder) #include "test_htmlbuilder.moc" |