diff options
| author | Danilo M. <danix@danix.xyz> | 2026-08-04 10:49:02 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-08-04 12:54:43 +0200 |
| commit | e19e2318fc637d50b86f663b21bd97494057232d (patch) | |
| tree | 28cd5e183511959abdc4a49ab19afda458c18d4a | |
| parent | f762e4ca0051a7a34123b5a526696f2feb5c6e03 (diff) | |
| download | qtmaildir-e19e2318fc637d50b86f663b21bd97494057232d.tar.gz qtmaildir-e19e2318fc637d50b86f663b21bd97494057232d.zip | |
feat(threads): mark an opened thread read after a delay
Opening a thread left it tagged unread, so the unread count never
matched what had actually been read and the app was quietly wrong every
day it was used. Item 6 of the usability backlog.
A single-shot timer, armed when a thread is selected and restarted rather
than stacked, so arrowing down a list marks only the thread still
selected when it fires and not every one passed through. Configurable
through mark_read_delay_ms in [general], defaulting to 2000: zero marks
read at once, and any negative value disables the behaviour, which is why
the value is neither clamped nor warned about at either end.
The automatic change deliberately does NOT go on the undo stack. It
routes through sendThreadTagChange() rather than tagSelected(), because
undoing an action the user never took is worse than leaving a thread
read, and toggle_unread already gives them a direct way back. It still
funnels through the single applyTags path; what differs is only whether
the inverse is pushed, which is a window-level decision above the worker.
An explicit toggle_unread cancels any pending timer, or marking a thread
unread by hand would be reversed a moment later and the key would look
broken.
Two guards beyond the plan, both from asking what happens when a timer
outlives the thread it was armed for. Arming is skipped for a thread that
is not unread, so no write is scheduled that would change nothing, and
the handler re-checks that its thread is still selected and still unread
before writing, so a stale timer does nothing rather than tagging the
wrong thread.
The plan expected the rapid-arrow case to need a database and a manual
check. It needs neither: ThreadListModel takes threads through
appendBatch(), so the case is unit-tested. All three tests were confirmed
to fail against deliberately broken versions, one arming for read threads
and one creating a timer per selection instead of restarting one.
Item 7 is closed in the same pass. The user verified against real mail
that HTML messages already open as HTML, which is what the item asked
for, so it is recorded as done with no code changed. The prefer_html key
it floated was not added: nobody has asked to default to plain text, and
Ctrl+H already switches a thread by hand.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| -rw-r--r-- | README.md | 6 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 48 | ||||
| -rw-r--r-- | src/config.cpp | 20 | ||||
| -rw-r--r-- | src/config.h | 8 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 85 | ||||
| -rw-r--r-- | src/mainwindow.h | 22 | ||||
| -rw-r--r-- | tests/test_config.cpp | 58 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 117 |
8 files changed, 362 insertions, 2 deletions
@@ -113,6 +113,12 @@ identity. ; Optional. Open the completion popup as soon as an empty query bar takes ; focus, without pressing the shortcut. Defaults to false. ; completion_on_focus = false +; Optional. How long an opened thread stays unread before it is marked read, +; in milliseconds. Defaults to 2000. Zero marks it read at once; any negative +; value turns the behaviour off, leaving threads unread until you toggle them +; with Ctrl+U. Arrowing quickly through a list marks only the thread you stop +; on, never the ones you pass through. +; mark_read_delay_ms = 2000 [completion] ; Optional. Extra content types offered after mimetype:, APPENDED to the 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 35d5937..417a980 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 @@ -45,8 +45,8 @@ taking that too literally. | 3 | Too few clickable affordances, shortcuts are the only route | discoverability | M | **done** | | 4 | Message-pane font size does not survive restart | persistence | S | **done** | | 5 | Thread list is cramped, poor readability | presentation | S | open | -| 6 | Opened message stays unread | behavior | S | open | -| 7 | HTML view should be default for HTML messages | behavior | XS | **verify first, may already be done** | +| 6 | Opened message stays unread | behavior | S | **done** | +| 7 | HTML view should be default for HTML messages | behavior | XS | **done** (already worked) | | 8 | No buttons or menu entries for archive, undo, etc | discoverability | M | **done** | | 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) | @@ -438,6 +438,35 @@ become read. `NotmuchWorker` tests already build, but the timer logic itself is UI-side and easier to check by hand. At minimum, verify the rapid-arrow case manually. +### Outcome (done) + +Built as specced, including every decision recorded above: a 2000 ms default, +`mark_read_delay_ms` in `[general]`, the automatic change kept off the undo +stack via `sendThreadTagChange()`, and an explicit `toggle_unread` cancelling +any pending timer. + +**The rapid-arrow case is unit-tested, not left to hand-checking.** The plan +expected it to need a database and a person; it needs neither. `ThreadListModel` +takes threads directly through `appendBatch()`, so a test builds three unread +rows, arrows through them, and asserts one timer stays armed. The timer carries +an object name so the test observes it through `findChild` rather than the +window exposing it. + +**The three tests were verified by breaking the code**, since a passing test +proves nothing until it has been seen to fail: + +- Removing the already-read check arms a timer for a read thread, caught. +- Creating a fresh timer per selection instead of restarting one, which is + precisely the stacking the plan warns about, fails two of the three. + +**Two guards the plan did not call for**, both from asking what happens when +the timer outlives its thread. `scheduleMarkRead()` refuses to arm for a thread +that is not unread, so a read thread never schedules a write that would change +nothing. `markCurrentThreadRead()` re-checks that the thread it was armed for +is still selected AND still unread before writing, so a timer that survives a +selection change or a manual toggle does nothing rather than tagging the wrong +thread. + ## 7. HTML view should be default for HTML messages **Verify before doing anything.** `MessageView::m_preferHtml` is already @@ -462,6 +491,21 @@ worth adding for people who want plain text by default. section of `CLAUDE.md`. Preferring HTML is orthogonal to remote content, which stays blocked and per-render. +### Outcome (done, 2026-08-04): nothing was broken + +**Verified by the user against real mail: HTML messages do open as HTML.** The +item was raised on an observation that could not be reproduced afterwards, and +the code was already correct: `m_preferHtml` initialises to `true` and `clear()` +resets it to `true`, so every thread starts in `PreferHtml`. + +No code changed. Recorded as done rather than dropped, since the behaviour the +item asked for is the behaviour that ships. + +The `prefer_html` config key the item floated for people who want plain text by +default was **not** added: nobody has asked for it, and `toggle_html` +(`Ctrl+H`) already switches a thread by hand. Add it if someone wants the +default flipped, not before. + ## 10. Reaching an account's inbox takes two steps **Observed:** select account from the dropdown, then click inbox or unread. diff --git a/src/config.cpp b/src/config.cpp index 1a8f322..f5d2d15 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -91,6 +91,26 @@ void Config::load(const QString &path) m_completionOnFocus = settings.value(QStringLiteral("completion_on_focus"), false).toBool(); + // Absent is silent, the default being 2000. Present but unparseable warns, + // for the same reason message_zoom does: the user asked for something and + // is not getting it. + // + // Zero and negative are NOT errors and must not be clamped. Zero means mark + // read at once, and any negative value means never, which is how the + // behaviour is turned off. + const QVariant markRead = settings.value(QStringLiteral("mark_read_delay_ms")); + if (markRead.isValid()) { + bool ok = false; + const int value = markRead.toString().toInt(&ok); + if (ok) { + m_markReadDelayMs = value; + } else { + addProblem(QStringLiteral("Mark-read delay '%1' is not a number; " + "using the default.") + .arg(markRead.toString())); + } + } + // [completion] is an ordinary section, so this one DOES take its prefix. // ',' separates entries and '|' separates a value from its description: // two different characters because QSettings splits comma lists itself, diff --git a/src/config.h b/src/config.h index 63fa541..7c6eb63 100644 --- a/src/config.h +++ b/src/config.h @@ -102,6 +102,13 @@ public: /// once it is known. The manual trigger works regardless. bool completionOnFocus() const { return m_completionOnFocus; } + /// How long an opened thread stays unread before it is marked read. + /// + /// Three meanings, all deliberate: a positive value is the delay in + /// milliseconds, 0 marks read immediately, and any negative value disables + /// the behaviour so a thread stays unread until toggled by hand. + int markReadDelayMs() const { return m_markReadDelayMs; } + /// User-supplied mimetype completions, APPENDED to the built-in list. /// Appending rather than replacing means a typo cannot leave completion /// worse off than the defaults. Mimetypes are the only completion list a @@ -135,6 +142,7 @@ private: QString m_notmuchConfig; qreal m_messageZoom = 1.0; bool m_completionOnFocus = false; + int m_markReadDelayMs = 2000; QList<CompletionEntry> m_extraMimetypes; QString m_startupQuery = QStringLiteral("Unread"); diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index a0ae7c4..45db452 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -42,6 +42,7 @@ #include <QStandardPaths> #include <QStatusBar> #include <QTableView> +#include <QTimer> #include <QToolBar> #include <QVBoxLayout> @@ -265,6 +266,14 @@ void MainWindow::buildUi() m_queryEdit->installEventFilter(this); m_queryCompleter = new QueryCompleter(m_queryEdit, m_config, this); + m_markReadTimer = new QTimer(this); + // Named so a test can observe whether it is armed without the window + // having to expose the timer or the decision that armed it. + m_markReadTimer->setObjectName(QStringLiteral("markReadTimer")); + m_markReadTimer->setSingleShot(true); + connect(m_markReadTimer, &QTimer::timeout, + this, &MainWindow::markCurrentThreadRead); + m_syncLog = new QPlainTextEdit(central); m_syncLog->setReadOnly(true); m_syncLog->setMaximumHeight(120); @@ -445,6 +454,13 @@ void MainWindow::registerActions() if (!current.isValid()) return; const ThreadSummary thread = m_model->threadAt(current.row()); + + // An explicit toggle overrides the automatic one. Without this, marking + // a thread unread by hand would be undone a moment later by a timer + // armed when it was opened, and the key would look broken. + m_markReadTimer->stop(); + m_markReadThreadId.clear(); + if (thread.isUnread()) tagSelected({}, { QStringLiteral("unread") }, tr("Mark read")); else @@ -843,6 +859,7 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, const ThreadSummary thread = m_model->threadAt(current.row()); m_currentThreadId = thread.threadId; m_messageView->setTags(thread.tags); + scheduleMarkRead(thread); QMetaObject::invokeMethod(m_worker, "loadThread", Qt::QueuedConnection, Q_ARG(QString, m_currentThreadId), Q_ARG(QString, m_lastQuery), @@ -932,6 +949,74 @@ void MainWindow::onSyncFinished(bool success, int exitCode) } } +void MainWindow::scheduleMarkRead(const ThreadSummary &thread) +{ + // Any pending timer belongs to a thread that is no longer on screen. + // Stopping unconditionally is what makes this a restart rather than a + // stack: arrowing down ten threads must mark only the one still selected + // when the timer finally fires. + m_markReadTimer->stop(); + m_markReadThreadId.clear(); + + // Negative disables the behaviour entirely, per the config key. + const int delay = m_config.markReadDelayMs(); + if (delay < 0) + return; + + // Nothing to do for a thread that is already read. Checked here rather + // than in the handler so no timer is even armed, which keeps a read thread + // from arming one that would fire into a no-op write. + if (!thread.tags.contains(QStringLiteral("unread"))) + return; + + m_markReadThreadId = thread.threadId; + + // Zero means immediately, and a zero-interval timer still fires through + // the event loop rather than reentering the selection handler. + m_markReadTimer->start(delay); +} + +void MainWindow::markCurrentThreadRead() +{ + if (m_markReadThreadId.isEmpty()) + return; + + // The selection can have moved on between the timer being armed and it + // firing, and the thread can have been marked read by hand in that window. + // Both mean this timer has nothing left to do. + if (m_markReadThreadId != m_currentThreadId) { + m_markReadThreadId.clear(); + return; + } + + const QModelIndex current = m_threadView->currentIndex(); + if (!current.isValid()) { + m_markReadThreadId.clear(); + return; + } + + const ThreadSummary thread = m_model->threadAt(current.row()); + if (thread.threadId != m_markReadThreadId + || !thread.tags.contains(QStringLiteral("unread"))) { + m_markReadThreadId.clear(); + return; + } + + const QStringList threadIds = { m_markReadThreadId }; + m_markReadThreadId.clear(); + + // sendThreadTagChange, NOT tagSelected: this deliberately does not go on + // the undo stack. The user never took this action, so hijacking Ctrl+Z to + // reverse it would undo something they did not do, and toggle_unread + // already gives them a direct way to put it back. Decided 2026-08-03. + // + // It still funnels through the one applyTags path, per CLAUDE.md; what + // differs is only whether the inverse is pushed, which is a window-level + // decision above the worker. + sendThreadTagChange(threadIds, {}, { QStringLiteral("unread") }, + tr("Mark read")); +} + void MainWindow::tagSelected(const QStringList &add, const QStringList &remove, const QString &description) { diff --git a/src/mainwindow.h b/src/mainwindow.h index 65f9636..0eea164 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -39,6 +39,7 @@ class QPushButton; class QComboBox; class QPlainTextEdit; class QSplitter; +class QTimer; class ThreadListModel; class MessageView; @@ -117,6 +118,15 @@ private: void tagSelected(const QStringList &add, const QStringList &remove, const QString &description); + /// Starts, restarts or cancels the mark-read timer for a newly opened + /// thread. Cancels outright for a thread that is not unread, so an already + /// read thread never schedules a write that would change nothing. + void scheduleMarkRead(const ThreadSummary &thread); + + /// Removes `unread` from the thread the timer was armed for, if it is still + /// the one on screen. + void markCurrentThreadRead(); + /// Sends a tag change for a set of threads without touching the undo stack. /// Both tagSelected() and ThreadTagCommand route through this. void sendThreadTagChange(const QStringList &threadIds, @@ -168,6 +178,18 @@ private: QString m_lastQuery; QString m_currentThreadId; + /// Marks the open thread read once it has been on screen long enough. + /// + /// Single-shot and RESTARTED on every selection change, never stacked: + /// arrowing down a list must mark only the thread still selected when it + /// fires, not each one passed through. + QTimer *m_markReadTimer = nullptr; + + /// The thread m_markReadTimer will mark read. Compared against the current + /// selection when it fires, so a timer that outlives its thread does + /// nothing rather than marking the wrong one. + QString m_markReadThreadId; + /// The optimistic update awaiting confirmation, kept so a worker error can /// put the model back. Only the most recent one: mutations are sent from /// the UI thread one user action at a time. diff --git a/tests/test_config.cpp b/tests/test_config.cpp index 451eb43..a09738b 100644 --- a/tests/test_config.cpp +++ b/tests/test_config.cpp @@ -41,6 +41,10 @@ private slots: void messageZoomDefaultsAndValidates(); void completionOnFocusDefaultsToFalse(); void completionOnFocusIsActuallyRead(); + void markReadDelayDefaultsToTwoSeconds(); + void markReadDelayIsActuallyRead(); + void markReadDelayAcceptsZeroAndNegative(); + void markReadDelayRejectsGarbage(); void extraMimetypesAppendToBuiltins(); void extraMimetypeDescriptionMayContainComma(); void malformedExtraMimetypeIsSkipped(); @@ -371,6 +375,60 @@ void TestConfig::completionOnFocusIsActuallyRead() QCOMPARE(config.completionOnFocus(), true); } +void TestConfig::markReadDelayDefaultsToTwoSeconds() +{ + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral("[general]\n"))); + QCOMPARE(config.markReadDelayMs(), 2000); + QVERIFY(config.problems().isEmpty()); +} + +void TestConfig::markReadDelayIsActuallyRead() +{ + // Round-trip a value that is not the default, which is what proves the key + // is really read: a "general/mark_read_delay_ms" lookup matches nothing and + // would still pass a test that only checked the default. + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral("[general]\n" + "mark_read_delay_ms=500\n"))); + QCOMPARE(config.markReadDelayMs(), 500); + QVERIFY(config.problems().isEmpty()); +} + +void TestConfig::markReadDelayAcceptsZeroAndNegative() +{ + // Both are documented settings, not mistakes: 0 marks read immediately and + // a negative value disables the behaviour entirely. Neither may be + // clamped away or warned about. + QTemporaryDir dir; + Config zero; + zero.load(writeIni(dir, QStringLiteral("[general]\n" + "mark_read_delay_ms=0\n"))); + QCOMPARE(zero.markReadDelayMs(), 0); + QVERIFY(zero.problems().isEmpty()); + + QTemporaryDir otherDir; + Config never; + never.load(writeIni(otherDir, QStringLiteral("[general]\n" + "mark_read_delay_ms=-1\n"))); + QCOMPARE(never.markReadDelayMs(), -1); + QVERIFY(never.problems().isEmpty()); +} + +void TestConfig::markReadDelayRejectsGarbage() +{ + // Absent is silent, but present-and-unparseable means the user asked for + // something and is not getting it, which warns rather than passing quietly. + QTemporaryDir dir; + Config config; + config.load(writeIni(dir, QStringLiteral("[general]\n" + "mark_read_delay_ms=soon\n"))); + QCOMPARE(config.markReadDelayMs(), 2000); + QVERIFY(!config.problems().isEmpty()); +} + void TestConfig::extraMimetypesAppendToBuiltins() { QTemporaryDir dir; diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index a66e1c2..fb33462 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -29,6 +29,7 @@ #include <QTemporaryDir> #include <QTableView> +#include <QTimer> #include "config.h" #include "keymap.h" @@ -55,6 +56,9 @@ private slots: void missingUiStateLeavesTheDefaults(); void headerStateFromADifferentColumnLayoutIsDiscarded(); void returnInTheQueryBarRunsTheQueryNotOpenThread(); + void markReadTimerRestartsRatherThanStacking(); + void markReadTimerIsNotArmedForAReadThread(); + void markReadCanBeDisabled(); }; void TestMainWindow::everyKnownActionIsRegistered() @@ -311,6 +315,119 @@ void TestMainWindow::returnInTheQueryBarRunsTheQueryNotOpenThread() QVERIFY(!actionFired); } +/// A thread summary carrying the tags a test needs. Enough to drive selection; +/// nothing here touches a database. +static ThreadSummary makeThread(const QString &id, const QStringList &tags) +{ + ThreadSummary thread; + thread.threadId = id; + thread.subject = QStringLiteral("Subject ") + id; + thread.authors = QStringLiteral("Someone <someone@example.org>"); + thread.tags = tags; + return thread; +} + +void TestMainWindow::markReadTimerRestartsRatherThanStacking() +{ + // The plan's hard requirement: arrowing quickly down a list must not mark + // every thread passed through as read, only the one still selected when the + // timer fires. A stacked timer per selection would mark all of them. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *timer = window.findChild<QTimer *>(QStringLiteral("markReadTimer")); + QVERIFY(timer); + auto *view = window.findChild<QTableView *>(); + QVERIFY(view); + + model->appendBatch({ makeThread(QStringLiteral("t1"), + { QStringLiteral("unread") }), + makeThread(QStringLiteral("t2"), + { QStringLiteral("unread") }), + makeThread(QStringLiteral("t3"), + { QStringLiteral("unread") }) }); + + view->selectRow(0); + QVERIFY2(timer->isActive(), "no timer armed for an unread thread"); + + // Move on before it can fire. One timer stays armed, not three. + view->selectRow(1); + QVERIFY(timer->isActive()); + view->selectRow(2); + QVERIFY(timer->isActive()); + + // Exactly one timer exists at all, which is what "restarted, not stacked" + // means concretely. + QCOMPARE(window.findChildren<QTimer *>(QStringLiteral("markReadTimer")).size(), + 1); +} + +void TestMainWindow::markReadTimerIsNotArmedForAReadThread() +{ + // Opening a thread that is already read must not schedule a write that + // would change nothing. + const Config config; + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *timer = window.findChild<QTimer *>(QStringLiteral("markReadTimer")); + QVERIFY(timer); + auto *view = window.findChild<QTableView *>(); + QVERIFY(view); + + model->appendBatch({ makeThread(QStringLiteral("read"), + { QStringLiteral("inbox") }), + makeThread(QStringLiteral("unread"), + { QStringLiteral("unread") }) }); + + view->selectRow(0); + QVERIFY2(!timer->isActive(), "armed a timer for an already-read thread"); + + // And the unread one still arms, so this is not "never arms". + view->selectRow(1); + QVERIFY(timer->isActive()); + + // Moving back to a read thread disarms it again, rather than leaving the + // previous thread's timer running to fire against the wrong row. + view->selectRow(0); + QVERIFY(!timer->isActive()); +} + +void TestMainWindow::markReadCanBeDisabled() +{ + // A negative delay turns the behaviour off entirely. Documented, so it must + // work rather than being clamped to "immediately". + QTemporaryDir dir; + const QString path = dir.filePath(QStringLiteral("qtmaildir.conf")); + { + QFile file(path); + QVERIFY(file.open(QIODevice::WriteOnly | QIODevice::Text)); + file.write("[general]\nmark_read_delay_ms=-1\n"); + } + + Config config; + config.load(path); + QCOMPARE(config.markReadDelayMs(), -1); + + MainWindow window(config); + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *timer = window.findChild<QTimer *>(QStringLiteral("markReadTimer")); + QVERIFY(timer); + auto *view = window.findChild<QTableView *>(); + QVERIFY(view); + + model->appendBatch({ makeThread(QStringLiteral("t1"), + { QStringLiteral("unread") }) }); + view->selectRow(0); + + QVERIFY2(!timer->isActive(), + "a negative mark_read_delay_ms must disable the timer"); +} + // 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. |
