diff options
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 36 | ||||
| -rw-r--r-- | src/keymap.cpp | 5 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 56 | ||||
| -rw-r--r-- | src/mainwindow.h | 17 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 105 |
5 files changed, 210 insertions, 9 deletions
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 4dab8a9..f7cf0e3 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 @@ -73,10 +73,10 @@ taking that too literally. | 26 | No way to add or remove an arbitrary tag from the UI | workflow | S | **done** | | 27 | The UI cannot see a sync it did not start | feedback | S | **done** | | 28 | Re-adding `unread` counts 2 unsynced changes, not 0 | correctness | S | open | -| 29 | Sync button stays enabled during a background sync | feedback | XS | open | +| 29 | Sync button stays enabled during a background sync | feedback | XS | **done** | | 30 | The blank right pane is wasted space | presentation | M | open | | 31 | The quit prompt has no highlighted default button | discoverability | XS | open, needs repro | -| 32 | Esc does not blank the right pane | workflow | XS | open | +| 32 | Esc does not blank the right pane | workflow | XS | **done** | | 33 | Status bar messages never expire | feedback | S | open | | 34 | No overview of the Maildir itself | information | M | open | | 35 | No refresh of the thread list after a sync | workflow | M | open | @@ -1403,6 +1403,22 @@ share the progress bar; the button is the piece that was missed. - Exit 75 handling stays. Disabling the button makes the skip rarer, not impossible: cron can take the lock between the poll and the click. +### Outcome (done) + +`updateSyncControls()` is the single function the constraint called for, taking +`m_localSyncBusy` and `m_externalSyncBusy` and writing both the progress bar and +the button. Neither sync path touches those widgets directly any more. + +**The external state is tracked as its own flag rather than read back from +`SyncMonitor`.** An earlier version called `m_syncMonitor->state()` inside the +update, which meant the handler received a state and then ignored it in favour +of re-reading the source. Acting on what you were told is both easier to follow +and testable without a live monitor. + +Unknown clears the busy flag exactly as Idle does, per the constraint. Verified +by reverting that half: leaving it set on anything but Idle strands the button +disabled, and the test catches it. + ## 30. The blank right pane is wasted space **Observed (user, 2026-08-04):** with no thread selected the message pane is @@ -1499,6 +1515,22 @@ race documented in `CLAUDE.md` and fixed in 0.8.0. - Decide what Escape does when the pane is already blank. Doing nothing is fine; clearing the selection as a second step would be surprising. +### Outcome (done) + +A `clear_pane` action bound to `Esc`, registered like every other action so it +reaches the menus, the shortcut reference and `[keys]`. It clears +`m_currentThreadId` alongside the pane, and cancels any pending mark-read: a +thread blanked from view must not be marked read two seconds later. + +**The completer keeps its Escape, verified by probe rather than by reading the +code.** A popup consumes the key before a window-level shortcut sees it: with +the popup open the popup's filter fires and the action does not, and with it +closed the action fires. This was the one real risk in the item, since `Return` +had already been lost to a window shortcut this way (item 21). + +Blanking is a view change only: the selection, the query and the undo stack are +untouched, which a test pins. + ## 33. Status bar messages never expire **Observed (user, 2026-08-04):** "the status bar should return to default status diff --git a/src/keymap.cpp b/src/keymap.cpp index dcb63a8..f27f9a9 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -36,6 +36,7 @@ QStringList KeyMap::knownActions() QStringLiteral("focus_query"), QStringLiteral("complete_query"), QStringLiteral("select_all"), + QStringLiteral("clear_pane"), QStringLiteral("toggle_html"), QStringLiteral("load_remote"), QStringLiteral("message_details"), @@ -76,6 +77,10 @@ QList<QPair<QString, QString>> KeyMap::defaultBindings() // The conventional select-all key, and free here: the thread list is a // read-only view, so nothing else in the window wants it. { QStringLiteral("Ctrl+A"), QStringLiteral("select_all") }, + // Escape is not claimed by anything else at window level. The query + // completer handles its own Escape while its popup is up, and a popup + // consumes the key before a window shortcut sees it. + { QStringLiteral("Esc"), QStringLiteral("clear_pane") }, { QStringLiteral("Ctrl+H"), QStringLiteral("toggle_html") }, { QStringLiteral("Ctrl+M"), QStringLiteral("load_remote") }, // Shifted because Ctrl+D is delete. Both are "D for details/delete" diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 792ba3f..e546af5 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -410,6 +410,7 @@ void MainWindow::buildUi() m_syncLogPane->hide(); m_syncButton = new QPushButton(tr("Sync"), central); + m_syncButton->setObjectName(QStringLiteral("syncButton")); m_sync = new MailSync(m_config.syncCommand(), this); m_syncButton->setEnabled(m_sync->isAvailable()); if (!m_sync->isAvailable()) { @@ -679,6 +680,21 @@ void MainWindow::registerActions() m_queryEdit->setFocus(); m_queryCompleter->triggerCompletion(); }); + addAction(QStringLiteral("clear_pane"), tr("Clear &message pane"), + tr("Blank the message pane without changing the selection"), + [this]() { + // A view change, not a mail change: the selection, the query and the + // undo stack are all left alone. + // + // m_currentThreadId is cleared with the pane, not merely alongside it. + // A threadLoaded still in flight for that id would otherwise paint the + // thread straight back, which is the queued-reply race documented in + // CLAUDE.md. + m_currentThreadId.clear(); + m_messageView->clear(); + m_markReadTimer->stop(); + m_markReadThreadId.clear(); + }); addAction(QStringLiteral("select_all"), tr("Select &all threads"), tr("Select every thread in the current result list"), [this]() { // A registered action rather than the view's built-in SelectAll key, so @@ -1318,7 +1334,8 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) if (m_localSyncHoldsLock) return; - m_syncProgress->setVisible(true); + m_externalSyncBusy = true; + updateSyncControls(); m_statusLabel->setText(tr("Background sync running...")); return; } @@ -1327,10 +1344,16 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) // said what happened, including for a failure, so there is nothing to add. if (m_localSyncHoldsLock) { m_localSyncHoldsLock = false; + m_externalSyncBusy = false; + updateSyncControls(); return; } - m_syncProgress->setVisible(false); + // Cleared for Idle AND for Unknown. Unknown means /proc/locks could not be + // read, so nothing is observed; leaving the button disabled there would + // strand it permanently on a platform that cannot see the lock at all. + m_externalSyncBusy = false; + updateSyncControls(); // Deliberately reports rather than refreshes. runCurrentQuery() clears the // undo stack, the selection and the message pane, which is right for a @@ -1350,16 +1373,35 @@ void MainWindow::onExternalSyncStateChanged(SyncMonitor::State state) void MainWindow::setSyncBusy(bool busy) { - m_syncProgress->setVisible(busy); - // Disabled rather than left clickable: MailSync::start() already refuses a - // second run, but a button that looks live and does nothing is worse than - // one that shows it is unavailable. - m_syncButton->setEnabled(!busy && m_sync && m_sync->isAvailable()); + m_localSyncBusy = busy; + updateSyncControls(); if (busy) m_statusLabel->setText(tr("Syncing...")); } +void MainWindow::updateSyncControls() +{ + // ONE function of both states, deliberately. Two independent assignments, + // one per sync path, means whichever fires second wins: a background sync + // ending would re-enable the button in the middle of a local run, and a + // local run ending would re-enable it while cron still holds the lock. + const bool busy = m_localSyncBusy || m_externalSyncBusy; + + m_syncProgress->setVisible(busy); + + // Disabled rather than left clickable: MailSync::start() already refuses a + // second run and the script exits 75 when another holds the lock, but a + // button that looks live and does nothing is worse than one that shows it + // is unavailable. + // + // Note this reads Running specifically, not "not Idle". Unknown means + // /proc/locks could not be read and nothing was observed, so the button + // stays usable: permanently disabling it where the lock cannot be seen is + // worse than occasionally offering a run that gets skipped. + m_syncButton->setEnabled(!busy && m_sync && m_sync->isAvailable()); +} + void MainWindow::updatePendingIndicator() { if (m_pendingEdits <= 0) { diff --git a/src/mainwindow.h b/src/mainwindow.h index 66044dc..61722aa 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -175,6 +175,14 @@ private: /// says "working, duration unknown", which is the truth. void setSyncBusy(bool busy); + /// Applies the sync progress bar and button state from BOTH sync sources. + /// + /// One function of both, never two assignments: with a local and a + /// background sync each writing the widgets independently, whichever + /// finished second would win and re-enable the button while the other was + /// still running. + void updateSyncControls(); + /// Opens the tag dialog on the current selection and applies its result. /// @@ -223,6 +231,15 @@ private: /// MailSync::isRunning() is already false and can no longer answer "was /// that ours?". bool m_localSyncHoldsLock = false; + + /// True while a sync this window started is running. Half of the input to + /// updateSyncControls(). + bool m_localSyncBusy = false; + + /// True while a sync this window did NOT start holds the lock. The other + /// half. Tracked here rather than read back from SyncMonitor so the state + /// the UI acted on is the state it was told about. + bool m_externalSyncBusy = false; QUndoStack m_undoStack; QLineEdit *m_queryEdit = nullptr; diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 93bb853..8a1dde3 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -26,6 +26,7 @@ #include <QLabel> #include <QLineEdit> #include <QMenu> +#include <QPushButton> #include <QProgressBar> #include <QFile> #include <QSettings> @@ -78,6 +79,9 @@ private slots: void aLocalSyncIsNotReportedAsABackgroundOne(); void aLocalSyncsOwnLockIsNeverReportedAsBackground(); void aSkippedLocalSyncStillReportsTheOtherRunFinishing(); + void theSyncButtonIsDisabledWhileABackgroundSyncHoldsTheLock(); + void anUnobservableLockTableLeavesTheSyncButtonUsable(); + void escapeBlanksTheMessagePane(); }; void TestMainWindow::everyKnownActionIsRegistered() @@ -965,6 +969,107 @@ void TestMainWindow::aSkippedLocalSyncStillReportsTheOtherRunFinishing() "says '%1'").arg(status->text()))); } +void TestMainWindow::theSyncButtonIsDisabledWhileABackgroundSyncHoldsTheLock() +{ + // Item 27 specified this and it shipped unbuilt: while a cron sync holds + // the lock the button stayed clickable, and pressing it could only produce + // the EX_TEMPFAIL skip. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + QVERIFY(QDir().mkpath(dir.filePath(QStringLiteral("qtmaildir")))); + const QString conf = dir.filePath(QStringLiteral("qtmaildir/qtmaildir.conf")); + { + QSettings s(conf, QSettings::IniFormat); + s.setValue(QStringLiteral("sync/command"), QStringLiteral("/bin/true")); + } + + Config config; + config.load(conf); + MainWindow window(config); + + auto *button = window.findChild<QPushButton *>(QStringLiteral("syncButton")); + QVERIFY2(button, "no sync button to check"); + QVERIFY2(button->isEnabled(), "the button starts disabled with a command set"); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Running)); + QVERIFY2(!button->isEnabled(), + "the sync button stayed enabled during a background sync"); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Idle)); + QVERIFY2(button->isEnabled(), + "the sync button was not re-enabled after the background sync"); +} + +void TestMainWindow::anUnobservableLockTableLeavesTheSyncButtonUsable() +{ + // Unknown means /proc/locks could not be read, so nothing was observed. A + // button left permanently disabled on a platform that cannot see the lock + // is worse than one that occasionally offers a run that gets skipped. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + QVERIFY(QDir().mkpath(dir.filePath(QStringLiteral("qtmaildir")))); + const QString conf = dir.filePath(QStringLiteral("qtmaildir/qtmaildir.conf")); + { + QSettings s(conf, QSettings::IniFormat); + s.setValue(QStringLiteral("sync/command"), QStringLiteral("/bin/true")); + } + + Config config; + config.load(conf); + MainWindow window(config); + + auto *button = window.findChild<QPushButton *>(QStringLiteral("syncButton")); + QVERIFY(button); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Running)); + QVERIFY(!button->isEnabled()); + + QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", + Q_ARG(SyncMonitor::State, + SyncMonitor::State::Unknown)); + QVERIFY2(button->isEnabled(), + "an unobservable lock table left the sync button disabled"); +} + +void TestMainWindow::escapeBlanksTheMessagePane() +{ + // A registered action like any other, so it reaches the menus, the shortcut + // reference and [keys]. Clearing m_currentThreadId with the pane is the + // part that matters: a late threadLoaded would otherwise paint the thread + // straight back, which is the race fixed in 0.8.0. + const Config config; + MainWindow window(config); + + auto *action = window.findChild<QAction *>(QStringLiteral("clear_pane")); + QVERIFY2(action, "no clear_pane action registered"); + QCOMPARE(action->shortcut(), QKeySequence(Qt::Key_Escape)); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTableView *>(); + QVERIFY(view); + + model->appendBatch({ makeThread(QStringLiteral("t1"), {}), + makeThread(QStringLiteral("t2"), {}) }); + + view->selectRow(0); + QVERIFY2(!window.currentThreadId().isEmpty(), + "no thread was opened to blank"); + + action->trigger(); + QVERIFY2(window.currentThreadId().isEmpty(), + "Escape left the thread loaded in the pane"); + + // Blanking is a view change, not a mail change: the selection stays. + QCOMPARE(view->selectionModel()->selectedRows().size(), 1); +} + // 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. |