diff options
| -rw-r--r-- | CLAUDE.md | 16 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 128 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 85 | ||||
| -rw-r--r-- | src/mainwindow.h | 5 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 43 |
5 files changed, 216 insertions, 61 deletions
@@ -40,13 +40,23 @@ Maildir. **No network protocol work at all** — fetching and sending are extern ``` UI thread Worker thread MainWindow NotmuchWorker - ├ QueryBar / SavedQueryBar └ owns the only notmuch_database_t* - ├ ThreadListView ── ThreadListModel - └ MessageView (HeaderWidget, QWebEngineView, AttachmentBar) + ├ query row: QComboBox, QLineEdit, └ owns the only notmuch_database_t* + │ saved-query QPushButtons + ├ QTableView ── ThreadListModel (SubjectDelegate) + └ MessageView (header QLabel, QWebEngineView, attachment bar, TagStrip) Config (INI) KeyMap MailSync (QProcess) MimeParser (GMime) +SyncMonitor (/proc/locks) TagColors QueryCompleter ThreadCidMap ``` +The query row and the message-pane header are **built inline in `MainWindow` and +`MessageView`**, not as named widget classes. Earlier revisions of this diagram +listed `QueryBar`, `SavedQueryBar`, `HeaderWidget` and `AttachmentBar`; none of +those types have ever existed, and looking for them wastes a search. The widget +classes that do exist are `MessageView`, `TagStrip`, `TagDialog` and +`SubjectDelegate`; `TagChip` is a namespace of painting helpers, not a widget, +and `ThreadCidMap` is a struct. + **No `notmuch_*` pointer ever crosses the thread boundary.** Data crosses as the plain value structs in `src/types.h` (`ThreadSummary`, `MessageRef`, `TagChange`), over queued signals in both directions. `notmuchworker.cpp` is the only file that includes `notmuch.h` 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 dd6a8ca..85e6ce8 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 @@ -92,8 +92,9 @@ taking that too literally. | 42 | "Syncing..." says nothing about what is being synced | feedback | S | open | | 43 | No "Mark all read" for the current view | workflow | S | open | | 44 | No way to manage the filters applied at sync time | workflow | ? | open, unspecified | -| 45 | Two Sync buttons on the main window | discoverability | XS | open | +| 45 | Two Sync buttons, and only one of them works properly | correctness | S | **done** | | 46 | `uiStateSurvivesARestart` fails under the offscreen platform | testing | XS | **done** | +| 47 | The query bar looks unfinished, and cannot be cleared by mouse | presentation | XS | **done** | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -2175,17 +2176,91 @@ passes.** `m_syncButton` is a `QPushButton` created in `buildUi()` toolbar, from item 3's menu work. Nothing removed the original button when the toolbar gained one, so the window shows both. -**Approach.** Drop the standalone `QPushButton` and keep the toolbar action. -The action carries its shortcut, its enabled state and its menu entry from one +### Revised 2026-08-06: this is a defect, not a cosmetic cleanup + +**The two controls do different things**, which the original write-up assumed +away by treating them as duplicates. Confirmed in code after the user reported +that the toolbar one "doesn't perform a sync": + +- The `QPushButton` handler (`src/mainwindow.cpp:464`) starts the sync, clears + the log pane, shows it, disables the button, and reports "Sync already + running" when `start()` returns false. +- The `sync` QAction handler (`src/mainwindow.cpp:721`) is + `if (m_sync->isAvailable()) m_sync->start();` and nothing else. No log pane, + no disable, and the return value is discarded, so a rejected start is silent. + +So the toolbar button most likely *does* start a sync; every piece of evidence +that it did lives in the other handler. That is item 13's failure restated: the +feedback exists where the user cannot see it. + +**Worse, item 29 shipped for one widget only.** `onExternalSyncStateChanged` +disables `m_syncButton` during a background sync (`src/mainwindow.cpp:1606`) and +never touches the action. During a cron sync the toolbar Sync stays clickable +and can only produce the EX_TEMPFAIL skip, which is the exact behaviour item 29 +exists to prevent. The user's note about "2 sync buttons" is therefore sitting +on top of a live defect rather than a redundancy. + +**The user's preference (2026-08-06):** keep the top-left one, next to Archive, +Delete and Undo. The one beside the query bar reads instinctively as a Search +button, which is a real misaffordance given what sits next to it. + +**Approach.** Not "pick a survivor". Move the button's handler onto the action, +so the two behave identically, then drop the now-redundant `QPushButton`. The +action carries its shortcut, its enabled state and its menu entry from one place, which is the whole point of item 3's conversion; the loose button is the last widget that predates it. +Route the enabled state through the action too, so `setEnabled` has one target +rather than two that can disagree. `QAction::setEnabled` propagates to every +widget showing it, which is what makes this smaller than it looks. + **Constraints.** - **The button is not just a button today.** It is disabled while a sync runs, including one started externally (item 27/29), and `test_mainwindow` asserts on it by name. Whatever replaces it has to carry that state, and the tests need - pointing at the action rather than at the widget. + pointing at the action rather than at the widget. Three tests find it via + `findChild<QPushButton *>("syncButton")`, including the one item 38 just + fixed, so they move together with the widget. + +- **A test must cover the toolbar path specifically.** The whole defect is that + one of two controls was never given the behaviour, and a test that drives only + the surviving widget would have passed throughout. Assert on the action's + enabled state during a background sync, which is the half that silently never + worked. + +### Outcome (done) + +Built in the order the user asked for: make the toolbar control work, prove it, +then remove the other one. + +`startSync()` is now the single handler behind every route in, the toolbar, the +File menu, the shortcut and, until it was removed, the button. The old action +handler was `if (m_sync->isAvailable()) m_sync->start();`, which cleared no log, +opened no pane, disabled nothing and discarded `start()`'s return value, so a +rejected start was silent. It now also reports when no sync command is +configured rather than doing nothing at all. + +`setSyncBusy()` sets the enabled state on the QAction, which reaches the toolbar +button, the menu entry and the shortcut at once. That was the actual defect: +item 29 set a separate QPushButton and never touched the action. + +**Verified red first, then load-bearing.** +`theSyncActionIsDisabledWhileABackgroundSyncHoldsTheLock` fails before the fix +with the action still enabled during a background sync, and fails again when the +action's `setEnabled` is removed afterwards. The pre-existing button test passed +throughout, which is exactly why the defect survived item 29: it drove the half +that worked. + +**Confirmed by hand before the widget was removed**, per the user's condition: +reading mail grew the unsynced count, the toolbar Sync ran the sync, and the app +refreshed and reported "Sync complete" at the end. + +**Then the QPushButton went.** Its unavailable-command tooltip moved to the +action, since with no command configured the control is disabled and the tooltip +is the only thing that says why. The old button test was deleted rather than +repointed, being an exact duplicate of the new action test, and the +unobservable-lock-table test now asserts on the action. - Check for other loose widgets doing the same thing before touching this one, so the fix is not repeated per widget later. - Removing a visible control is the kind of change that looks like a regression. @@ -2241,6 +2316,51 @@ Verified both ways round, since this one passed on Wayland throughout: 45 of 45 under offscreen where it previously failed, and still green on the real platform. +## 47. The query bar looks unfinished, and cannot be cleared by mouse + +**Observed (user, 2026-08-06), immediately after item 45 removed the Sync +button:** the bar "having no button seems kind of incomplete", and the user +asked whether a clear icon could be shown in it. + +**Cause:** the query field was the last stretching item in its row, so with the +Sync button gone it ran flush to the window edge with nothing terminating it. +Clearing it needed the keyboard; `QLineEdit` does not draw a clear button unless +asked. + +**Approach, and what was deliberately NOT built.** The user's first instinct was +a "🔎 Search" button. That was argued against and dropped: Return already runs +the query, and a button beside a text field is exactly what read as Search and +got removed in item 45. Adding one back would restate items 13 and 45 in a new +spot. + +What was built instead: + +- `setClearButtonEnabled(true)` on the query field. Qt draws the ✕ inside the + field, shows it only when there is text, and themes it from the desktop. One + line, no icon asset, no new widget. +- The saved-query buttons moved from their own row onto the query row, after the + field, so the bar is framed by the account dropdown on the left and the saved + queries on the right. The row they left is gone and the thread list gains that + vertical space. This was the user's own proposal and it addresses the + "incomplete" reading directly, without adding a control. + +**Constraints.** + +- **`[queries]` is unbounded.** Three entries fit comfortably; enough of them + would squeeze the field. No overflow handling was built, marked with a + `ponytail:` comment pointing at item 23, which already specifies + buttons-plus-menu and is where that belongs. +- Button order follows `childKeys()`, which sorts alphabetically, so the buttons + read Flagged, Inbox, Unread regardless of the order written in the config. + Pre-existing, and its own item if it matters. + +**Known behaviour, accepted:** clicking ✕ focuses the field, and an empty field +makes `QueryCompleter` offer everything, so the popup opens. Confirmed by the +user as acceptable; suppressible if it becomes annoying in daily use. + +**Verification:** by hand. The tests do not click the ✕, which is a mouse path. +The user confirmed the icon renders correctly, is themed, and clears the field. + ## Deferred, unsized, or split out Items noted while triaging but not part of the original list. Same numbering diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 75549d3..a2174a2 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -401,6 +401,10 @@ void MainWindow::buildUi() m_queryEdit = new QLineEdit(central); m_queryEdit->setPlaceholderText(tr("notmuch query, e.g. tag:inbox")); + // Qt draws the clear button inside the field and shows it only when there + // is text, themed by the desktop. A hand-rolled button beside the bar would + // read as "Search" and duplicate Return, which is how item 45 started. + m_queryEdit->setClearButtonEnabled(true); connect(m_queryEdit, &QLineEdit::returnPressed, this, &MainWindow::runCurrentQuery); @@ -453,24 +457,11 @@ void MainWindow::buildUi() m_syncLogPane->hide(); - m_syncButton = new QPushButton(tr("Sync"), central); - m_syncButton->setObjectName(QStringLiteral("syncButton")); + // Sync is reached from the toolbar, the File menu and the shortcut, all of + // them one QAction. A second QPushButton sat beside the query bar until + // 0.9.x, where it read as a Search button given what it stood next to, and + // carried behaviour the action did not: item 45. m_sync = new MailSync(m_config.syncCommand(), this); - m_syncButton->setEnabled(m_sync->isAvailable()); - if (!m_sync->isAvailable()) { - m_syncButton->setToolTip( - tr("No sync command configured ([sync] command in qtmaildir.conf)")); - } - connect(m_syncButton, &QPushButton::clicked, this, [this]() { - if (!m_sync->start()) { - showTransientStatus(tr("Sync already running")); - return; - } - // Fresh run, fresh output: leaving the previous run's lines in place - // makes a stale failure look like the current one. - m_syncLog->clear(); - setSyncBusy(true); - }); connect(m_sync, &MailSync::finished, this, &MainWindow::onSyncFinished); connect(m_sync, &MailSync::outputReceived, this, [this](const QString &chunk) { m_syncLog->appendPlainText(chunk.trimmed()); @@ -485,23 +476,25 @@ void MainWindow::buildUi() this, &MainWindow::onExternalSyncStateChanged); m_syncMonitor->start(); + // One row: the account dropdown, the query field, then the saved queries. + // The field is the only stretching item, so it is framed on both sides + // rather than running flush to the window edge, which is what the removed + // Sync button used to terminate. + // + // ponytail: no overflow handling. [queries] is unbounded and enough entries + // would squeeze the field, but three is the real-world case today. Item 23 + // already specifies buttons-plus-menu and is where that belongs. queryRow->addWidget(m_accountBox); queryRow->addWidget(m_queryEdit, 1); - queryRow->addWidget(m_syncButton); - layout->addLayout(queryRow); - - // Saved query buttons. - auto *savedRow = new QHBoxLayout; for (const SavedQuery &saved : m_config.savedQueries()) { auto *button = new QPushButton(saved.name, central); connect(button, &QPushButton::clicked, this, [this, saved]() { m_queryEdit->setText(saved.query); runCurrentQuery(); }); - savedRow->addWidget(button); + queryRow->addWidget(button); } - savedRow->addStretch(); - layout->addLayout(savedRow); + layout->addLayout(queryRow); // Thread list and message pane. m_model = new ThreadListModel(this); @@ -735,8 +728,7 @@ void MainWindow::registerActions() }); addAction(QStringLiteral("sync"), tr("&Sync"), tr("Run the configured sync command"), [this]() { - if (m_sync->isAvailable()) - m_sync->start(); + startSync(); }); addAction(QStringLiteral("complete_query"), tr("&Complete query"), tr("Offer completions for the query bar"), [this]() { @@ -870,7 +862,16 @@ void MainWindow::buildMenus() auto *toolBar = addToolBar(tr("Main")); toolBar->setObjectName(QStringLiteral("main_toolbar")); toolBar->setToolButtonStyle(Qt::ToolButtonTextBesideIcon); - toolBar->addAction(m_actions.value(QStringLiteral("sync"))); + QAction *syncAction = m_actions.value(QStringLiteral("sync")); + // Carried over from the QPushButton this replaced: with no command + // configured the control is disabled, and the tooltip is the only thing + // that says why. + if (syncAction && m_sync && !m_sync->isAvailable()) { + syncAction->setEnabled(false); + syncAction->setToolTip( + tr("No sync command configured ([sync] command in qtmaildir.conf)")); + } + toolBar->addAction(syncAction); toolBar->addSeparator(); toolBar->addAction(m_actions.value(QStringLiteral("archive"))); toolBar->addAction(m_actions.value(QStringLiteral("delete"))); @@ -1603,7 +1604,33 @@ void MainWindow::updateSyncControls() // /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()); + // The QAction is the only Sync control now, and setEnabled on it reaches + // the toolbar button, the menu entry and the shortcut at once. Item 29 + // originally set a separate QPushButton and missed the action entirely, so + // the toolbar stayed clickable through a background sync. + if (QAction *action = m_actions.value(QStringLiteral("sync"))) + action->setEnabled(!busy && m_sync && m_sync->isAvailable()); +} + +void MainWindow::startSync() +{ + // One handler for every route in: the toolbar, the menu, the shortcut and + // the button. They previously had two, and only the button's cleared the + // log, showed the pane and disabled the control, so a sync started from the + // toolbar ran with no visible sign it had. + if (!m_sync->isAvailable()) { + showTransientStatus( + tr("No sync command configured ([sync] command in qtmaildir.conf)")); + return; + } + if (!m_sync->start()) { + showTransientStatus(tr("Sync already running")); + return; + } + // Fresh run, fresh output: leaving the previous run's lines in place + // makes a stale failure look like the current one. + m_syncLog->clear(); + setSyncBusy(true); } void MainWindow::recordPendingEdit(const QString &messageId, const QString &tag, diff --git a/src/mainwindow.h b/src/mainwindow.h index 96b27f4..e673181 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -217,6 +217,10 @@ private: /// says "working, duration unknown", which is the truth. void setSyncBusy(bool busy); + /// Starts a sync and shows that it started. Every route in goes through + /// here: the toolbar, the menu, the shortcut and the button. + void startSync(); + /// Applies the sync progress bar and button state from BOTH sync sources. /// /// One function of both, never two assignments: with a local and a @@ -322,7 +326,6 @@ private: QMenu *m_threadContextMenu = nullptr; QSplitter *m_splitter = nullptr; QComboBox *m_accountBox = nullptr; - QPushButton *m_syncButton = nullptr; QLabel *m_statusLabel = nullptr; /// Expires a transient status message. See showTransientStatus(). diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 6a4f903..480be39 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -79,8 +79,8 @@ private slots: void aLocalSyncIsNotReportedAsABackgroundOne(); void aLocalSyncsOwnLockIsNeverReportedAsBackground(); void aSkippedLocalSyncStillReportsTheOtherRunFinishing(); - void theSyncButtonIsDisabledWhileABackgroundSyncHoldsTheLock(); void anUnobservableLockTableLeavesTheSyncButtonUsable(); + void theSyncActionIsDisabledWhileABackgroundSyncHoldsTheLock(); void escapeBlanksTheMessagePane(); void deleteTogglesOnAnAlreadyDeletedThread(); void deleteOnAMixedSelectionDeletesRatherThanSplittingIt(); @@ -985,11 +985,13 @@ void TestMainWindow::aSkippedLocalSyncStillReportsTheOtherRunFinishing() "says '%1'").arg(status->text()))); } -void TestMainWindow::theSyncButtonIsDisabledWhileABackgroundSyncHoldsTheLock() +void TestMainWindow::theSyncActionIsDisabledWhileABackgroundSyncHoldsTheLock() { - // 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. + // Item 29 shipped for the QPushButton only: onExternalSyncStateChanged + // disabled m_syncButton and never touched the QAction, so the toolbar and + // menu Sync stayed clickable during a cron sync and could only produce the + // EX_TEMPFAIL skip. The button-based test passed throughout, because it + // drove the half that worked. QTemporaryDir dir; QVERIFY(dir.isValid()); QVERIFY(QDir().mkpath(dir.filePath(QStringLiteral("qtmaildir")))); @@ -999,10 +1001,6 @@ void TestMainWindow::theSyncButtonIsDisabledWhileABackgroundSyncHoldsTheLock() s.setValue(QStringLiteral("sync/command"), QStringLiteral("/bin/true")); } - // An empty lock table, so construction observes no sync. Against the real - // /proc/locks this assertion fails whenever the user's cron sync happens to - // be running: cron fires every ten minutes and a run lasts ~35s, so roughly - // 6% of runs landed inside one and the failure looked like flakiness. const QString locks = dir.filePath(QStringLiteral("locks")); { QFile f(locks); @@ -1014,25 +1012,22 @@ void TestMainWindow::theSyncButtonIsDisabledWhileABackgroundSyncHoldsTheLock() 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"); + auto *action = window.findChild<QAction *>(QStringLiteral("sync")); + QVERIFY2(action, "no sync action to check"); + QVERIFY2(action->isEnabled(), "the action 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"); + QVERIFY2(!action->isEnabled(), + "the sync action 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"); + QVERIFY2(action->isEnabled(), + "the sync action was not re-enabled after the background sync"); - // The override is process-wide, and QTemporaryDir takes the file with it at - // the end of this scope: leaving it set would point every later window at a - // path that no longer exists. MainWindow::setLocksPathForTesting(QStringLiteral("/proc/locks")); } @@ -1054,19 +1049,19 @@ void TestMainWindow::anUnobservableLockTableLeavesTheSyncButtonUsable() config.load(conf); MainWindow window(config); - auto *button = window.findChild<QPushButton *>(QStringLiteral("syncButton")); - QVERIFY(button); + auto *action = window.findChild<QAction *>(QStringLiteral("sync")); + QVERIFY(action); QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", Q_ARG(SyncMonitor::State, SyncMonitor::State::Running)); - QVERIFY(!button->isEnabled()); + QVERIFY(!action->isEnabled()); QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", Q_ARG(SyncMonitor::State, SyncMonitor::State::Unknown)); - QVERIFY2(button->isEnabled(), - "an unobservable lock table left the sync button disabled"); + QVERIFY2(action->isEnabled(), + "an unobservable lock table left the sync action disabled"); } void TestMainWindow::escapeBlanksTheMessagePane() |