From 69e777cc72bd846c250d68656a8d558c9401fcd5 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 4 Aug 2026 19:16:07 +0200 Subject: feat(ui): disable Sync during a background sync, and blank the pane on Esc Items 29 and 32. 29 was a constraint item 27 specified and that shipped unbuilt: while a cron sync held the lock the Sync button stayed clickable, and pressing it could only produce the EX_TEMPFAIL skip. The progress bar and the button are now written by one updateSyncControls() taking both sync sources, which the item asked for by name: two independent assignments, one per path, means whichever finishes second wins, so a background sync ending would re-enable the button in the middle of a local run. Unknown re-enables the button, deliberately. It means /proc/locks could not be read and nothing was observed, so leaving the button disabled would strand it permanently wherever the lock cannot be seen. 32 adds a clear_pane action on Esc. It clears m_currentThreadId with the pane, not merely alongside it, or a threadLoaded still in flight would paint the thread straight back; and it cancels any pending mark-read, since a thread blanked from view must not be marked read two seconds later. The selection, the query and the undo stack are untouched. The one real risk in 32 was Escape being stolen from the query completer, the way Return was once lost to a window shortcut. Probed rather than reasoned about: a popup consumes the key before a window-level shortcut sees it, so the completer still dismisses. Every test here was verified by reverting the code it covers. Co-Authored-By: Claude Opus 5 --- .../plans/2026-08-03-post-0.1.0-usability.md | 36 ++++++- src/keymap.cpp | 5 + src/mainwindow.cpp | 56 +++++++++-- src/mainwindow.h | 17 ++++ 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> 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 #include #include +#include #include #include #include @@ -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(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(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(QStringLiteral("clear_pane")); + QVERIFY2(action, "no clear_pane action registered"); + QCOMPARE(action->shortcut(), QKeySequence(Qt::Key_Escape)); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + 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. -- cgit v1.2.3