diff options
| -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. |