diff options
| author | Danilo M. <danix@danix.xyz> | 2026-08-06 20:06:45 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-08-06 20:06:45 +0200 |
| commit | ca89402d7e244e2616fdf13eb35c083afeb21826 (patch) | |
| tree | 34a3278510205e783e023f2c3cbcb2622e501067 | |
| parent | eef7f6cc1b2f845d2ca1eba53336f64597c49e62 (diff) | |
| download | qtmaildir-ca89402d7e244e2616fdf13eb35c083afeb21826.tar.gz qtmaildir-ca89402d7e244e2616fdf13eb35c083afeb21826.zip | |
fix(ui): one Sync control, and a query bar that looks finished
Sync had two controls that behaved differently. The QPushButton beside the
query bar cleared the log, opened the pane and disabled itself; the QAction
behind the toolbar, menu and shortcut called start() and did nothing else,
discarding its return value so a rejected start was silent. Worse, item 29's
"disable Sync during a background sync" set the button only, so the toolbar
entry stayed clickable through a cron sync and could only produce the
script's EX_TEMPFAIL skip.
startSync() is now the single handler behind every route in, and the enabled
state lives on the QAction, which reaches the toolbar, the menu and the
shortcut at once. It also reports when no sync command is configured rather
than doing nothing.
The QPushButton is gone. It read as a Search button given it sat beside a
text field, which is the user's own observation and the reason the toolbar
one survives instead. Its unavailable-command tooltip moved to the action,
since that is the only thing that says why the control is dead.
Removing it left the query field running flush to the window edge, so the
saved-query buttons move from their own row onto the query row. The bar is
now framed by the account dropdown on the left and the saved queries on the
right, the empty row is gone, and the thread list gains the space. The field
also gains setClearButtonEnabled, which is Qt's own themed clear icon rather
than a hand-rolled button. A "Search" button was considered and rejected:
Return already runs the query.
No overflow handling for [queries], which is unbounded. Three entries fit;
item 23 already specifies buttons-plus-menu and is where that belongs.
CLAUDE.md's architecture diagram named four widget classes that have never
existed, QueryBar, SavedQueryBar, HeaderWidget and AttachmentBar. The query
row and the message header are built inline. Corrected, and the components
that do exist but were missing from it added.
Tests: the new action test was verified red first and load-bearing by
mutation. The old button test is deleted rather than repointed, being an
exact duplicate of it, and the unobservable-lock test now drives the action.
The clear button and the row layout were confirmed by hand; no test clicks
the icon, which is a mouse path.
Backlog: 45 done and reclassified as a defect rather than a cosmetic
redundancy, 47 added for the bar.
| -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() |
